Stop messing about with JSContexts in WorkerMainThreadRunnable::Dispatch

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(16 attachments, 1 obsolete attachment)

5.87 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.96 KB, patch
khuey
: review+
Details | Diff | Splinter Review
6.56 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.46 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.56 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.10 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.11 KB, patch
khuey
: review-
Details | Diff | Splinter Review
2.87 KB, patch
khuey
: review+
Details | Diff | Splinter Review
8.19 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.23 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.31 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.27 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.31 KB, patch
khuey
: review+
Details | Diff | Splinter Review
15.96 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.12 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.58 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
The current setup is all weird.  Some callers just leave the exception (if any) dangling.  Some report it immediately instead of propagating to caller (why?).

My plan is as follows:

1)  Introduce a version of Dispatch() that takes an ErrorResult instead of a JSContext.

2)  Convert all consumers to using it.  They will then probably all reliably throw the exception to caller.

3)  Burn the JSContext version with fire.
> They will then probably all reliably throw the exception to caller.

I'm such an optimist.  Some of this code has nowhere to get even close to throwing.  :(  But at least now it could try if it wanted to....
Comment on attachment 8687486 [details] [diff] [review]
part 14.  Switch URL to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/URL.cpp
@@ +419,5 @@
>    : WorkerMainThreadRunnable(aWorkerPrivate)
>    , mValue(aValue)
>    , mType(aType)
>    , mURLProxy(aURLProxy)
> +  , mFailed(false)

This means that we don't propagate the correct ErrorResult. What about if we use nsresult here and we set it back to the original ErrorResult?

Maybe we can do:

class BaseSetterGetterRunnable : public WorkerMainThreadRunnable
{
public:
  BaseSetterGetterRunnable(ErrorResult& aRv) : mRv(aRv) {}

  bool Dispatch() {
    if (WorkerMainThreadRunnable::Dispatch() && NS_FAILED(mNSResult)) {
      mRv.Throw(mNSResult);
    }
  }

private:
  void SetNSResult(nsresult aRv) { mNSResult = aRv; }
}

Then, all the setter and getter classes can just do: SetNSResult(localRv.StealNSResult) and the rest of the code is untouched.
Attachment #8687486 - Flags: review?(amarchesini) → review-
> This means that we don't propagate the correct ErrorResult.

Yes, but does it matter?  The only case we propagate it for is SetHref (both before and after the patch).  The only errors it throws are:

1)  Failure to get the nsIIOService.  This throws whatever nsresult GetService returned, but honestly, that's just not going to happen on this codepath: if we couldn't create an ioservice, we wouldn't even _have_ a worker.

2)  Failure to do NewURI.  In this case, the result returned by NewURI is ignored and the code instead does ThrowTypeError<MSG_INVALID_URL>, which is exactly what I did in SetHref if we come back with a failure.

> What about if we use nsresult here and we set it back to the original ErrorResult?

That won't work for ErrorResults which had ThrowTypeError, or in fact anything other than the basic Throw() called on them.

> Then, all the setter and getter classes can just do:
> SetNSResult(localRv.StealNSResult) and the rest of the code is untouched.

I'm not sure what you're suggesting here.  Again the only getter or setter on URL that can throw per spec is the "href" setter.  That's what we currently implement for the worker URL, afaict.
Flags: needinfo?(amarchesini)
Comment on attachment 8687486 [details] [diff] [review]
part 14.  Switch URL to using the new WorkerMainThreadRunnable::Dispatch signature

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

Thanks for you comment.
Attachment #8687486 - Flags: review- → review+
Flags: needinfo?(amarchesini)
Comment on attachment 8687487 [details] [diff] [review]
part 16.  Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

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

I'm sorry, but I'm not sure what we really get from propagating this boolean failure back.  It doesn't seem to be doing anything useful to me.  Can you explain what its actually causing to be cleaned up or shutdown?  I'm not seeing it.

Thanks.

