536379 - crashes using firebug / DOM Inspector [@ nsCSSCompressedDataBlock::MapRuleInfoInto(nsRuleData*) const]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Crashes at the signature nsCSSCompressedDataBlock::MapRuleInfoInto(nsRuleData*) const started appearing the day after bug 522595 landed with comments and extension lists and stacks showing things related to messing with style rules in firebug or DOM inspector.

http://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=nsCSSCompressedDataBlock%3A%3AMapRuleInfoInto%28nsRuleData*%29%20const&version=Firefox%3A3.7a1pre

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I couldn't reproduce the problem using the steps in one of the comments, and the stacks aren't quite in good enough shape to figure out what's happening.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I do have a part of a theory as to why this is happening:  we somehow clone the same CSSStyleRuleImpl twice:  first using the DeclarationChanged version (after which it should never be used again!) and then using Clone.  Since I changed CSSStyleRuleImpl::CSSStyleRuleImpl(CSSStyleRuleImpl& aCopy, nsCSSDeclaration* aDeclaration) to transfer ownership of mDeclaration and null it out, the second copy constructor would crash with a null mDeclaration when calling mDeclaration->Clone().

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

Steps to reproduce:
 * install Firebug 1.5b8 from http://getfirebug.com/releases/firebug/1.5X/
 * load this testcase
 * in the DOM view, choose the P
 * choose the style rules tab on the right
 * remove one of the 'color: green' declarations using the red circle-slash icon

This is crashing because the APIs through which DOM Inspector and Firebug get ahold of the matching rules allow them to get to a rule and call removeProperty on it before we've called EnsureUniqueInner on the style sheet.  So then we call EnsureUniqueInner from within RemoveProperty and crash here:

#5  nsTArray_base::Length (this=0x0) at ../../dist/include/nsTArray.h:65
#6  0x00007f68f1f07a21 in nsTArray<unsigned char>::AppendElements<unsigned char> (this=0x3620f70, array=...) at ../../dist/include/nsTArray.h:626
#7  0x00007f68f1f07aa9 in nsAutoTArray (this=0x0)
    at ../../dist/include/nsTArray.h:897
#8  0x00007f68f1f06d67 in nsCSSDeclaration (this=0x0, aCopy=...)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSDeclaration.cpp:79
#9  0x00007f68f1f06e59 in nsCSSDeclaration::Clone (this=0x0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSDeclaration.cpp:1423
#10 0x00007f68f1f4686c in CSSStyleRuleImpl (this=0x36e1c90, aCopy=...)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1311
#11 0x00007f68f1f468f1 in CSSStyleRuleImpl::Clone (this=0x2fcdde0, 
    aClone=@0x7fffb0b264f8)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1450
#12 0x00007f68f1f48a3c in CloneRuleInto (aRule=0x0, aArray=0x36210b0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:801
#13 0x00007f690470ccb6 in nsVoidArray::EnumerateForwards (this=0x2ec9aa0, 
    aFunc=0x7f68f1f48a20 <CloneRuleInto>, aData=0x36210b0)
    at nsVoidArray.cpp:678
#14 0x00007f68f1f4a156 in nsCSSStyleSheetInner (this=0x3621040, aCopy=..., 
    aPrimarySheet=0x3101c50)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:873
#15 0x00007f68f1f4a1e4 in nsCSSStyleSheetInner::CloneFor (this=0x2ec9a30, 
    aPrimarySheet=0x3101c50)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:891
#16 0x00007f68f1f4a22d in nsCSSStyleSheet::EnsureUniqueInner (this=0x3101c50)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:1472
#17 0x00007f68f1f4b5d0 in nsCSSStyleSheet::ReplaceStyleRule (this=0x3101c50, 
    aOld=0x2fcde08, aNew=0x3620ff8)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:1386
#18 0x00007f68f1f4298c in CSSStyleRuleImpl::DeclarationChanged (
    this=0x2fcdde0, aHandleContainer=1)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1494
#19 0x00007f68f1f435b0 in DOMCSSDeclarationImpl::DeclarationChanged (
    this=0x1e3c528)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1085
#20 0x00007f68f1f655a5 in nsDOMCSSDeclaration::RemoveProperty (this=0x1e3c528, 
    aPropID=eCSSProperty_color)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsDOMCSSDeclaration.cpp:374
