Get rid of MaybeReportMainThreadException

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

It looks like it's reporting things, but it's a lie.  An inconsistently applied lie, afaict.
Created attachment 8722150 [details] [diff] [review]
part 1.  Stop pretending to report exceptions in MainThreadStopSyncLoopRunnable::PostDispatch
Attachment #8722150 - Flags: review?(khuey)
Created attachment 8722151 [details] [diff] [review]
part 2.  Stop pretending to report exceptions in MainThreadWorkerSyncRunnable::PostDispatch
Attachment #8722151 - Flags: review?(khuey)
Created attachment 8722152 [details] [diff] [review]
part 3.  Stop pretending to report exceptions in WorkerDebuggerRunnable::PostDispatch
Attachment #8722152 - Flags: review?(khuey)
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.
Attachment #8722150 - Flags: review?(khuey) → review+
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 ...?
Attachment #8722151 - Flags: review?(khuey) → review+
> but presumably we need to do *something* with it ...?

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

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/649599f02250
https://hg.mozilla.org/mozilla-central/rev/660969a64c7d
https://hg.mozilla.org/mozilla-central/rev/6bfe211bf2f0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.