::: dom/fetch/Fetch.cpp
@@ +548,5 @@
>  
>    bool
>    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>    {
> +    return mFetchBody->ContinueConsumeBody(mStatus, mLength, mResult);

What do we really gain from returning false from WorkerRun()?  I guess WorkerRunnable::PostRun() calls JS_ReportPendingException() in this case?  I don't think we have any pending exceptions from the dispatch failure, though.

@@ +620,5 @@
>          }
>        } else {
>          mBody->ContinueConsumeBody(NS_ERROR_FAILURE, 0, nullptr);
> +        // XXXbz And if that fails and we're supposed to bail out of
> +        // everything, then what?

You can use MOZ_ALWAYS_TRUE() here.  This code is AssertIsOnMainThread(), while ContinueConsumeBody() only ever returns false on the worker thread.

@@ +755,5 @@
>    {
>      MOZ_ASSERT(aStatus > workers::Running);
>      if (!mWasNotified) {
>        mWasNotified = true;
> +      return mBody->ContinueConsumeBody(NS_BINDING_ABORTED, 0, nullptr);

Returning false here does nothing other than triggering a warning statement:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5163

Since we already get a warning in ContinueConsumeBody() we probably don't need that.  I think we should always just return true.

@@ +987,3 @@
>            NS_WARNING("Could not dispatch CancelPumpRunnable. Nothing we can do here");
> +          // XXXbz We're really supposed to totally bail out of everything
> +          // here... Ah, well.

Is there anything to bail out of if the main thread is gone?  Its not like the pump is going to continue to run...

@@ +987,5 @@
>            NS_WARNING("Could not dispatch CancelPumpRunnable. Nothing we can do here");
> +          // XXXbz We're really supposed to totally bail out of everything
> +          // here... Ah, well.
> +          localPromise->MaybeReject(rv);
> +          return false;

I'm personally not sure its worth propagating this boolean back.  Its generally not used for anything useful.  To the casual reader, however, it appears significant.  I think it would be better to just not return anything here and avoid confusion.
Attachment #8687487 - Flags: review?(bkelly) → review-
> but I'm not sure what we really get from propagating this boolean failure back.

Ben (Turner) told me the contract for WorkerRun (and in fact pretty much everything in workers that takes a JSContext argument) is that if you hit a syncloop failure under it that means the worker has been shut down and you have to stop everything and return false until you get back to the main worker event loop.  This code is clearly violating that contract, and I don't claim that after my changes it _follows_ it, but it's a bit closer.

> I don't think we have any pending exceptions from the dispatch failure, though.

You totally do in today's world if the actual dispatch to mainthread fails.  If the syncloop fails, you're supposed to act like an uncatchable exception and stop doing everything, per above.

> You can use MOZ_ALWAYS_TRUE() here.

OK.

> Is there anything to bail out of if the main thread is gone?

I don't know.  I'm not particularly familiar with this code, just trying to make it obey the invariants I'm told these things are supposed to obey.

I'm happy to just eat the exception instead if you think that's the right thing in ContinueConsumeBody (rv.SuppressException() and move on with life).  But,again, that's not what I'm told worker code expects to happen in general.
Flags: needinfo?(bkelly)
OK, talked to bkelly.  Summary:

1)  The return value of WorkerRun only affects one thing: what gets passed to PostRun.  These runnables in Fetch don't override PostRun, so it only matters if there is an exception on the JSContext.  We know there isn't one.  So no reason to return false from WorkerRun.

2)  As bkelly points out, in ~AutoFailConsumeBody we're on the main thread so don't reach this runnable codepath at all.

3)  FetchBodyFeature::Notify is an override of WorkerFeature::Notify.  Looks like the only caller is WorkerPrivate::NotifyFeatures which basically ignores the return value.

Given all that, going to change ContinueConsumeBody back to just returning void and doing SuppressException.
Attachment #8687487 - Attachment is obsolete: true
Comment on attachment 8688206 [details] [diff] [review]
part 16.  Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

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

Thanks!
Attachment #8688206 - Flags: review?(bkelly) → review+
Flags: needinfo?(bkelly)
Comment on attachment 8687473 [details] [diff] [review]
part 1.  Add a version of WorkerMainThreadRunnable::Dispatch that takes an ErrorResult to report failure to dispatch on

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

::: dom/workers/WorkerRunnable.cpp
@@ +589,5 @@
> +
> +  if (NS_FAILED(NS_DispatchToMainThread(runnable.forget(), NS_DISPATCH_NORMAL))) {
> +    aRv.ThrowTypeError<MSG_NO_MAINTHREAD_DISPATCH>();
> +    return;
> +  }

Based on my reading of the code in xpcom/threads, I think it's impossible for this to fail at least through xpcom-shutdown-threads. And all worker threads are joined then.

If you agree, I would like to just assert that here.

@@ +595,5 @@
> +  if (!syncLoop.Run()) {
> +    // XXXbz or should this just throw an uncatchable exception instead?  That
> +    // way we could even throw it from promise-returning methods...
> +    aRv.ThrowTypeError<MSG_WORKER_SHUTTING_DOWN>();
> +  }

