1250291 - Get rid of MaybeReportMainThreadException

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like it's reporting things, but it's a lie.  An inconsistently applied lie, afaict.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop pretending to report exceptions in MainThreadStopSyncLoopRunnable::PostDispatch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop pretending to report exceptions in MainThreadWorkerSyncRunnable::PostDispatch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop pretending to report exceptions in WorkerDebuggerRunnable::PostDispatch

Comment 4

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

Comment on attachment 8722150 [details] [diff] [review]
part 1.  Stop pretending to report exceptions in MainThreadStopSyncLoopRunnable::PostDispatch

Review of attachment 8722150 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerRunnable.cpp
@@ +532,5 @@
> +  //
> +  // Our PreDispatch always returns true and is final.  We inherit
> +  // DispatchInternal from StopSyncLoopRunnable, and that itself is final and
> +  // only returns false if dispatch to the syncloop target fails.  So we can
> +  // never have a JS exception here.

FWIW, I'm fairly confident that dispatching to the sync loop target (nsNestedEventTarget) can never fail, but that makes a lot of assumptions about nsThread internals that we probably shouldn't do here.

Comment 5

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

Comment on attachment 8722151 [details] [diff] [review]
part 2.  Stop pretending to report exceptions in MainThreadWorkerSyncRunnable::PostDispatch

Review of attachment 8722151 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/XMLHttpRequest.cpp
@@ +1181,5 @@
> +  DebugOnly<bool> ok =
> +    jsapi.Init(xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx)), aCx);
> +  MOZ_ASSERT(ok);
> +  MOZ_ASSERT(jsapi.cx() == aCx);
> +  jsapi.TakeOwnershipOfErrorReporting();

So there's JSAPI in here that can definitely throw. (e.g. JS_NewArrayObject)  You correctly point out that we weren't reporting that exception before (because PreDispatch still returns true) but presumably we need to do *something* with it ...?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but presumably we need to do *something* with it ...?

Sure.  ~AutoJSAPI will report it, since we called TakeOwnershipOfErrorReporting().

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/649599f02250
https://hg.mozilla.org/integration/mozilla-inbound/rev/660969a64c7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfe211bf2f0

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/649599f02250
https://hg.mozilla.org/mozilla-central/rev/660969a64c7d
https://hg.mozilla.org/mozilla-central/rev/6bfe211bf2f0