253354 - Crash setting/deleting style properties in Inspector

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Anything current

STEPS TO REPRODUCE:
1)  Open a browser window
2)  Inspect it (the chrome, not the document)
3)  Select the <window> node
4)  Click on the "Style Rules" view over on the right
5)  Select any rule (the last one will do).
6)  Right-click on any property in that rule's declaration and remove it.

You are now living on borrowed time; trigger some restyles to crash.

FURTHER INFORMATION:

In a debug build, you get:

WARNING: NS_ENSURE_TRUE(index != -1) failed, file  
/home/bzbarsky/mozilla/xlib/mozilla/content/html/style/src/nsCSSStyleSheet.cpp,
line 2135
###!!! ASSERTION: container didn't take ownership: 'Not Reached', file
/home/bzbarsky/mozilla/xlib/mozilla/content/html/style/src/nsCSSStyleRule.cpp,
line 1072
JavaScript error: , line 0: uncaught exception: [Exception... "Component
returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
[nsIDOMCSSStyleDeclaration.removeProperty]"  nsresult: "0x8000ffff
(NS_ERROR_UNEXPECTED)"  location: "JS frame ::
chrome://inspector/content/viewers/styleRules/styleRules.js :: anonymous :: line
265"  data: no]

ANALYSIS:

The assert is easy to explain.  The problem is that we share rule lists between
stylesheets.  So the rules pointed to by the rulenodes corresponding to the
<window> element are in a stylesheet inner that's shared by several sheets.  Now
since we sort of sneak up on these rules through the rulenodes, the sheet
doesn't know we have pointers to them, so the inner remains shared.  When we
actually change the declaration, we wind up in
CSSStyleSheetImpl::ReplaceStyleRule (called via the various DeclarationChanged()
methods).  Here we call WillDirty(), which finally clones the inner.  But now
the sheet has brand-new rules and aOld is a rule in the _old_ inner, not the new
one).  Hence the IndexOf() returns -1 and we bail out with NS_ERROR_UNEXPECTED.
 This returns us to CSSStyleRuleImpl::DeclarationChanged where we blithely
ignore the error return and return our newly-minted clone.  This returns us to
DOMCSSDeclarationImpl::DeclarationChanged, where we call Release() on mRule. 
But since no replacement ever took place, we were holding the only ref to mRule,
so we fall into the "set mRule to null and bail out" branch.

I'm not _quite_ sure about the crash itself.  We crash when enumerating the
rulehash, which suggests that we have a deleted rule hanging out in there or
something...  but I'm not sure the above process leads to that directly.

Comment 1

[Adam Guthrie]

*** Bug 319625 has been marked as a duplicate of this bug. ***

Comment 2

[Adam Guthrie]

*** Bug 319625 has been marked as a duplicate of this bug. ***

Comment 3

[Peter Weilbacher]

WFM with SM 1.1.2 on OS/2, no crash, no errors in the error console any more.

Comment 4

[Peter Weilbacher]

It also works for me with trunk, both on Linux and OS/2.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The problem is still there, sorry.  Might just take more work to get it to crash (though I doubt it), but misbehavior in other ways is still easy.

If you feel the need to resolve this after all, please make sure to refile a bug with the above analysis.

Comment 7

[Matthew Middleton (:zzxc)]

These steps reproduce this bug on: Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.9a9pre) Gecko/2007110212 Minefield/3.0a9pre

1. Go to a SSL site.  (bugzilla)
2. Open DOM inspector, inspect the browser chrome window.
3. Select the element with the ID "identify-icon-label".  It is the box with
the domain name of the secure site in the urlbar.  (This box will be set
display: none for Beta 1, so it won't be in builds for long. See bug 402260)
4. Go to CSS rules, and find the "#identify-icon-label" CSS rule.  Add the
rule: display = none
5. The entire rule disappears when you press "OK"
6. Go into the DOM node of the same object.
7. Add the node "style" with "display: none;"
8. Firefox crashes, Airbag/Breakpad doesn't catch it.

Note: Firefox does not crash if you use javascript to do step #7. 
(target.style.display = 'none';)

Presumably, these steps would crash with any child of anonymous content.

Comment 8

[Shawn Wilsher :sdwilsh]

This is a semi-serious crash that has regressed behavior before.  I think that Firebug might also use this interface...

Comment 9

[Boris Zbarsky [:bzbarsky]]

We should disallow editing of rules via this view if people really care about safety.  Either that, or rewrite the inspector CSS utils to ensure unique inners on everything in sight before walking the rule tree.  That _might_ help.

In any case, I'm not quite sure what the point of comment 7 was.  It's not like we didn't have steps to reproduce or didn't understand what the problem is...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and I have no idea what comment 8 is talking about in terms of "regressed behavior".  I certainly don't think this should be blocking Gecko 1.9, since the problem is not exactly in Gecko but in this UI (or in the Gecko functionality that allows this UI to exist, I suppose; I'm happy to remove such functionality of the people writing the UI don't want to fix it but do want the crash to magically go away).

Comment 11

[Damon Sicore (:damons)]

Not blocking per Comment #10.

Comment 12

[Boris Zbarsky [:bzbarsky]]

When this is fixed, we can undo the changes to layout/base/tests/test_bug416896.html I had to make to work around this bug.

Comment 15

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

I just tested that:
 * with the patch for bug 536379, I see neither assertion nor crash
 * with patch 4 for bug 536379 reverted, I crash reliably using the steps in comment 0
 * with that *plus* the CSSStyleRuleImpl::CSSStyleRuleImpl changes in http://hg.mozilla.org/mozilla-central/rev/e26f15a70aac reverted I see the assertion described in comment 0

On that basis, plus expectation that this would be fixed, marking as fixed.

Comment 16

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

(In reply to comment #12)
> When this is fixed, we can undo the changes to
> layout/base/tests/test_bug416896.html I had to make to work around this bug.

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