1224664 - Assert if an ErrorResult is accessed on a thread different than the one it's created on

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

People seem to think that this code pattern:

  ErrorResult rv;
  RefPtr<MyRunnable> r = new MyRunnable(rv); // holds reference to rv
  // dispatch r to different thread, spin until it's done, call methods on rv on
  // the other thread.

is somehow OK.  It totally isn't.  If the other thread throws a JS exception on rv, we're in a world of trouble, for example.

We should add asserts in ErrorResult when this happens.  I wish I could make the runtime aborts, but that involves stashing the current thread in the constructor in opt builds, which is too expensive.  :(

And of course we have to fix our existing abusers first.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Note, worker script loader does this using a sync loop to keep it "safe" (if scary).

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's not safe.  See the "If the other thread" bit in comment 0.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I see.  Sorry for skimming.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I'd like to work on this, I've got a patch ready.

I don't know if there are any occurrences of this now (since a long time has passed since this was reported), but I'm happy to fix them too :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: 0001-Bug-1224664-Assert-if-an-ErrorResult-is-accessed-on-.patch

Comment 6

[Emilio Cobos Álvarez (:emilio)]

By the way, I don't know what the semantics of CloneTo and operator= should be, should both ErrorResults have the same mOwningThread?

Comment 7

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

Comment on attachment 8739792 [details] [diff] [review]
0001-Bug-1224664-Assert-if-an-ErrorResult-is-accessed-on-.patch

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

We actually have some helpers for this already that you can use.  See http://hg.mozilla.org/mozilla-central/annotate/1801b99994e4/xpcom/glue/nsISupportsImpl.h#l116  Please use these instead in ErrorResult.h

Comment 8

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> By the way, I don't know what the semantics of CloneTo and operator= should
> be, should both ErrorResults have the same mOwningThread?

Yes, I believe so.

Comment 9

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

Note that we may need to fix comment 1 ... which is referring to Load and LoadMainScript taking an ErrorResult& at http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.h#53 and the ErrorResult& in ScriptLoaderRunnable.  These probably need to be changed to just return an nsresult.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Does the worker code really throw on the ErrorResult on the other thread, though?  LoadMainScript/Load call LoadAllScripts, which hand the ErrorResult& to the ScriptLoaderRunnable, true.  then ScriptLoaderRunnable never directly touches its mRv.  It's touched via mScriptLoader.mRv by the following methods:

ScriptExecutorRunnable::PreRun, ScriptExecutorRunnable::WorkerRun, ScriptExecutorRunnable::PostRun, ScriptExecutorRunnable::ShutdownScriptLoader, ScriptExecutorRunnable::LogExceptionToConsole.  But those all AssertIsOnWorkerThread.  So I think we're good here.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> So I think we're good here.

That makes sense, I tested my first patch and wasn't able to make it crash while testing some Worker demos, what surprised me (that was the reason for comment 4).

Thanks for saving me a bunch of time reading code + with the debugger wondering what could make that assertion not trigger!

Sending a cleaned-up patch right away :)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Cleaned up patch

Thanks for letting me know about those helpers, I missed them!

Comment 13

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

Comment on attachment 8740313 [details] [diff] [review]
Cleaned up patch

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

Awesome, thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

This sees to have gotten lost in the shuffle somehow....

So I did a try push with this patch.  There are some failures:

dom/tests/mochitest/fetch/test_request.html  -- The ConstructorRunnable in URL.cpp uses an ErrorResult created on workers to call URLMainThread::Constructor.  This is ... not OK.  Looks like that was also the state of things before bug 1269161, and in fact has been broken ever since worker URL landed.  This causes various other test failures too.  Filed bug 1286955.

dom/workers/test/serviceworkers/test_workerUpdate.html -- The mStatus of UpdateResultRunnable is constructed (and has an exception thrown on it) on one thread, and then is read on another thread (but that doesn't trigger the asserts; maybe we should assert in the Failed() getter) and then has its exception suppressed on that other thread (unnecessarily, actually; it's already handed the exception to the promise).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03362dd7616d
Assert if an ErrorResult is accessed on a thread different than the one it's created on, r=khuey

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fef388bc6cf
followup: NS_ASSERT_OWNINGTHREAD needs to be #ifdef DEBUG if our NS_DECL_OWNINGTHREAD is.

Comment 17

[Carsten Book [:Tomcat]]

sorry boris, backed out for hazard failures like 


Function '_ZN7mozilla3dom36BrowserElementNextPaintEventCallback4CallEPKc$void mozilla::dom::BrowserElementNextPaintEventCallback::Call(int8*)' has unrooted 'rv' of type 'mozilla::IgnoredErrorResult' live across GC call '_ZN7mozilla3dom36BrowserElementNextPaintEventCallback4CallERNS_11ErrorResultEPKcNS0_14CallbackObject17ExceptionHandlingEP13JSCompartment$void mozilla::dom::BrowserElementNextPaintEventCallback::Call(mozilla::ErrorResult*, int8*, uint32, JSCompartment*)' at obj-analyzed/dist/include/mozilla/dom/BrowserElementBinding.h:233
    BrowserElementBinding.h:232: Call(1,2, rv.IgnoredErrorResult())
    BrowserElementBinding.h:233: Call(2,3, __temp_1 := rv.field:0.operator 159())
    BrowserElementBinding.h:233: Call(3,4, this*.Call(__temp_1*,aExecutionReason*,0,0)) [[GC call]]
    BrowserElementBinding.h:233: Call(4,5, rv.~IgnoredErrorResult())

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9852c943ba72
Backed out changeset 7fef388bc6cf 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe216aea3ab
Backed out changeset 03362dd7616d

Comment 19

[Emilio Cobos Álvarez (:emilio)]

I don't know too much about those (GC, I assume?) hazards, but I'll be willing to debug them and try to fix them.

Do they happen consistently or only on a few tests? Any hints for where should I look at, Boris?

(No rush if you're busy, I can take a deeper look myself, but any kind of hint would be appreciated)

Comment 20

[Boris Zbarsky [:bzbarsky]]

That was fallout from bug 933378, not this bug.  Not your problem, mine.  ;)  Going to reland this.

Comment 21

[Boris Zbarsky [:bzbarsky]]

In particular, see bug 933378 comment 20.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/469d01eebea4
Assert if an ErrorResult is accessed on a thread different than the one it's created on, r=khuey

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/469d01eebea4