1251369 - Use AutoJSAPI in WorkerThreadPrimaryRunnable::Run around DoRunLoop

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Instead of manually reporting exceptions.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Use an AutoJSAPI that reports its own exceptions around the main runloop in workers

Comment 2

[Boris Zbarsky [:bzbarsky]]

This actually causes one interesting behavior change: when loading a toplevel worker that we're not allowed to load (e.g. |new Worker("about:blank")| or in general anything cross-site) we end up in scriptloader::ReportLoadError with NS_ERROR_DOM_BAD_URI, which takes the JS_ReportError path.  Now before this patch we get here with no JS on the stack, so the JS_ReportError reports _immediately_ instead of setting a pending exception.  So we get the random Error with message set to "Failed to load script (nsresult = 0x805303f4)".  The ErrorResult ends up with an uncatchable exception on it, which is then squelched.

With this patch, we still have no JS on the stack, but we took ownership of error reporting.  So we end up setting a pending exception then moving it to the ErrorResult.  Then in ShutdownScriptLoader, we are muted (because we never constructed the mMutedErrorFlag on the LoadInfo in this case: we never got that far).  So we log the exception to console, clear it off the JSContext, and throw NS_ERROR_DOM_NETWORK_ERR on the ErrorResult.  This makes its way up to CompileScriptRunnable::WorkerRun, which goes ahead and sets the pending exception for the NetworkError on the JSContext, where it is duly reported.  So the actual message sent to the error listener on the worker changes.

Now I did just check the spec, and it says absolutely nothing about what the error event's message should be in this case, as far as I can tell.  So we can do whatever the heck we want here...

I could try to figure out a way to preserve our current string thing (e.g. by adding a way to throw a vanilla Error with a string on an ErrorResult).  Or we could, for example, make this case a TypeError or some sort of DOMException (for NS_ERROR_DOM_BAD_URI, changing it to NS_ERROR_DOM_SECURITY_ERR makes some sense to me).

Thoughts?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Preserving the existing behavior seems like a non-goal.  Sensible DOMExceptions are the way to go, IMO.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Use an AutoJSAPI that reports its own exceptions around the main runloop in workers

The silly leading ": " on the error messages is due to bug 1251518.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Use an AutoJSAPI that reports its own exceptions around the main runloop in workers

The silly leading ": " on the error messages is due to bug 1251518.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I found a subtle bug in that patch: for NS_BINDING_ABORTED we do NOT want to ThrowDOMException, because that defeats the checks for that error code in CompileScriptRunnable::WorkerRun and CompileDebuggerScriptRunnable::WorkerRun.  This manifests as an intermittent/perma-fail in dom/media/webaudio/test/test_AudioBuffer.html calling finish() twice: once normally and once from an error event.  Sadly, I have never managed to write a testcase that reliably reproduces the timing involved here, so we don't have an explicit test for this.  :(

In any case, I'll change the NS_BINDING_ABORTED case to just throw that directly on the ErrorResult, without the nicer message, before I check in.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fce4115d2040

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fce4115d2040