Closed Bug 1224664 Opened 4 years ago Closed 3 years ago
Assert if an Error
Result is accessed on a thread different than the one it's created on
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?
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.
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 :)
Thanks for letting me know about those helpers, I missed them!
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+
Assignee: nobody → ecoal95
3 years ago
Depends on: 1286955
3 years ago
Depends on: 1286956
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 firstname.lastname@example.org: 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 email@example.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())
Backout by firstname.lastname@example.org: 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
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.
In particular, see bug 933378 comment 20.
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.