Last Comment Bug 777373 - CssRuleView.jsm uses invalid weak map keys
: CssRuleView.jsm uses invalid weak map keys
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 17
Assigned To: Dave Camp (:dcamp)
:
Mentors:
Depends on:
Blocks: 761620 777877
  Show dependency treegraph
 
Reported: 2012-07-25 09:09 PDT by Andrew McCreight (PTO-ish through 6-29) [:mccr8]
Modified: 2012-07-31 07:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
excerpts from a Moth run, showing where the weak map put failed (6.41 KB, text/plain)
2012-07-25 09:11 PDT, Andrew McCreight (PTO-ish through 6-29) [:mccr8]
no flags Details
Use a Map for now (4.38 KB, patch)
2012-07-26 12:59 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Review

Description Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-07-25 09:09:08 PDT
Line 490 in CssRuleView.jsm uses (I think) a nsIDOMCSSStyleDeclaration as a key in the weak map |disabled| [1].  Unfortunately, this is not okay because the only C++ things we support as weak map keys are nodes (for technical reasons, we only can only support objects that support wrapper preservation, and just allowing nodes was a simple way to do that).

The current behavior is that the key will be successfully added (with a warning), but it can eventually get its entry in the weak map thrown out during a later GC.  In bug 761620, I am planning to make it so that unsupported keys are not added, and an exception is thrown, so it is more obvious that something has gone wrong.

To fix this, you either have to switch to using some other kind of key, which could be tricky.  If you can get back and forth between the style and its node maybe that would work.  Otherwise, you may need to switch to another data structure, like a map, though then you have to be more careful to avoid leaks.

In the longer term, in bug 753517, CSS things are going to be converted over to the new DOM bindings, which I believe means they will be wrapper cached, and thus we can loosen the restriction on weak map keys to include these CSS things.  But you probably want to fix this before that.

CCing Boris who understands what is going on with wrapper preservation of CSS stuff.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/CssRuleView.jsm#490
Comment 1 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-07-25 09:11:58 PDT
Created attachment 645786 [details]
excerpts from a Moth run, showing where the weak map put failed
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-07-25 09:13:08 PDT
I guess these failing test are in the Style Inspector and Highlighter directories, not the Style Editor.
Comment 3 Dave Camp (:dcamp) 2012-07-25 09:14:21 PDT
We can do something different here without too much trouble.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-25 09:25:52 PDT
Until bug 753517 happens, css declarations do have a wrapper cache, but don't do wrapper preservation.
Comment 5 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-07-25 09:30:25 PDT
Oops, yeah, that's what I meant...
Comment 6 Dave Camp (:dcamp) 2012-07-25 16:06:21 PDT
Switching to a Map would mean retaining the DOMCSSStyleDeclaration of any declaration edited by the rule view until the inspector is closed or the page is reloaded.

If I'm reading this right (and I'm probably not), holding a DOMCSSStyleDeclaration won't actually retain any style data.  Is that accurate?

If so, the impact of using a Map instead of a WeakMap will be pretty small.  If bug 753517 is going to let us use WeakMap support and is coming soonish, I'm inclined to just use a Map for now.  Anybody disagree?
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-25 19:30:51 PDT
> Is that accurate?

It depends on what you mean by "style data".

If you're holding onto a DOMCSSDeclarationImpl, then it'll keep alive the DOMCSSStyleRule.  It won't keep alive the underlying css::StyleRule.

So I think you'll just be out sizeof(DOMCSSStyleRule) but nothing else...

If bug 753517 doesn't land this cycle, then I really screwed up somewhere.
Comment 8 Dave Camp (:dcamp) 2012-07-26 12:59:28 PDT
Created attachment 646275 [details] [diff] [review]
Use a Map for now
Comment 9 Rob Campbell [:rc] (:robcee) 2012-07-27 14:32:58 PDT
Comment on attachment 646275 [details] [diff] [review]
Use a Map for now

r+... FOR NOW!
Comment 11 Tim Taubert [:ttaubert] 2012-07-31 07:40:45 PDT
https://hg.mozilla.org/mozilla-central/rev/f08a7ecc6d9f

Note You need to log in before you can comment on or make changes to this bug.