#21 0x00007f690478a1cf in NS_InvokeByIndex_P (that=0x1e3c528, methodIndex=7, 
    paramCount=2, params=0x7fffb0b269a0)
    at /home/dbaron/builds/mozilla-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Notice that |this| in frame #18 and |this| in frame #11 are the same.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The more I think about it, the more I think that any API that hands back style rules needs to ensure that they're all from unique inners before handing them back.  In the case of inDOMUtils::GetCSSStyleRules, I think this means ensuring all document style sheets are unique (and, if any changed, rerunning rule matching) before handing anything back.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the reason for doing all document style sheets (rather than just the ones that are parents of the matched rules) is that if a document links to the same style sheet more than twice, the first time around we'd only unique-ify one of the occurrences and leave the rest sharing an inner, and then we'd either (depending on how it was implemented) do the wrong thing or have to repeat the process a bunch of times.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> before we've called EnsureUniqueInner on the style sheet.

I thought Firebug had added code to make sure that happens before they poke at rules.  Maybe they screwed it up?

I any case we do need something like comment 5; we have longstanding bugs on that...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: make nsCSSStyleSheet::EnsureUniqueInner return tri-state result

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: add code to nsStyleSet to ensure unique inner on all CSS sheets

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: add code to nsPresContext to ensure it's safe to hand out CSS rules

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: make inDOMUtils ensure unique inners before handing out CSS rules

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: test

Comment 13

[John J. Barton]

Please let me know if anything needs to be done in Firebug for this bug.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Once this bug is fixed, you can remove the Firebug code to poke at stylesheets to make sure they're cloned correctly....  In the meantime, this bug's steps to reproduce point out a case in which said Firebug code is either not being run or not doing what it should.

Comment 15

[John J. Barton]

Any impact on User Agent stylesheets? We disallow edits now and I recall that was related to the touch code

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch alone wouldn't be sufficient for allowing editing of UA sheets, since we don't actually Clone UA sheets in DocumentViewerImpl::CreateStyleSet.  We probably could without too much cost, though, but that ought to be a different bug.

Comment 17

[John J. Barton]

Even tho the title seems a bit of a mismatch the bug I believe is related is:
https://bugzilla.mozilla.org/show_bug.cgi?id=503007

Comment 18

[John J. Barton]

