Closed Bug 1251697 Opened 8 years ago Closed 8 years ago

Change worker XHR sync runnables to propagate exceptions via an ErrorResult, not JSContext


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: bzbarsky, Assigned: bzbarsky)



(Whiteboard: btpp-active)


(3 files, 1 obsolete file)

We need this so we can report exceptions between WorkerRun and PostRun in WorkerRunnable::Run.
Whiteboard: btpp-active
Comment on attachment 8724161 [details] [diff] [review]
part 1.  Thread an ErrorResult reference through the worker XHR WorkerThreadProxySyncRunnable implementations

Review of attachment 8724161 [details] [diff] [review]:

::: dom/workers/XMLHttpRequest.cpp
@@ +182,5 @@
>    RefPtr<Proxy> mProxy;
>    nsCOMPtr<nsIEventTarget> mSyncLoopTarget;
> +  // mRv is set on the worker thread by the constructor.  Must not be touched on
> +  // the main thread, except for copying the reference to the ResponseRunnable.
> +  ErrorResult& mRv;

private, please.  Having a reference to stack memory of another thread is a bit disconcerting, best to keep it as locked down as possible.
Attachment #8724161 - Flags: review?(khuey) → review+
Attachment #8724162 - Attachment is obsolete: true
Attachment #8724162 - Flags: review?(khuey)
Comment on attachment 8724240 [details] [diff] [review]
Part 2 adjusted to eliminate issues with the exception on the ErrorResult being clobbered

Review of attachment 8724240 [details] [diff] [review]:

Your commit message is missing some words.

::: dom/workers/XMLHttpRequest.cpp
@@ +240,5 @@
>      mSyncLoopTarget = syncLoop.EventTarget();
>      if (NS_FAILED(NS_DispatchToMainThread(this))) {
> +      mRv.Throw(NS_ERROR_FAILURE);
> +      return;

Honestly I'd rather just MOZ_CRASH here.
Attachment #8724240 - Flags: review?(khuey) → review+
> private, please. 

Done, as part of part 2 to avoid compiler warnings about unused private members.

> Honestly I'd rather just MOZ_CRASH here.

> Your commit message is missing some words.

Added "::Dispatch".
You need to log in before you can comment on or make changes to this bug.