Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[audioworklet] Error handling in AudioWorkletGlobalScope #1282

Closed
hoch opened this issue Aug 4, 2017 · 22 comments
Closed

[audioworklet] Error handling in AudioWorkletGlobalScope #1282

hoch opened this issue Aug 4, 2017 · 22 comments

Comments

@hoch
Copy link
Member

hoch commented Aug 4, 2017

Currently the Worklet infrastructure does not define the error handling clearly. (issue) AudioWorkletGlobalScope can do its own error handling, but it is likely to cause the layering problem when the Worklet infra introduces the error handling later.

When this is addressed by the Worklet infra, downstream the fix to AudioWorkletGlobalScope.

@annevk
Copy link

annevk commented Aug 6, 2017

It seems to me it's somewhat tied to worklets that expose messaging capabilities. And thus far only audio worklets have that.

Though I suppose the benefit for worklets with no messaging capabilities is that they could use it as some kind of error handling fallback? Not entirely sure what the use cases for that would be.

(If all worklets need it though, we should just use EventTarget throughout and port the various window.onerror and corresponding promise handlers to worklets.)

cc @domenic

@hoch
Copy link
Member Author

hoch commented Aug 7, 2017

The messaging capabilities is limited to the pair of AudioWorkletNode/AudioWorkletProcessor. The WorkletGlobalScope itself does not have any messaging channel. Wouldn't it be simpler to have the identical setup with the worker global scope?

@annevk
Copy link

annevk commented Aug 8, 2017

I meant that if you cannot message that exceptions happened to the parent process, it's rather unclear to me what the benefit of a catchall error handler is.

@domenic
Copy link
Contributor

domenic commented Aug 8, 2017

I agree this is confusing. I would like to hear from @bfgeek (probably in w3c/css-houdini-drafts#433) what the intent is for worklets and errors. Typically large sites like to have some mechanism of catching errors and reporting them to the server, but indeed most worklets don't have enough communication capabilities to do that.

@bfgeek
Copy link

bfgeek commented Aug 9, 2017

The rough plan for paintWorklet was to do a reporting API similar to the Content-Security-Policy-Report-Only header for that usecase. (For paintWorklet we don't want a back communication channel from worklet->main).

e.g. a bad proposal

CSS.paintWorklet.addErrorReportingUri('/my-endpoint');

For those worklets that can have this backchannel, those that want it can extend and do a similar thing to workers? E.g.

interface AudioWorklet : Worklet {
  attribute EventHandler onerror;
} 

And get Worklet to inherit from EventTarget for these?

@annevk
Copy link

annevk commented Aug 9, 2017

How would you prevent the paint worklet leaking data that way? (At least I thought part of the concern was breaking the same-origin policy, but I haven't followed closely.)

@bfgeek
Copy link

bfgeek commented Aug 9, 2017

The primary concern for paintWorklet is people encoding paint assumptions into their code, e.g. what a specific browsers pre-paint window is, and attempting to react to it (e.g. a infinite list using this data). With a reporting uri (which I'm not advocating we do now, but when people start asking for it), this data could be batched, and out of band.

@joeberkovitz
Copy link
Contributor

Since we don't have info from the Worklet folks yet, we propose deferring this. Its absence doesn't compromise the API or cause future problems.

@mdjp
Copy link
Member

mdjp commented Mar 27, 2018

closed see w3c/css-houdini-drafts#433

@mdjp mdjp closed this as completed Mar 27, 2018
@annevk
Copy link

annevk commented Mar 28, 2018

@mdjp I think you should keep this open. While a generic variant for all worklets might not be feasible, given that audio worklets have message passing we could do something here. It's fine to wait for a resolution there, but it seems good to keep this for tracking purposes.

@hoch
Copy link
Member Author

hoch commented Apr 5, 2018

Reopening this issue per feedback from @annevk and @domenic.

@hoch hoch reopened this Apr 5, 2018
@cristiano-belloni
Copy link

cristiano-belloni commented Apr 24, 2018

Excuse me if I'm not getting this, but implementation-wise (in Chrome 66), AudioWorkletNodes have a onprocessorerror callback, which has an Event of processorerror type as only argument (pretty useless, since it has no lineno or message). There is even an example of catching errors from Audio Worklets in the GoogleChromeLabs worklet samples.

  • Is this implementation detail related with the present issue?
  • If it is, why an Event (and not an ErrorEvent) is returned? I guess the main use of catching errors in the parent script is logging them; with no lineno, message or fileName, catching errors is pretty useless.

@hoch
Copy link
Member Author

hoch commented Apr 25, 2018

In short, someone needs to do the spec work.

https://html.spec.whatwg.org/multipage/webappapis.html#the-errorevent-interface

ErrorEvent is not exposed to the worklet system yet. So technically the WorkletGlobalScope is not capable of creating an ErrorEvent object to throw. This is why the current Chrome impl does not have that yet.

https://html.spec.whatwg.org/multipage/workers.html#workerglobalscope

Technically we should use OnErrorEventHandler for the proper error handling. In that sense, the regular EventHandler receiving the Event is not a wrong behavior.

The error handling of Worklet infra is still under the discussion, so I prefer to wait to have more spec work on the error handling mechanism before we design the WebAudio specific one.

I believe this is why we pushed this issue to V2 and sorry if this was not clear.

@cristiano-belloni
Copy link

Thanks, @hoch.
Since I care a lot for this feature, is there any things I can do to help / any other place where you'd like me to post about this?

@hoch
Copy link
Member Author

hoch commented Apr 25, 2018

Sure. I think the best bet is the associated Houdini issue. @annevk, @domenic and @bfgeek have better idea what needs to be done here. My preference is to expose ErrorEvent in the WorkletGlobalScope rather than AudioWorkletGlobalScope, but other folks might think otherwise.

Also we might have to escalate the issue in the AudioWG to mark this as a "v1 blocker".

@annevk
Copy link

annevk commented Apr 26, 2018

It seems problematic that Chrome has implemented something here without a specification backing it...

@cristiano-belloni
Copy link

Just please don't take it away :)
ErrorEvent is better than Event, but Event is much better than nothing.

@hoch
Copy link
Member Author

hoch commented Apr 26, 2018

@annevk Chrome implemented the spec as it is now. Can you clarify?

@annevk
Copy link

annevk commented Apr 26, 2018

@hoch my bad, I thought defining error handling in general was deferred until we figured out what it should look like.

@karlt
Copy link
Contributor

karlt commented Sep 21, 2018

onprocessorerror provides a minimal notification of error when an error is associated with a node or its processor, but there is no notification of exceptions during "Run a module script", which is
w3c/css-houdini-drafts#407

@hoch
Copy link
Member Author

hoch commented Sep 21, 2018

I think rejecting the promise from .addModule() might be an obvious solution, but that's not up to us. This is an external dependency and it should be downstreamed when Hoduini group fixes the script loading mechanism.

In that sense, onprocessorerror makes sense because it is specifically for the Audio Worklet operation.

@mdjp
Copy link
Member

mdjp commented Sep 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants