Figure out how to check in debug builds that all wrappercache objects trace through their wrapper

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({sec-want})

36 Branch
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42-])

Attachments

(3 attachments)

Assignee

Description

4 years ago
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

Updated

4 years ago
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?
Assignee

Comment 2

4 years ago
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
Assignee

Updated

4 years ago
Attachment #8633597 - Flags: review?(continuation)
Assignee

Updated

4 years ago
Attachment #8633597 - Flags: review?(continuation)

Comment 5

4 years ago
Marking sec-want based on bug 1183450
Keywords: sec-want
Assignee

Updated

4 years ago
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+
Assignee

Comment 9

4 years ago
I guess I should add an assert for participant too.
Assignee

Comment 10

4 years ago
Posted 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
Assignee

Comment 13

4 years ago
lovely.
Assignee

Comment 14

4 years ago
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.
Assignee

Comment 15

4 years ago
Posted patch v3Splinter Review
suppress the warning.
https://hg.mozilla.org/mozilla-central/rev/6b352a8b2f1a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

4 years ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.