Thanks to fixing up some problems with destructor handling, I got the hazard pasted below. The problem is the following code: RefPtr<JS::WasmModule> module = file.mWasmModule; JS::Rooted<JSObject*> wrappedModule(aCx, module->createObject(aCx)); if (NS_WARN_IF(!wrappedModule)) { return nullptr; } result.set(wrappedModule); return result; where 'result' is a Rooted<JSObject*>. The hazard analysis sees that you're returning a JSObject* extracted from a Rooted<JSObject*>, but after you put it on the stack, ~RefPtr fires, possibly deleting the WasmModule and GCing in the process. (I don't know if that GC is actually possible, but it sure seems to run a scary amount of code.) But in this case, I don't think the refcount can go to zero, though there's no simple way to prove it. file.mWasmModule has a reference to it, and 'file' seems very likely to survive past the end of this function call. I'm not sure what I'm going to do with this hazard. Perhaps I should figure out how to prove that ~RefPtr<WasmModule> won't GC? My other idea was using a RefPtrOfLongLivingThing<T>, though the easiest way to do that would be a simple T*, and I'm not sure how keen people would be about that. The hazard: Function '_ZN7mozilla3dom12_GLOBAL__N_134CopyingStructuredCloneReadCallbackEP9JSContextP23JSStructuredCloneReaderjjPv$IDBObjectStore.cpp:JSObject* mozilla::dom::{anonymous}::CopyingStructuredCloneReadCallback(JSContext*, JSStructuredCloneReader*, uint32, uint32, void*)' has unrooted '<returnvalue>' of type 'JSObject*' live across GC call '_ZN6RefPtrIN2JS10WasmModuleEED1Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule]' at dom/indexedDB/IDBObjectStore.cpp:1328 IDBObjectStore.cpp:1328: Assign(139,140, return := __temp_46**) IDBObjectStore.cpp:1328: Call(140,141, wrappedModule.~Rooted()) IDBObjectStore.cpp:1328: Call(141,142, module.~WasmModule>()) [[GC call]] IDBObjectStore.cpp:1328: Call(142,144, result.~Rooted()) IDBObjectStore.cpp:1333: [[end of function]] GC Function: _ZN6RefPtrIN2JS10WasmModuleEED4Ev$RefPtr<T>::~RefPtr() [with T = JS::WasmModule] static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModule; T = JS::WasmModule] static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModule] void js::AtomicRefCounted<T>::Release() const [with T = JS::WasmModule] Utility.h:void js_delete(JS::WasmModule*) [with T = JS::WasmModule] JS::WasmModule.__comp_dtor _ZN2js4wasm6ModuleD1Ev _ZN2js4wasm6ModuleD2Ev void js::wasm::Module::~Module(int32) _ZN2js21ExclusiveWaitableDataINS_4wasm7TieringEED1Ev _ZN2js21ExclusiveWaitableDataINS_4wasm7TieringEED2Ev js::ExclusiveWaitableData<js::wasm::Tiering>::~ExclusiveWaitableData() _ZN2js13ExclusiveDataINS_4wasm7TieringEED2Ev js::ExclusiveData<js::wasm::Tiering>::~ExclusiveData() _ZN2js4wasm7TieringD1Ev _ZN2js4wasm7TieringD2Ev void js::wasm::Tiering::~Tiering(int32) mozilla::Vector<T, N, AllocPolicy>::~Vector() [with T = RefPtr<JS::WasmModuleListener>; long unsigned int MinInlineCapacity = 0ul; AllocPolicy = js::SystemAllocPolicy] _ZN7mozilla6VectorI6RefPtrIN2JS18WasmModuleListenerEELm0EN2js17SystemAllocPolicyEED2Ev _ZN7mozilla6VectorI6RefPtrIN2JS18WasmModuleListenerEELm0EN2js17SystemAllocPolicyEED4Ev static void mozilla::detail::VectorImpl<T, N, AP, IsPod>::destroy(T*, T*) [with T = RefPtr<JS::WasmModuleListener>; long unsigned int N = 0ul; AP = js::SystemAllocPolicy; bool IsPod = false] RefPtr<T>::~RefPtr() [with T = JS::WasmModuleListener] _ZN6RefPtrIN2JS18WasmModuleListenerEED2Ev _ZN6RefPtrIN2JS18WasmModuleListenerEED4Ev static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener; T = JS::WasmModuleListener] static void mozilla::RefPtrTraits<U>::Release(U*) [with U = JS::WasmModuleListener] JS::WasmModuleListener.Release IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Release() IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream() _ZN7mozilla3dom12_GLOBAL__N_124WasmCompiledModuleStreamD1Ev _ZN7mozilla3dom12_GLOBAL__N_124WasmCompiledModuleStreamD2Ev IDBObjectStore.cpp:void mozilla::dom::{anonymous}::WasmCompiledModuleStream::~WasmCompiledModuleStream(int32) IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::Close() IDBObjectStore.cpp:uint32 mozilla::dom::{anonymous}::WasmCompiledModuleStream::CloseWithStatus(uint32) nsIInputStream.Close unresolved nsIInputStream.Close

