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

Categories

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

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main42-])

Attachments

(3 files)

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.
Assignee: nobody → bugs
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
Attachment #8633597 - Flags: review?(continuation)
Attachment #8633597 - Flags: review?(continuation)
Marking sec-want based on bug 1183450
Keywords: sec-want
Attachment #8633597 - Flags: review?(continuation)
Group: core-security
Let's just make this a security bug for now.
Attachment #8633597 - Attachment is private: false
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.
Attached patch +commentsSplinter Review
I think I could push this now to m-i. Waiting for try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=483b5b7b238f
lovely.
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.
Attached patch v3Splinter Review
suppress the warning.
https://hg.mozilla.org/mozilla-central/rev/6b352a8b2f1a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42-]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.