Closed Bug 1224664 Opened 4 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox45 --- affected
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tw-dom])

Attachments

(1 file, 1 obsolete file)

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.
Note, worker script loader does this using a sync loop to keep it "safe" (if scary).
It's not safe.  See the "If the other thread" bit in comment 0.
I see.  Sorry for skimming.
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 :)
By the way, I don't know what the semantics of CloneTo and operator= should be, should both ErrorResults have the same mOwningThread?
Flags: needinfo?(khuey)
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
Attachment #8739792 - Flags: review?(khuey) → review-
(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.
Flags: needinfo?(khuey)
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.
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.
(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 :)
Attached patch Cleaned up patchSplinter Review
Thanks for letting me know about those helpers, I missed them!
Attachment #8739792 - Attachment is obsolete: true
Attachment #8740313 - Flags: review?(khuey)
Comment on attachment 8740313 [details] [diff] [review]
Cleaned up patch

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

Awesome, thanks!
Attachment #8740313 - Flags: review?(khuey) → review+
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).
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fef388bc6cf
followup: NS_ASSERT_OWNINGTHREAD needs to be #ifdef DEBUG if our NS_DECL_OWNINGTHREAD is.
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())
Flags: needinfo?(ealvarez)
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)
Flags: needinfo?(ealvarez) → needinfo?(bzbarsky)
That was fallout from bug 933378, not this bug.  Not your problem, mine.  ;)  Going to reland this.
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/469d01eebea4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.