WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.05k stars 167 forks source link

[audioworklet] Error handling in AudioWorkletGlobalScope #1282

Closed hoch closed 5 years ago

hoch commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

I agree this is confusing. I would like to hear from @bfgeek (probably in https://github.com/w3c/css-houdini-drafts/issues/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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

closed see https://github.com/w3c/css-houdini-drafts/issues/433

annevk commented 6 years ago

@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 commented 6 years ago

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

cristiano-belloni commented 6 years ago

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.

hoch commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

cristiano-belloni commented 6 years ago

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

hoch commented 6 years ago

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

annevk commented 6 years ago

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

karlt commented 6 years ago

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 https://github.com/w3c/css-houdini-drafts/issues/407

hoch commented 6 years ago

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 commented 5 years ago

Closing as duplicate https://github.com/WebAudio/web-audio-api-v2/issues/29 in V2