218222 - Crash [@ CSSStyleRuleImpl::DeclarationChanged] toggling visibility value via DOM Inspector

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 1.5b mozilla.org talkback zip build, on Windows 98

STEPS TO REPRODUCE:
1)  Load http://uk.trendmicro-europe.com/PWT/pl/index.php
2)  Hit Ctrl-Shift-I to start DOM Inspector
3)  Click the binoculars at top left and search for the element with
    ID "cHoveruk" (a div containing an img)
4)  In the right panel, select the "CSS Style Rules" viewer
5)  Click on the third rule in the top right panel (the inline style rule)
6)  Right-click on the "visibility" row in the bottom right panel and
    select "edit"
7)  Replace the value with "visible" and hit "enter"

ACTUAL RESULTS: crash

EXPECTED RESULTS: no crash

TALKBACK IDS: TB23305815Q, TB23305574Y

A stack (based on those IDs or from a debug build) would be most helpful.

Comment 1

[Chase Tingley]

Attachment: stack trace

###!!! ASSERTION: rule must be in a sheet: 'mSheet', file nsCSSStyleRule.cpp,
line 1434
Break: at file nsCSSStyleRule.cpp, line 1434

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 951)]
0x412decb6 in CSSStyleRuleImpl::DeclarationChanged(int) (this=0x8845b68,
    aHandleContainer=1) at nsCSSStyleRule.cpp:1438
1438	      mSheet->ReplaceStyleRule(this, clone);
(gdb) p this->mSheet
$4 = (class nsICSSStyleSheet *) 0x0

Comment 2

[Chase Tingley]

This was on Linux; OS -> ALL

Comment 3

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

How does an inline style rule have a DOMCSSDeclarationImpl instead of an
nsDOMCSSAttrDeclaration?

Comment 4

[Stefan [:stefanh]]

Attachment: OS X crash log

I see this on Mac OS 10.1.5 with a one week old nightly (2003082903), setting
Hardware to All.

Comment 5

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

More crash logs don't help.  The crash is fully reproducable and we have a stack.

Comment 6

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

Well, the basic problem is that DOMCSSStyleRuleImpl is broken.  I'm amazed it
took so long to find this problem, and I'll need to refresh my memory about why
it works the way it does before I figure out the right way to fix this.

I'd be surprised if there aren't other related regressions, and I'd rather not
ship 1.5 with this.

Comment 7

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

mine

Comment 8

[timeless]

would this explain that other topcrash i filed for which bz didn't like my
wallpaper? bug 217674 see also bug 218031.

Comment 9

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

Actually, I think the comment about other regressions is wrong -- I don't think
there's any way to get to this style rule wrapper from the DOM -- it requires
C++ code, since nsDOMCSSAttributeDeclaration::GetParentRule returns null.

Comment 10

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

Attachment: patch?

I think we might want something like this, but I need to look at the other
callers of GetDOMRule.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This would also make the inline rules not show up in the "css rules" list in DOM
inspector, right?  That's fine in general, I guess...

Perhaps the Inspector code should be modified to always list a placeholder for
the inline style rule and to display the inline style information if that's
selected... (but that could be a separate bug on Inspector).

Comment 12

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

I also need to do something about nsHTMLEditor::ParseStyleAttrIntoCSSRule .

DOMCSSDeclarationImpl::GetParentRule is supposed to return null.

And if I take this approach, I should add a comment to nsICSSRule.h

Comment 13

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

Attachment: patch

I think this should do it.  I need to retest bug 91548, whose fix I am
removing, and hopefully refixing (with the change to nsGenericHTMLElement.cpp).

Comment 14

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

Attachment: patch

Slightly modified, and a little more defensive code to make sure things work in
edge cases.

I tested that bug 91548 is still fixed with this patch.  (I think it actually
may not have been with the previous one, although I may have been testing
wrong.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky, but please file a follow-up bug on Inspector to show the inline
style in that view (probably by just showing it there no matter what) and cc me
on it?

Comment 16

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

Attachment: inspector testcase

Comment 17

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

Attachment: patch to restore style attribute to style rule view in inspector

This puts the style attribute back in inspector (listed as style="", with no
file or line -- getting the file would be a bit of a pain, and the line's
always been wrong anyway, IIRC).

It's a little ugly, but it seems to work.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 131403 [details] [diff] [review]
patch to restore style attribute to style rule view in inspector

Seems pretty reasonable... Maybe set this.mStyleAttribute to null if it's not
instanceof DOMCSSDeclaration?  And I think something like:

this.mStyleAttribute = Components.lookupMethod(aObject, "style").call(aObject);

is a safer way to get the data we want (caillon, is that the right syntax?).

sr=bzbarsky with that nit.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 131403 [details] [diff] [review]
patch to restore style attribute to style rule view in inspector

>+StyleRuleView.prototype.getDecAt = 
>+function(aRow) 
>+{
>+  if (this.mRules) {
>+    if (this.mStyleAttribute && aRow + 1 == this.rowCount) {
>+      return this.mStyleAttribute;
>+    }
>+    var rule = this.mRules.GetElementAt(aRow);
>+    try {
>+      return XPCU.QI(rule, "nsIDOMCSSStyleRule").style;
>+    } catch (ex) {
>+      return null;
>+    }
>+  } else
>+    return this.mSheetRules[aRow].style;

Else-after-return.


And yes, bz's syntax is correct, although I'm not too worried about it because
there are many other places (outside of this patch) which need to be fixed too.

Comment 20

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

Could someone explain to me what the line in comment 18 does, and why it's better?

Comment 21

[Boris Zbarsky [:bzbarsky]]

David, it makes XPConnect look up the getter on the native object and use that
instead of just using the getter on the JSObject.

If a page does:

document.getElementById("foo").style = myObject;

in an XML document in which the object with ID "foo" is not an XHTML element,
the code you wrote (which gets .style on the node) will return myObject, whereas
the code I suggest will return null.

Comment 22

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

Comment on attachment 131403 [details] [diff] [review]
patch to restore style attribute to style rule view in inspector

Patch checked in to trunk, 2003-09-20 22:28 -0700.

Comment 23

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

Marking fixed.

Comment 24

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the Mozilla 1.5 branch. Please add
the fixed1.5 keyword when this is landed on the branch. Thanks.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 131403 [details] [diff] [review]
patch to restore style attribute to style rule view in inspector

a=asa (on behalf of drivers) for checkin to the Mozilla 1.5 branch. Please add
the fixed1.5 keyword when this is landed on the branch. Thanks.

Comment 26

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

Fix checked in to MOZILLA_1_5_BRANCH, 2003-09-22 23:44 -0700.

Comment 27

[[e-mail]]

I've just discovered that this fails for XUL elements, as .style isn't
implemented :-(

Comment 28

[Boris Zbarsky [:bzbarsky]]

This bug was about the crash.  It's fixed.  The inspector change got lumped in
here because it was somewhat straightforward to fix, but was not really this
bug....  The fact that you can't inspect the inline style of XUL elements via
the DOM is sort of hard to fix without giving them a DOM representation for said
inline style.  Which is already covered by a separate bug.

Comment 29

[[e-mail]]

OK, but are you aware that this has regressed DOM inspector to the point that it
is unable to display any styles for XUL elements with inline styles?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix that

No, I was not aware of that...

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132726 [details] [diff] [review]
Fix that

Checked in.

Comment 32

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

Comment on attachment 132726 [details] [diff] [review]
Fix that

Did this land on the 1.5 branch?

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 132726 [details] [diff] [review]
Fix that

1.5 shipped. removing obsolete request.