233643 - XUL needs to clone inline-style rule before modifying it

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently the prototype and contentnode share stylerule. When that stylerule is
changed through .style we need to clone the rule before modifying it's
declaration. Otherwise we end up modifying the declaration used by the prototype.

Comment 1

[Boris Zbarsky [:bzbarsky]]

sicking, any chance you could just do this?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This might be another case where doing the RightThing might be too costly. I'm
not sure if we should clone the rule just because nsXULElement::GetStyle is
called, but anything later then that will be too late.

One solution is to let the stylerule clone the declaration if it notices that it
has more then one reference to it. Or some such...

Comment 3

[Brendan Eich [:brendan]]

Do we need to make a lighter-weight thing to clone?  Or is the style rule as
light as it can get for the needs of a fix for this bug?

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

Frankly, we should just clone in GetStyle.  If someone does that to a XUL
element, they're up to no good anyway and deserve the perf hit.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This fixes the testcase in this bug, but I'm not sure that I've done everything correctly. The MakeHeavyweight change is an unrelated correctness fix that Boris requested over IRC (though I'm not sure that this is what he meant).

Cloning the CSSStyleRuleValue on any given element's first GetStyle() seems rather expensive (unless there's some simpler way that I'm unaware of). People calling GetStyle on a XUL element may be "up to no good", but the fact is we seem to do it quite a bit in Firefox/Toolkit chrome (see e.g. http://lxr.mozilla.org/seamonkey/search?string=.style. ), though maybe none of those are in any critical paths.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That patch looks like about what I was thinking...

As for it being expensive... yes.  It is.  The problem is that after that point we can't easily detect attempts to set.

If that change actually causes a noticeable Txul/Ts/memory-use hit, we can worry about it then.  I rather doubt it will.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Looks good. I assume that you tested that this test fails on trunk, but passes with this test?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> (From update of attachment 260310 [details] [diff] [review])
> Looks good. I assume that you tested that this test fails on trunk, but passes
> with this test?

I did, but I'll test again before landing.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

This is the same patch, with the test updated to deal with the test issue found in bug 377295.

Comment 11

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

Comment on attachment 263175 [details] [diff] [review]
updated patch

> _TEST_FILES = 	test_bug330705-2.xul \
>+								test_bug233643.xul \
> 		$(NULL)

Something's wrong with the indentation here.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> Something's wrong with the indentation here.

Thanks, I noticed that too but forgot to fix it before uploading the patch. I've fixed this locally.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

For some reason the "r+sr=sicking" part of my commit message got cut off, but I've landed this on the trunk.

mozilla/content/xul/content/src/nsXULElement.cpp 	1.696
mozilla/content/xul/content/test/Makefile.in 	1.2
mozilla/content/xul/content/test/test_bug233643.xul 	1.1