522595 - transitions determine transition start from incorrect style data because of rule immutability violations

Status: RESOLVED FIXED

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

Description

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

Right now CSS transitions determine the data at transition start by calling GetStyleData().  By the time this happens CSSStyleRule objects have violated rule immutability and are now pointing to the declaration's new data block (since they access the data block through the declaration, which changes).

To fix this, we probably need to:
 * make compressed data blocks reference counted
 * make the style rule have a pointer to its compressed data block
 * clone the data block in the fast element.style codepath

This is filed as a followup based on bug 435441 comment 87, where bz wrote:
>>+nsTransitionManager::ConsiderStartingTransition(nsCSSProperty aProperty,
>>+  // FIXME: This call on the old style context gets incorrect style data
>>+  // since we don't quite enforce style rule immutability:  we didn't
>>+  // need to worry about callers calling GetStyleData rather than
>>+  // PeekStyleData after a style rule becomes "old" before transitions
>>+  // existed.
>
>Why do we need GetStyleData here?  Because we might need to transition even
>things no one has asked about (like color in a visibility:hidden subtree)?
>
>This part really worries me, though I think it's ok...  I don't _think_ we can
>ever run into crashes this way.  I hope.

and I responded in bug 435441 comment 89:
>> Why do we need GetStyleData here?  Because we might need to transition even
>> things no one has asked about (like color in a visibility:hidden subtree)?
>
>Yes.  And there are other important cases for this, e.g., two style
>changes with a flush in between, where the first causes frame
>reconstruction.
>
>> This part really worries me, though I think it's ok...  I don't _think_ we can
>> ever run into crashes this way.  I hope.
>
>Yeah.  It's the thing I dislike the most about this patch.  I think it
>would have been relatively fixable before your recent property-changing
>optimizations to poke directly into compressed data blocks, but now I'm
>really not sure.

This also covers the fixme comments in:
http://hg.mozilla.org/mozilla-central/file/4046f3843bdb/layout/reftests/css-transitions/transitions-inline-already-wrapped-1.html
http://hg.mozilla.org/mozilla-central/file/4046f3843bdb/layout/reftests/css-transitions/transitions-inline-already-wrapped-2.html
http://hg.mozilla.org/mozilla-central/file/4046f3843bdb/layout/reftests/css-transitions/transitions-inline-rewrap-1.html
http://hg.mozilla.org/mozilla-central/file/4046f3843bdb/layout/reftests/css-transitions/transitions-inline-rewrap-2.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

How does making compressed data blocks refcounted help?  Note that we always blow the compressed data block away right now when expanding, so even in the non-fast codepath we need to clone it instead.  Or something.

Comment 2

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

OK, I thought this out a little more, and I think the idea would be:
 * make compressed data blocks reference counted (and assert that the reference count is <= 2)
 * Expand is something that reduces the caller's reference count by 1:
   + if Expand is called when the reference count is 1, it does what it does now (transferring structs)
   + if Expand is called when the reference count is >1, it clones the structs and thus doesn't destroy the block

Thus the style rule would own a reference to the data blocks, but only starting when its MapRuleInfoInto is first called, so we wouldn't reclone for multiple mututations.

The fast element.style codepath could thus look at the reference count and only clone when needed.

Thoughts?

Comment 3

[Boris Zbarsky [:bzbarsky]]

So one ref would be from the CSSDeclaration; another (optional) one would be from the style rule?

Comment 4

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

Yes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Seems like a reasonable approach.  With this, sounds like we could also stop keeping refs to the declaration in the old rule (and so maybe make CSSDeclarations non-refcounted), right?

Comment 6

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

Attachment: patch 1: call BeginUpdate before updating style attributes

I'm including this in the first patch so that I can do the first half of comment 5 in the last patch (set to null, but not yet stop refcounting).

It's a required prerequisite for passing tests since layout/base/tests/test_scrolling.html reliably triggers the same problem as in the stack in bug 209575 comment 4 (except the crash ends up happening at GetImportantRule() now since MapRuleInfoInto can deal with having a null mDeclaration).

A big question to think about when reviewing is whether this is sufficient.  Can we rely on calling BeginUpdate first meaning that stuff that does rule matching won't happen in the inner BeginUpdate call?

Or instead (or in addition) should we make GetImportantRule deal with mDeclaration being null (perhaps only conditionally on having been called before).

Comment 7

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

Attachment: patch 2: reference count compressed data blocks and make them immutable when refcnt>1

Comment 8

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

Attachment: patch 3: remove unnecessary calls to SlotForValue, which will need to assert about mutability

Comment 9

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

Attachment: patch 4: ensure declarations are mutable before calling SlotForValue

Comment 10

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

Attachment: patch 5: make style rules honor immutability contract by holding reference to data block, and remove test workarounds to test this bug

Comment 11

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

Note that the intermediate state with patch 2 applied and patch 4 not applied would trigger assertions *if* it were possible to get an immutable block.  However, immutable blocks don't actually start happening until patch 5 rolls around, so each patch should leave things in a good state.  I might have been better off doing things in a different order, but I don't think it's worth changing at this point.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412903 [details] [diff] [review]
patch 1: call BeginUpdate before updating style attributes

This is fine, but makes me really sad.  Can you please file a bug to remove these changes, depending on the "enable html5 parser" bug?  Though I guess we'll still have the problem for XML for a bit...

Document that we need a GetOwnerDoc(), not GetCurrentDoc() because we in fact need to start the update on the document we _might_ get inserted into by the update start?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412904 [details] [diff] [review]
patch 2: reference count compressed data blocks and make them immutable when refcnt>1

Looks ok; worth logging the refcounting?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412905 [details] [diff] [review]
patch 3: remove unnecessary calls to SlotForValue, which will need to assert about mutability

HasNonImportantValueFor(nsCSSProperty aProperty) const {
>+    NS_ABORT_IF_FALSE(nsCSSProps::IsShorthand(aProperty), "must be longhand");

Missing ! before nsCSSProps?

With that fixed, r=bzbarsky

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412909 [details] [diff] [review]
patch 5: make style rules honor immutability contract by holding reference to data block, and remove test workarounds to test this bug

>+  // NOTE that transferring ownership of the declaration relies on our
>+  // making *some* MapRuleInfoInto calls immediately when we construct
>+  // the rule tree.  This means that any style rule that has become part
>+  // of the rule tree has already retrieved the necessary data blocks
>+  // from its declaration

Hmm.  It's not clear to me why this would have to be the case.  If we construct a new ruletree branch for a style context, but then the more specific rules fully specify the structs the style context asks for, then the less specific rules won't have MapRuleInfoInto called on them.

Comment 16

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

Attachment: patch 5: Add nsICSSStyleRule::RuleMatched and call it when rules are matched

Comment 17

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

Attachment: patch 6: cache the important rule in RuleMatched to make nulling out mDeclaration safe

Comment 18

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

Attachment: patch 7: make style rules honor immutability contract by holding reference to data block, and remove test workarounds to test this bug

Comment 19

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

(In reply to comment #15)
> Hmm.  It's not clear to me why this would have to be the case.  If we construct
> a new ruletree branch for a style context, but then the more specific rules
> fully specify the structs the style context asks for, then the less specific
> rules won't have MapRuleInfoInto called on them.

Good point.  The revised patches should address this.

(In reply to comment #12)
> This is fine, but makes me really sad.  Can you please file a bug to remove
> these changes, depending on the "enable html5 parser" bug?  Though I guess
> we'll still have the problem for XML for a bit...

How is the HTML5 parser going to guarantee that this isn't needed anymore?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> How is the HTML5 parser going to guarantee that this isn't needed anymore?

It'll make it so that BeginUpdate no longer triggers notifications (which it does with the batching setup in our current html parser).  And it does that by not sticking nodes in the DOM and leaving them there without notifying: any time it goes to put nodes in the DOM it notifies on them immediately.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 413151 [details] [diff] [review]
patch 7: make style rules honor immutability contract by holding reference to data block, and remove test workarounds to test this bug

> +  // mDeclaration unused in CSSStyleRuleImpl::GetImportantRule,

"is unused"

r=bzbarsky

Comment 22

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

Turns out I missed the call from nsSVGElement.cpp that should have been in patch 5.  I made the obvious change and also added an assertion in CSSStyleRule::MapRuleInfoInto in patch 7 that would tell any new callers who make the same mistake what it was they missed.  I don't feel the need to request re-review for those changes, but revised patches are at:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/107b080b65c6/call-rulematched
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/107b080b65c6/style-rule-immutability

Comment 23

[Boris Zbarsky [:bzbarsky]]

Those look fine.  Good catch.

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/7c7bd415a3d3
http://hg.mozilla.org/mozilla-central/rev/f7ccfb798c36
http://hg.mozilla.org/mozilla-central/rev/9a34d0aa3784
http://hg.mozilla.org/mozilla-central/rev/fc75fba7ca7f
http://hg.mozilla.org/mozilla-central/rev/76cee6fdfade (with 2 more callers!)
http://hg.mozilla.org/mozilla-central/rev/97c71fd5d6a0
http://hg.mozilla.org/mozilla-central/rev/e26f15a70aac

Comment 25

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

I landed a rather serious build warning fix (and DEBUG-only crash):
http://hg.mozilla.org/mozilla-central/rev/48175bb06cb1