Yes, it should probably throw an uncatchable exception. There are probably some places where we return false where that's overkill though. e.g. http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133
Attachment #8687473 - Flags: review?(khuey) → review+
Comment on attachment 8687476 [details] [diff] [review]
part 4.  Switch WebSocket to using the new WorkerMainThreadRunnable::Dispatch signature

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

These changes look fine. If you want answers to your questions you should probably ask baku or smaug.
Attachment #8687476 - Flags: review?(khuey) → review+
Comment on attachment 8687478 [details] [diff] [review]
part 6.  Switch BroadcastChannel to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +387,5 @@
>  
>      RefPtr<InitializeRunnable> runnable =
>        new InitializeRunnable(workerPrivate, origin, principalInfo,
>                               privateBrowsing, aRv);
> +    runnable->Dispatch(aRv);

Hmm shouldn't this also return null if dispatch fails (and aRv is never set by InitializeRunnable)?
Attachment #8687478 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #29)
> Comment on attachment 8687478 [details] [diff] [review]
> part 6.  Switch BroadcastChannel to using the new
> WorkerMainThreadRunnable::Dispatch signature
> 
> Review of attachment 8687478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/broadcastchannel/BroadcastChannel.cpp
> @@ +387,5 @@
> >  
> >      RefPtr<InitializeRunnable> runnable =
> >        new InitializeRunnable(workerPrivate, origin, principalInfo,
> >                               privateBrowsing, aRv);
> > +    runnable->Dispatch(aRv);
> 
> Hmm shouldn't this also return null if dispatch fails (and aRv is never set
> by InitializeRunnable)?

Er, ignore that, I got confused.
Comment on attachment 8687479 [details] [diff] [review]
part 7.  Switch ImageBitmap to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/canvas/ImageBitmap.cpp
@@ +1069,5 @@
>      ErrorResult rv;
>      RefPtr<DecodeBlobInMainThreadSyncTask> task =
>        new DecodeBlobInMainThreadSyncTask(mWorkerPrivate, *mBlob, mCropRect,
>                                           rv, getter_AddRefs(data));
> +    task->Dispatch(rv); // This is a synchronous call.

This runnable returns false if rv gets set, which is bogus. Want to fix that too?
Attachment #8687479 - Flags: review?(khuey) → review-
> I think it's impossible for this to fail at least through xpcom-shutdown-threads.

Hmm.  So this can fail if nsThreadManager::Shutdown has been called, which certainly happens after xpcom-shutdown-threads.

It can also fail if nsThread::Dispatch fails, which can only happen if DispatchInternal called with a null nsNestedEventTarget* fails.  That can fail if:

1) The runnable is null.  Let's assume we don't do that.
2) gXPCOMThreadsShutDown.  This is only set after xpcom-shutdown-threads.
3) Some cases in the DISPATCH_SYNC stuff, not relevant here.
4) PutEvent() fails.  This can only happen if queue is null or if mEventsAreDoomed.  Since
   aTarget is null, queue can't be null.  mEventsAreDoomed gets set once we have exited
   MessageLoop::Run in ThreadFunc.  As far as I can tell, we never call ThreadFunc on the
   main thread, so we never set mEventsAreDoomed there.

Given that analysis, I agree with your claim.  Nathan thinks we should be setting mEventsAreDoomed _sometime_ on the main thread, though.  I assume if we do it will be after xpcom-shutdown-threads.  I'll change to asserting.

> Yes, it should probably throw an uncatchable exception.

OK, done.  

> There are probably some places where we return false where that's overkill though. e.g.
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133

Seems to me like throwing uncatchable exception there is fine.  If we're going away, we want to just stop the JS that's calling the URL constructor and go away.

> This runnable returns false if rv gets set, which is bogus. Want to fix that too?

You mean in MainThreadRun?  Done.  What _are_ the semantics of the return value of MainThreadRun, exactly?
Flags: needinfo?(khuey)
> You mean in MainThreadRun?  Done. 

On, and was comment 31 "r+ if you fix this one return value", or are there other issues?
(In reply to Boris Zbarsky [:bz] from comment #32)
> > I think it's impossible for this to fail at least through xpcom-shutdown-threads.
> 
> Hmm.  So this can fail if nsThreadManager::Shutdown has been called, which
> certainly happens after xpcom-shutdown-threads.
> 
> It can also fail if nsThread::Dispatch fails, which can only happen if
> DispatchInternal called with a null nsNestedEventTarget* fails.  That can
> fail if:
> 
> 1) The runnable is null.  Let's assume we don't do that.
> 2) gXPCOMThreadsShutDown.  This is only set after xpcom-shutdown-threads.
> 3) Some cases in the DISPATCH_SYNC stuff, not relevant here.
> 4) PutEvent() fails.  This can only happen if queue is null or if
> mEventsAreDoomed.  Since
>    aTarget is null, queue can't be null.  mEventsAreDoomed gets set once we
> have exited
>    MessageLoop::Run in ThreadFunc.  As far as I can tell, we never call
> ThreadFunc on the
>    main thread, so we never set mEventsAreDoomed there.
> 
> Given that analysis, I agree with your claim.  Nathan thinks we should be
> setting mEventsAreDoomed _sometime_ on the main thread, though.  I assume if
> we do it will be after xpcom-shutdown-threads.  I'll change to asserting.