(In reply to Steve Fink [:sfink] [:s:] from comment #0) That's interesting. Can we end up doing a CC (and hence GC) when we release a reference? It does seem that this could cause us to run pretty much any code. In this case, do we need to extract mWasmModule into a new separate RefPtr or can we just do file.mWasmModule->createObject(aCx)? I'm assuming that can't clear file.mWasmModule or something.

(In reply to Jon Coppeard (:jonco) from comment #1) > (In reply to Steve Fink [:sfink] [:s:] from comment #0) > That's interesting. Can we end up doing a CC (and hence GC) when we release > a reference? It does seem that this could cause us to run pretty much any > code. > > In this case, do we need to extract mWasmModule into a new separate RefPtr > or can we just do file.mWasmModule->createObject(aCx)? I'm assuming that > can't clear file.mWasmModule or something. That's a great question! And I don't know the answer. I don't understand anything about RefPtr. But if it did clear file.mWasmModule, I don't think it would be a problem. Or rather, if it were a problem, it would report a hazard in the code that did the clear. As long as we get the JSObject* back successfully, there's nothing else that will trigger a GC after the return value is placed on the stack. baku, is the right fix to do file.mWasmModule->createObject(aCx) here, or if not, is there another option? Oh... I guess one way to do this would be just { RefPtr<JS::WasmModule> module = file.mWasmModule; JS::Rooted<JSObject*> wrappedModule(aCx, module->createObject(aCx)); if (NS_WARN_IF(!wrappedModule)) { return nullptr; } result.set(wrappedModule); } return result; Then ~RefPtr can GC all it wants while result is safely wrapped up in a Rooted. But I'm leaving the needinfo, as that seems simpler & cleaner if it's valid.

Better than a needinfo, here's a review request. It seems to compile, and fixes the two hazards.

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac2b54e22a8 Fix hazard in CopyingStructuredCloneReadCallback, r=baku

Comment on attachment 8997576 [details] [diff] [review] Fix hazard in CopyingStructuredCloneReadCallback Approval Request Comment [Feature/Bug causing the regression]: longstanding [User impact if declined]: bug 1404274 is blocked from landing onto esr60. I don't know if that means we want to land this on beta to reduce the delta between beta and esr60? [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: it is in mozilla-central [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: it shifts around some operations slightly to remove a problematic series of actions that the rooting hazard analysis flags. [String changes made/needed]: none [esr60 Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed for bug 1404274 landing (to avoid a rooting hazard) User impact if declined: see other bug. But this cleans up some sketchiness as well -- I don't see how to trigger it, but if something JS::WasmModule::createObject were to cause the WasmModule to self-destruct, then this is a theoretical UAF-style rooting hazard. Like I said, I don't see how that would be actually possible, but something could be added in the future or maybe there's an angle to this that I'm not seeing. Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): very low risk. It's just shifting some operations around a little. String or UUID changes made by this patch: none

Comment on attachment 8997576 [details] [diff] [review] Fix hazard in CopyingStructuredCloneReadCallback Let's take this for beta - sounds like a good idea to keep 62 in sync with ESR here even if the issue isn't showing up obviously in 62 beta.

Comment on attachment 8997576 [details] [diff] [review] Fix hazard in CopyingStructuredCloneReadCallback Needed to uplift bug 1404274 to ESR60. Approved for ESR 60.2.