(In reply to comment #7)
> > before we've called EnsureUniqueInner on the style sheet.
> 
> I thought Firebug had added code to make sure that happens before they poke at
> rules.  Maybe they screwed it up?

We do not call our 'touch' code in the test case because window.document.styleSheets has length = 0 so we have no sheets to look for rules to touch. (Also the test case does not crash for me).

Comment 19

[John J. Barton]

(In reply to comment #18)
> (In reply to comment #7)
> > > before we've called EnsureUniqueInner on the style sheet.
> > 
> > I thought Firebug had added code to make sure that happens before they poke at
> > rules.  Maybe they screwed it up?
> 
> We do not call our 'touch' code in the test case because
> window.document.styleSheets has length = 0 so we have no sheets to look for
> rules to touch. (Also the test case does not crash for me).

And the styleSheets.length == 0 because the document has not started loading. The design seems to be to check once, then watch for DOMNodeInserted of '
link' nodes. But the DOMNodeInserted handler assumes that every node is inserted individually which does not seem to be the case. I think this design was added recently in 1.5 to solve some other problem with the 'touch' code. It is attempting to simulate a "stylesheet added" event.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> But the DOMNodeInserted handler assumes that every node is inserted individually

DOMNodeInserted is not fired for nodes inserted by the parser.  You shouldn't be caching things like that before DOMContentLoaded has fired.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419212 [details] [diff] [review]
patch 1: make nsCSSStyleSheet::EnsureUniqueInner return tri-state result

r=bzbarsky

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419213 [details] [diff] [review]
patch 2: add code to nsStyleSet to ensure unique inner on all CSS sheets

This isn't good enough; we need to also ensure unique inners on descendant sheets.  Should be possible to write tests for that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419214 [details] [diff] [review]
patch 3: add code to nsPresContext to ensure it's safe to hand out CSS rules

r=bzbarsky

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419215 [details] [diff] [review]
patch 4: make inDOMUtils ensure unique inners before handing out CSS rules

r=bzbarsky

Comment 25

[Boris Zbarsky [:bzbarsky]]

For the test, could you also grab the first two rules in the list and assert that they have the right .parentStyleSheet?

Comment 26

[Boris Zbarsky [:bzbarsky]]

David, this should also fix bug 253354 and let us take out the workaround mentioned in bug 253354 comment 12, right?

Comment 27

[Steve Roussey (:sroussey)]

"DOMNodeInserted is not fired for nodes inserted by the parser."

Yes, but it would be for inline script that gets run while parsing, correct?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Yes, but it would be for inline script that gets run while parsing, correct?

Yes.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have the child sheets issue fixed in my patch queue.  I'll probably post the patch once I figure out the second problem below.

(In reply to comment #25)
> For the test, could you also grab the first two rules in the list and assert
> that they have the right .parentStyleSheet?

Yeah, so I wrote this test, and another one, and found at least two bugs, only one of which I understand so far.

The one that I do understand is pretty bad, hence this comment being security-private.  I think it can lead to dangling pointers galore.  The problem is this:  if you start of with sheets A and B that have a common inner I, and all the rules in I have pointers to A, and then you call A->EnsureUniqueInner(), you end up with B and A having different inners, all of whose rules have pointers to A.  Then if A goes away (which I think it can, though I haven't confirmed), the problem would get much worse than that.  I'll probably file a separate bug on this later.

The one that I haven't debugged yet is that I actually get the same DOM rule object for the last two rules in the list.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> you end up with B and A having different inners, all of whose rules have
> pointers to A.

I was actually worried about that when I was reviewing, so I checked over the code.  Does the |if (aSheet == mSheets.ElementAt(0)) {| case in nsCSSStyleSheetInner::RemoveSheet not handle this correctly?

> The one that I haven't debugged yet is that I actually get the same DOM rule
> object for the last two rules in the list.

This is with your patches?

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #30)
> > you end up with B and A having different inners, all of whose rules have
> > pointers to A.
> 
> I was actually worried about that when I was reviewing, so I checked over the
> code.  Does the |if (aSheet == mSheets.ElementAt(0)) {| case in
> nsCSSStyleSheetInner::RemoveSheet not handle this correctly?

I guess it should.  So never mind that.

> > The one that I haven't debugged yet is that I actually get the same DOM rule
> > object for the last two rules in the list.
> 
> This is with your patches?

Yes.

So I guess I'll debug the other problem and maybe have clues as to this one as well.  Probably tomorrow, though.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, the whole mess was just that I needed a ClearRuleCascades call; the rest of my analysis in comment 29 to now was wrong.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: add code to nsStyleSet to ensure unique inner on all CSS sheets

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1.5: call ClearRuleCascades() in EnsureUniqueInner()

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: tests

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419630 [details] [diff] [review]
patch 2: add code to nsStyleSet to ensure unique inner on all CSS sheets

r=bzbarsky

It might be nice to also add some tests with @import to exercise this child sheet code.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #36)
> It might be nice to also add some tests with @import to exercise this child
> sheet code.

The revised patch 5 above has a second test file that has @imports, and crashes without the revisions.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #26)
> David, this should also fix bug 253354 and let us take out the workaround
> mentioned in bug 253354 comment 12, right?

Looks like it will, yes.  (Definitely for the workaround; I need to double-check for the bug itself.)

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/42df7ad82c91
http://hg.mozilla.org/mozilla-central/rev/3c2de87b07a5
http://hg.mozilla.org/mozilla-central/rev/442c4f676d52
http://hg.mozilla.org/mozilla-central/rev/2a7ecbab0a69
http://hg.mozilla.org/mozilla-central/rev/3cb9a4b03575
http://hg.mozilla.org/mozilla-central/rev/8bb9bb88e997

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #26)
> David, this should also fix bug 253354 and let us take out the workaround
> mentioned in bug 253354 comment 12, right?

Workaround removed: http://hg.mozilla.org/mozilla-central/rev/8a4d46256951

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For the record, I missed a piece, and fixed it in bug 578810.