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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We need this so we can report exceptions between WorkerRun and PostRun in WorkerRunnable::Run.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Thread an ErrorResult reference through the worker XHR WorkerThreadProxySyncRunnable implementations

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Have WorkerThreadProxySyncRunnable hand the ErrorResult reference it holds to its ResponseRunnable so it can report exceptions on there instead of on a JSContext

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the JSContext argument of StopSyncLoopRunnable::MaybeSetException

Comment 4

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

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 adjusted to eliminate issues with the exception on the ErrorResult being clobbered

Comment 6

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

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> private, please. 

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

> Honestly I'd rather just MOZ_CRASH here.

Done.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Your commit message is missing some words.

Added "::Dispatch".

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ce9810ffe6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94ad4babe1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d9ecda1acf

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97ce9810ffe6
https://hg.mozilla.org/mozilla-central/rev/f94ad4babe1d
https://hg.mozilla.org/mozilla-central/rev/e2d9ecda1acf