Closed Bug 1183604 Opened 5 years ago Closed 5 years ago
Figure out how to check in debug builds that all wrappercache objects trace through their wrapper
Currently we have assertions for the case there actually is a preserved wrapper, but that doesn't catch errors in tests, since expando properties aren't that common.
Could you put some magic debug-only think into the wrappercache tracing macro boilerplate, so that we could tell if you call a wrappercache-y trace hook?
I'm planning to add some debug stuff the bindings code so that it gets triggered whenever a wrapper is created (doesn't need to be preserved). There are couple of different things, I think, we should have. Ensure sane QI implementation for nsISupports objects and ensure wrapper tracing for all CCables
Let's just make this a security bug for now.
Comment on attachment 8633597 [details] [diff] [review] wip, for nsISupports Review of attachment 8633597 [details] [diff] [review]: ----------------------------------------------------------------- Thank you so much for looking into this and fixing it! ::: dom/bindings/BindingUtils.h @@ +897,5 @@ > + nsWrapperCache* wrapperCacheFromQI = nullptr; > + aObject->QueryInterface(NS_GET_IID(nsWrapperCache), > + reinterpret_cast<void**>(&wrapperCacheFromQI)); > + > + MOZ_ASSERT(wrapperCacheFromQI); Please add a description to this assert so a random DOM developer who hits this assert can figure out how to fix it. @@ +908,5 @@ > + > + nsISupports* ccISupports; > + aObject->QueryInterface(NS_GET_IID(nsCycleCollectionISupports), > + reinterpret_cast<void**>(&ccISupports)); > + MOZ_ASSERT(ccISupports); Likewise. @@ +911,5 @@ > + reinterpret_cast<void**>(&ccISupports)); > + MOZ_ASSERT(ccISupports); > + > + nsXPCOMCycleCollectionParticipant* participant; > + CallQueryInterface(ccISupports, &participant); Do you need to maybe assert that participant is non-null here? If so, also add a description that will help a DOM developer figure out how to fix it. :)
Attachment #8633597 - Flags: review?(continuation) → review+
I guess I should add an assert for participant too.
I think I could push this now to m-i. Waiting for try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=483b5b7b238f
I'll use AutoSuppressGCAnalysis here, since we're dealing strictly C++ objects only and QIing to nsWrapperCache, nsCycleCollectionISupports, nsXPCOMCycleCollectionParticipant which are all very much native. So QIing to those can't GC.
suppress the warning.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42-]
You need to log in before you can comment on or make changes to this bug.