Adding elem.style to a WeakMap makes the CC go boom

RESOLVED FIXED in Firefox 11

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla11
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 582388 [details]
testcase (crashes Firefox)

###!!! ASSERTION: Cycle collection participant didn't traverse to preserved wrapper! This will probably crash.: 'callback.mFound', file /builds/slave/m-cen-osx64-dbg/build/content/base/src/nsContentUtils.cpp, line 5471

And then things *really* go wrong.

Regression from bug 680937?
(Assignee)

Comment 1

6 years ago
This test puts the style field of a DOM node into a weak map.

Looking at nsGenericElement, the style field has type nsICSSDeclaration.  
nsComputedDOMStyle and nsDOMCSSAttrDeclaration are both subtypes of that class and of nsWrapperCache.

NS_IMPL_CYCLE_COLLECTION_1(nsDOMCSSAttributeDeclaration, mElement)

NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent)

So these classes seem to be missing the wrapper cache glue in the cycle collector declarations, as can be seen in nsHTMLOptionCollection.

specifically:
NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in Traverse, the NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER in Unlink, as well as 
NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsFoo)
  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
NS_IMPL_CYCLE_COLLECTION_TRACE_END

It seems to me that that's a bug in those classes, but maybe I'm wrong?  I'm going to look at every nsWrapperCache to see any others are missing this.
Assignee: nobody → continuation
(Assignee)

Comment 2

6 years ago
All other uses look okay to me.  I don't see anything for node lists, but I assume that with the node bindings they aren't using the existing CC macros.
(Assignee)

Comment 3

6 years ago
These classes did have the preserved wrapper stuff at one point, but Peter removed them in bug 437449.
(Assignee)

Comment 4

6 years ago
Created attachment 582433 [details] [diff] [review]
alternate way to preserve wrappers

This is an alternate way to preserve wrappers that I found, which seems to pass my existing tests and not crash with Jesse's test case, but I have no idea where this PreserveWrapper is implemented, which is disturbing.

Thanks for the test case, Jesse.
Attachment #582433 - Flags: feedback?(jst)
(Assignee)

Comment 5

6 years ago
Another simpler solution would be to just QI to nsINode or whatever.  That will cover the common cases, and fail on the weird CSS things.
(Assignee)

Comment 6

6 years ago
Created attachment 582582 [details] [diff] [review]
QI wrapped native weak map keys to nsINode, not nsWrapperCache

This is a simple fix to the particular problem.  Instead of QIing to nsWrapperCache, which has some subclasses that can't have their wrappers preserved, we QI to nsINode, which traces its wrapper, and thus is able to have its wrapper preserved.  This should cover almost any case we actually care about.  I also added Jesse's test case.
Attachment #582433 - Attachment is obsolete: true
Attachment #582433 - Flags: feedback?(jst)
Attachment #582582 - Flags: review?(mrbkap)
Comment on attachment 582582 [details] [diff] [review]
QI wrapped native weak map keys to nsINode, not nsWrapperCache

Stealing review. I think this is a good stop gap here until we figure out the "right" way to do this, which may not happen before we have new DOM bindings. The most important case here is IMO to do this right for DOM nodes, and this does exactly that.
Attachment #582582 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

6 years ago
Try run looked good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b4b0ce8ad5
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla11
What's wrong with do_QueryInterface(native, &rv)?
(Assignee)

Comment 10

6 years ago
I'm just not very familiar with all of the various ways to QI, so it looks like I ended up using a lower level one.  Good to know.
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b0b4b0ce8ad5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #7)
> Stealing review. I think this is a good stop gap here until we figure out
> the "right" way to do this, which may not happen before we have new DOM
> bindings. The most important case here is IMO to do this right for DOM
> nodes, and this does exactly that.

Except that in Bug 669240 we want to be able to stick nsIFrameMessageManagers into a weak map, and this moves us farther away from that.
status-firefox-esr10: --- → unaffected
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.