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

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.

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?

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

Yes.

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?

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).

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 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 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 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 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.

(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?

> 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 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

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

Those look fine. Good catch.

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

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