578810 - Crash at [@ nsTArray<unsigned char>::AppendElements<unsigned char>(nsTArray<unsigned char> const&) ] using DOMi inspector on Page Info Window

Status: VERIFIED FIXED

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

Description

[Tanner M. Young [:tmyoung]]

In testing possible CSS values for Bug 431644, I found that setting the border-top-width to 0px on the xul splitter in the Media Panel caused a crash at [@ nsTArray<unsigned char>::AppendElements<unsigned char>(nsTArray<unsigned char> const&) ]

http://crash-stats.mozilla.com/report/index/53d16c06-387a-4139-9642-9a5e72100714

STR:
1. Install DOMi
2. Open a Page Info dialog with a Media Panel
3. View Chrome Window in DOMi
4. Select #document > window > deck > vbox (mediaPanel) > splitter
5. Change splitter[orient="vertical"] border-top-width to 0px

This has been reproduced in two clean profiles.

Mozilla/5.0 (Windows; Windows NT 6.0; WOW64; en-US; rv:2.0b2pre) Gecko/20100714 Minefield/4.0b2pre ID:20100714040818

xul!nsTArray<unsigned char>::AppendElements<unsigned char>+0x4:
63383447 8b00            mov     eax,dword ptr [eax]  ds:002b:00000000=????????

The line that WinDBG tells me is crashing is nstarray.h:665

      return AppendElements(array.Elements(), array.Length());

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#665

Comment 1

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

This is a null-dereference:  the this pointer in the nsCSSDeclaration::Clone call is null.  (Investigating further now.)

Comment 2

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

The problem here is, I think, that |this| in frame #22 and |this| in frame #12 are the same, but we shouldn't be cloning a sheet while it's in the inconsistent state caused by frames 22/23/24.

It wouldn't surprise me if we had another bug on this...

#12 0x00007fa452f85ab6 in CSSStyleRuleImpl::Clone (this=0x250a620, 
    aClone=@0x7fffc589a748)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1501
#13 0x00007fa452f866bc in CloneRuleInto (aRule=0x10, aArray=0x3535a90)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:796
#14 0x00007fa465a6f5fd in nsVoidArray::EnumerateForwards (this=0x250b6f0, 
    aFunc=0x7fa452f866a0 <CloneRuleInto>, aData=0x3535a90)
    at nsVoidArray.cpp:724
#15 0x00007fa452f8b721 in nsCOMArray_base::EnumerateForwards (this=0x3535a20, 
    aCopy=..., aPrimarySheet=0x250e910) at ../../dist/include/nsCOMArray.h:65
#16 nsCOMArray<nsICSSRule>::EnumerateForwards (this=0x3535a20, aCopy=..., 
    aPrimarySheet=0x250e910) at ../../dist/include/nsCOMArray.h:232
#17 nsCSSStyleSheetInner (this=0x3535a20, aCopy=..., aPrimarySheet=0x250e910)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:868
#18 0x00007fa452f8b7d6 in nsCSSStyleSheetInner::CloneFor (this=0x250b680, 
    aPrimarySheet=0x250e910)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:886
#19 0x00007fa452f8c075 in nsCSSStyleSheet::EnsureUniqueInner (this=0x250e910)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:1385
#20 0x00007fa452f8c105 in nsCSSStyleSheet::WillDirty (this=0x10)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:1503
#21 0x00007fa452f8d163 in nsCSSStyleSheet::ReplaceStyleRule (this=0x250e910, 
    aOld=0x250a648, aNew=0x2adbd58)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleSheet.cpp:1319
#22 0x00007fa452f7f3d8 in CSSStyleRuleImpl::DeclarationChanged (
    this=0x250a620, aHandleContainer=1)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1545
#23 0x00007fa452f80efb in DOMCSSDeclarationImpl::DeclarationChanged (
    this=0x352eb78)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsCSSStyleRule.cpp:1137
#24 0x00007fa452faa92c in nsDOMCSSDeclaration::ParsePropertyValue (
    this=0x352eb78, aPropID=<value optimized out>, 
    aPropValue=<value optimized out>, aIsImportant=<value optimized out>)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/style/nsDOMCSSDeclaration.cpp:298

Comment 3

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

Really, it seems like the problem is that DOMI shouldn't have its hands on a declaration for a rule in a non-uniqued style sheet.

Comment 4

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

... which probably means inDOMUtils::GetRuleNodeForContent's call to presContext->EnsureSafeToHandOutCSSRules() isn't doing what it should.

Comment 5

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

... which I'm guessing is because the style sheet in question (chrome://global/skin/splitter.css) is a sheet reached via XBL <resources>.

Comment 6

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

I think this probably doesn't need to be security-sensitive, since it only affects debugging tools like DOM inspector that use the inDOMUtils CSS rules API (and also only affects debugging of documents that use XBL).

This particular case is also a null-dereference, although I suspect some other manifestations of this bug would not be.

Comment 7

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

Attachment: patch

This fixes it.

An automated test would be nice, but it seems like an awful lot of work...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 457433 [details] [diff] [review]
patch

r=bzbarsky.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/53a5c3bb6c5c

Comment 10

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

And this doesn't need to be security-sensitive; it's a missing piece of bug 536379.  I should have unmarked sooner.

Comment 11

[Tanner M. Young [:tmyoung]]

Verified Fixed on Mozilla/5.0 (Windows; Windows NT 6.0; WOW64; en-US; rv:2.0b2pre) Gecko/20100717 Minefield/4.0b2pre ID:20100717054110