I'm 99.9% sure Nathan is wrong. The only place I can see where we set it is at http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#401, and on line 372 it's pretty clear we're dealing with non-main-threads here.

> > Yes, it should probably throw an uncatchable exception.
> 
> OK, done.  
> 
> > There are probably some places where we return false where that's overkill though. e.g.
> > http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133
> 
> Seems to me like throwing uncatchable exception there is fine.  If we're
> going away, we want to just stop the JS that's calling the URL constructor
> and go away.

Perhaps this is a bad example since it really shouldn't happen in practice, but what about taking that early return suggests that we're going away.

> > This runnable returns false if rv gets set, which is bogus. Want to fix that too?
> 
> You mean in MainThreadRun?  Done.  What _are_ the semantics of the return
> value of MainThreadRun, exactly?

That return value gets propogated to be the return value of AutoSyncLoopHolder::Run.
Flags: needinfo?(khuey)
(In reply to Boris Zbarsky [:bz] from comment #33)
> > You mean in MainThreadRun?  Done. 
> 
> On, and was comment 31 "r+ if you fix this one return value", or are there
> other issues?

Yes. I don't see any other problems.
> I'm 99.9% sure Nathan is wrong

Sorry, I was unclear.  He agrees we're not ever setting that boolean to true on the main thread right now.  He thinks we should change to doing so.

> but what about taking that early return suggests that we're going away.

Oh, I was talking specifically about throwing an uncatchable exception if the syncloop call in runnable dispatch returns false.  As I understand, _that_ can only happen if we're going away.  Is that not the case?

> That return value gets propogated to be the return value of AutoSyncLoopHolder::Run.

I see.  I'll try to add some docs for that.

> Yes. I don't see any other problems.

OK, great.  I made that change.
> Is that not the case?

I guess given what you said about MainThreadRun's return value, false return from syncloop really doesn't mean so much....
Flags: needinfo?(khuey)
(In reply to Boris Zbarsky [:bz] from comment #37)
> > Is that not the case?
> 
> I guess given what you said about MainThreadRun's return value, false return
> from syncloop really doesn't mean so much....

It should only be used in cases where we want to kill the running script in the worker. Those should be quite rare.
Flags: needinfo?(khuey)
Comment on attachment 8687480 [details] [diff] [review]
part 8.  Switch Notification to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/notification/Notification.cpp
@@ +2193,5 @@
> +    r->Dispatch(rv);
> +    // XXXbz I'm told throwing and returning false from here is pointless (and
> +    // also that doing sync stuff from here is really weird), so I guess we just
> +    // suppress the exception on rv, if any.
> +    rv.SuppressException();

Yeah, this is kind of fucked.  Can you file a bug on making this not-sync?
Attachment #8687480 - Flags: review?(khuey) → review+
Comment on attachment 8687483 [details] [diff] [review]
part 11.  Switch WorkerNavigator to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/Navigator.cpp
@@ +137,5 @@
>                                              eventProxy);
> +    ErrorResult rv;
> +    runnable->Dispatch(rv);
> +    if (rv.Failed()) {
> +      ThrowMethodFailed(aCx, rv);

What is ThrowMethodFailed? And why can't I find it in MXR?
Comment on attachment 8687484 [details] [diff] [review]
part 12.  Switch ServiceWorkerRegistration to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +1114,1 @@
>      }

This gets called from Notify, so in practice I don't this ever happens.  Suppressing rv seems fine to me.
Attachment #8687484 - Flags: review?(khuey) → review+
> Yeah, this is kind of fucked.  Can you file a bug on making this not-sync?

Bug 1227397 filed.

> What is ThrowMethodFailed? And why can't I find it in MXR?

It used to be the way to take an ErrorResult and throw it on a JSContext.

You can't find it because it got removed in bug 1224007 a few days ago.  The corresponding bit in the merge I have locally is now:

+    ErrorResult rv;
+    runnable->Dispatch(rv);
+    if (rv.MaybeSetPendingException(aCx)) {
+      return nullptr;
+    }
Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.