Closed Bug 253354 Opened 20 years ago Closed 15 years ago

Crash setting/deleting style properties in Inspector


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



(Not tracked)



(Reporter: bzbarsky, Assigned: dbaron)



(Keywords: crash)

BUILD: Anything current

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.


In a debug build, you get:

WARNING: NS_ENSURE_TRUE(index != -1) failed, file  
line 2135
###!!! ASSERTION: container didn't take ownership: 'Not Reached', file
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]


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.
Severity: normal → critical
Keywords: crash
Product: Core → Other Applications
*** Bug 319625 has been marked as a duplicate of this bug. ***
*** Bug 319625 has been marked as a duplicate of this bug. ***
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
WFM with SM 1.1.2 on OS/2, no crash, no errors in the error console any more.
It also works for me with trunk, both on Linux and OS/2.
Closed: 17 years ago
Resolution: --- → WORKSFORME
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.
Resolution: WORKSFORME → ---
Keywords: regression
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. 
( = 'none';)

Presumably, these steps would crash with any child of anonymous content.
This is a semi-serious crash that has regressed behavior before.  I think that Firebug might also use this interface...
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9 M10
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...
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).
Keywords: regression
Not blocking per Comment #10.
Flags: blocking1.9? → blocking1.9-
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.
Depends on: 536379
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 reverted I see the assertion described in comment 0

On that basis, plus expectation that this would be fixed, marking as fixed.
Assignee: nobody → dbaron
Closed: 17 years ago15 years ago
OS: Linux → All
Priority: -- → P2
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta2 → mozilla1.9.3a1
(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.

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