Static analysis caught this in bug 1294915 comment 43. Error: Dereference write __temp_1 Location: void mozilla::EffectSet::UpdateAnimationRuleRefreshTime(uint32, mozilla::TimeStamp*) @ ff-dbg/dist/include/mozilla/EffectSet.h:181 ### SafeArguments: arg1 Stack Trace: mozilla::ServoAnimationRule* mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385

Also: > Error: Field write nsRefreshDriver.mActiveTimer > Location: void nsRefreshDriver::EnsureTimerStarted(uint32) @ > layout/base/nsRefreshDriver.cpp:1302 ### SafeArguments: arg0 > Stack Trace: > mozilla::TimeStamp nsRefreshDriver::MostRecentRefresh() const @ > layout/base/nsRefreshDriver.cpp:1187 > mozilla::ServoAnimationRule* > mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, > uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 > Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385

OK, Looks like we should not call UpdateAnimationRuleRefreshTime in GetServoAnimationRule [1]. We should move it out of the parallel traversal. [1] http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/dom/animation/EffectCompositor.cpp#496-497

CCing Brian, since he might have some insight of this. I think the fist one is OK, but the second one is bad. We could add another variant of nsRefreshDriver::MostRecentRefresh() which does not call EnsureTimerStarted(), but I am not sure it will work on test mode as well.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > I think the fist one is OK, but the second one is bad. We could add another > variant of nsRefreshDriver::MostRecentRefresh() which does not call > EnsureTimerStarted(), but I am not sure it will work on test mode as well. Oh, on test mode, we just return at the first line of EnsureTimerStarted(). So MostRecentRefresh() without EnsureTimerStarted() works on test mode. I assume in normal refresh mode refresh driver has been waked up somewhere before we use it in GetAnimationRule(), but hmm I hope.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > CCing Brian, since he might have some insight of this. > > I think the fist one is OK, but the second one is bad. Even if one of these writes is benign, we should still find a way to fix it so that it doesn't trigger the static analysis. The static analysis is smart enough to know about NS_IsMainThread and ServoStyleSet::IsInServoTraversal, and only considers the branches where those things are false and true (respectively), in case that helps.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > > CCing Brian, since he might have some insight of this. > > > > I think the fist one is OK, but the second one is bad. > > Even if one of these writes is benign, we should still find a way to fix it > so that it doesn't trigger the static analysis. The static analysis is smart > enough to know about NS_IsMainThread and ServoStyleSet::IsInServoTraversal, > and only considers the branches where those things are false and true > (respectively), in case that helps. Yeah, agree. We should do something against the first case too. Here I don't think it's a good idea to kick UpdateAnimationRuleRefreshTime() off outside styling, it will be more complicated to me.

What's the trouble that you're seeing? Is it to do with accessing the EffectSet or the RefreshDriver? For accessing the EffectSet, are we still going to switch to using DOM slots? For the refresh driver, we should be able to just fetch its time before we begin traversal and pass that as context, right?

(In reply to Brian Birtles (:birtles) from comment #7) > What's the trouble that you're seeing? Is it to do with accessing the > EffectSet or the RefreshDriver? > > For accessing the EffectSet, are we still going to switch to using DOM slots? > > For the refresh driver, we should be able to just fetch its time before we > begin traversal and pass that as context, right? We still use DOM slots for EffectSet. However, we shouldn't revise anything in EffectSet (only read-only), I think. I didn't notice that while implementing GetServoAnimationRule().

(In reply to Boris Chiou [:boris] from comment #8) > We still use DOM slots for EffectSet. However, we shouldn't revise anything > in EffectSet (only read-only), I think. I didn't notice that while > implementing GetServoAnimationRule(). Have we made that change already? On mozilla-central it looks like we are still storing them using node properties which I believe is a hashmap attached to the document? If we switch to element-specific storage then I think it's ok to mutate during traversal? Or at least I thought that was the plan we discussed in Hawaii?

(In reply to Brian Birtles (:birtles) from comment #9) > (In reply to Boris Chiou [:boris] from comment #8) > > We still use DOM slots for EffectSet. However, we shouldn't revise anything > > in EffectSet (only read-only), I think. I didn't notice that while > > implementing GetServoAnimationRule(). > > Have we made that change already? On mozilla-central it looks like we are > still storing them using node properties which I believe is a hashmap > attached to the document? No, not yet. I should file the follow-up bug for it. Thanks. > > If we switch to element-specific storage then I think it's ok to mutate > during traversal? Or at least I thought that was the plan we discussed in > Hawaii? OK, I see.

For the record, Cameron told us about SequentialTask. We can use it for updating mAnimationRuleRefreshTime.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11) > For the record, Cameron told us about SequentialTask. We can use it for > updating mAnimationRuleRefreshTime. Our irc log: 16:10:38 H<heycam> so if you have some work that can be saved for later on the main thread, you do it by adding a new enum value to SequentialTask (in context.rs) 16:11:05 H<heycam> and then doing: context.thread_local.tasks.push(your_new_task) I will try to do that. Thanks for heycam's suggestion.

Please ignore the previous comment, was reading through and decided to just cc me, but forgot to delete the half-written comment. This should be ok as long as the storage is element-specific and not shared with other elements.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14) > Please ignore the previous comment, was reading through and decided to just > cc me, but forgot to delete the half-written comment. This should be ok as > long as the storage is element-specific and not shared with other elements. OK. I am planning to try to use element-specific storage first and run the static analysis (not sure how to run it locally now). If it still has this error, I will try heycam's idea to dispatch the task to main thread.

Can someone refresh my memory on what kind of writes we need to make to EffectSet during the traversal, and how frequently we need to do it? It is true that we generally have exclusive access to the element when computing its style. But the type system doesn't guarantee it. From the Servo side, we protect ElementData with AtomicRefCell, which makes all our writes to it safe. But if we mutate a C++ data structure, we don't have a good way of guaranteeing that some other Gecko_* FFI call on another worker thread won't somehow trigger a read of the EffectSet we're mutating. That's why the static analysis is designed to prevent all mutation over FFI. The risk here is low, and we could find a way to whitelist. But it weakens our guarantees, and the guarantees are one of the reasons we're using Rust here in the first place. So I'd like to understand the problem space a bit more to weigh the tradeoffs. What percentage of Elements during a traversal will need EffectSet mutation? Just the ones with animations and transitions? And how significant is the mutation? Would SequentialTask do the job without significant overhead?

Another question, which might make it hard to use SequentialTask here: do we make use of the parent EffectSet when styling the children? (In reply to Boris Chiou [:boris] from comment #15) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #14) > OK. I am planning to try to use element-specific storage first and run the > static analysis (not sure how to run it locally now). If it still has this > error, I will try heycam's idea to dispatch the task to main thread. To be clear, SequentialTask doesn't dispatch tasks to the main thread. It just collects them in a vector and executes them when the traversal completes. There's no virtual call, so it's quite fast.

(NI-ing Boris, but Brian or Hiro could probably respond too).

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16) > Can someone refresh my memory on what kind of writes we need to make to > EffectSet during the traversal, and how frequently we need to do it? It's EffectSet::mAnimationRuleRefreshTime, it is used to unthrottle transform animations running on the compositor every 200ms, this is because transform animations causes layout overflow. I think this mAnimationRuleRefreshTime is basically harmless because it's per element. But yes, we need to make sure its safety access somehow. There is another problem in this bug, it's writing nsRefreshDriver members. nsRefreshDriver is per document. This writing access is caused by EnsureTimerStarted() which is called from MostRecentRefresh(), and the value of MostRecentRefresh() is set to mAnimationRUleRefreshTime. Also MostRecentRefresh() is called from various places in our animation code, it's actually called during calculating animation style other than the place comment #1. What Brian suggested in comment 7 is that we get MostRecentRefresh() in ServoStyleSet::PrepareAndTraverseSubtree (for example) and pass the value to the context that is used for traversal, and we use the value during animation process instead of calling MostRecentRefresh(). (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16) > What percentage of Elements during a traversal > will need EffectSet mutation? Just the ones with animations and transitions? What I don't quite understand is that there is another writing access to an EffectSet member, mAnimationRule, it's accessed every time we calculate animating styles, but the analysis tool does not catch it. I don't understand what the difference is. But I think we should care this member as well (or white-listed?). Oops, to avoid confusion, we access mAnimationRuleRefreshTime every time we calculate animation styles as well.

This stops mutating nsRefreshDriver::mActiveTimer and mMostRecentRefresh during parallel traverse. Initially I did check mPresContext->RestyleManager()->IsInStyleRefresh() in nsRefreshDriver::MostRecentRefresh() but it caused another problem since mPresContext is WeakPtr<> (not thread-safe!). So I had to add mInStyleRefresh into nsRefreshDriver which represents RestylManager::mInStyleRefresh. I have no idea other than adding this. I have no idea how we should handle EffectSet::mAnimationRuleRefreshTime (and mAnimationRule) yet.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16) > Can someone refresh my memory on what kind of writes we need to make to > EffectSet during the traversal, and how frequently we need to do it? Currently we store up to 9 different things on node properties: - EffectSet{0..3} - Animations affecting the element, its ::before or ::after pseudo elements - CSSTransitionCollection{0..3} - Running / finished transitions on element, its ::before or ::after pseudo elements - CSSAnimationCollection{0..3} - Defined CSS animations on element, its ::before or ::after pseudo elements We typically only touch the CSSTransitionCollection/CSSAnimationCollection when either a new transition / CSS animation starts, or when one of the transition-* or animation-* properties changes. For EffectSet, however, I believe the Stylo implementation currently updates the refresh time stored on the EffectSet on each tick where the EffectSet, i.e. only elements with animations (including transitions). We used to use this to detect cases where we can skip updating the animation style, but I believe it is now only used for transform animations that run on the compositor where we use it to update the animation style on the main thread every 200ms so that scrollbars update. I'm pretty sure we could find another mechanism for doing this that doesn't require writing to the EffectSet, however. In the Gecko implementation, we skip updating animation style unnecessarily by tracking which EffectSets need updating in a document-level hashmap and update that during restyling. That's a pretty important optimization which had a positive impact on talos numbers when we introduced it (Tart tests from memory) so I think we may want to do that for Stylo too. It's a document-level data structure but we only need to read it during during the parallel traversal (i.e. does this effect set need updating?). Perhaps we could even just snapshot that data before we begin traversal. At the end of traversal, we could clear the hashmap as a sequential task I expect (it wouldn't be an unconditional clear, but would be based on the type of restyle). Perhaps more importantly, however, is that even if we only touch the animation rule refresh time on EffectSets themselves, we do write to the objects inside EffectSets during traversal (e.g. the mProgressOnLastCompose and mCurrentIterationOnLastCompose members of KeyframeEffectReadOnly, as well as mPreviousPhase / mPreviousIteration in CSSAnimation and similar in CSSTransition). That seems like it might be harder to do as a sequential task, or at least would require accumulating a bit more data and breaking open the encapsulation of those objects a bit. But it's still doable. I'm not sure I have a good sense for the trade-off here.

(In reply to Brian Birtles (:birtles) from comment #21) > In the Gecko implementation, we skip updating animation style unnecessarily > by tracking which EffectSets need updating in a document-level hashmap and > update that during restyling. That's a pretty important optimization which > had a positive impact on talos numbers when we introduced it (Tart tests > from memory) so I think we may want to do that for Stylo too. It's a > document-level data structure but we only need to read it during during the > parallel traversal (i.e. does this effect set need updating?). We need to update EffectSet if we starts CSS animations/transitions during parallel traversal. Currently we do start CSS animations after parallel traversal though, but I am planning to start CSS animations during parallel traversal (bug 1335938). > Perhaps more importantly, however, is that even if we only touch the > animation rule refresh time on EffectSets themselves, we do write to the > objects inside EffectSets during traversal (e.g. the mProgressOnLastCompose > and mCurrentIterationOnLastCompose members of KeyframeEffectReadOnly, as > well as mPreviousPhase / mPreviousIteration in CSSAnimation and similar in > CSSTransition). That seems like it might be harder to do as a sequential > task, or at least would require accumulating a bit more data and breaking > open the encapsulation of those objects a bit. But it's still doable. I'm > not sure I have a good sense for the trade-off here. I think those of members are safe to be used. I am not sure what the analysis tool says about them.

Comment on attachment 8838835 [details] [diff] [review] Stop mutating nsRefreshDriver I should also note about nsRefreshDriver::mMostRecentRefresh. Apart from the issue of EffectSet, we have to ensure that we don't mutate mMostRecentRefresh, because we do call nsRefreshDriver::MoseRecentRefresh() a bunch of times through Animation::GetCurrentTime() during the parallel traversal.

Comment on attachment 8838835 [details] [diff] [review] Stop mutating nsRefreshDriver There is no UI at all..

Comment on attachment 8838835 [details] [diff] [review] Stop mutating nsRefreshDriver Is this because we access MostRecentRefresh in many places? i.e. rather than just get the refresh time once at the start and pass it as context (or set it on EffectCompositor?) it's better to just make sure MostRecentRefresh is always safe to call?

(In reply to Brian Birtles (:birtles) from comment #25) > Comment on attachment 8838835 [details] [diff] [review] > Stop mutating nsRefreshDriver > > Is this because we access MostRecentRefresh in many places? i.e. rather than > just get the refresh time once at the start and pass it as context (or set > it on EffectCompositor?) it's better to just make sure MostRecentRefresh is > always safe to call? It's one reason, but not a big one. If we call MostRecentRefresh() at the start, we don't need to pass it to somewhere at all, since we bail out from EnsureTimerStarted() soon (there is a check of mActiveTimer). That means 'if (!mActiveTimer)' is not so important. The only one important thing is that we call MostRecentRefresh() before the parallel traversal. It maybe be better not add mInStyleRefresh in nsRefreshDriver to avoid confusion.

EnsureTimerStarted() won't set mActiveTimer in various cases, such as when mTestControllingRefreshes is true, so the assertion is not exactly right, I think. But instead of adding mInStyleRefresh, how about we check ServoStyleSet::IsInServoTraversal(): TimeStamp nsRefreshDriver::MostRecentRefresh() const { if (!ServoStyleSet::IsInServoTraversal()) { const_cast<nsRefreshDriver*>(this)->EnsureTimerStarted(); } return mMostRecentRefresh; } For an assertion that we've correctly called EnsureTimerStarted, maybe we can have a DEBUG only bool that we set at the top of EnsureTimerStarted, and which we clear at the end of ServoRestyleManager::ProcessPendingRestyles?

(In reply to Cameron McCormack (:heycam) from comment #27) > EnsureTimerStarted() won't set mActiveTimer in various cases, such as when > mTestControllingRefreshes is true, so the assertion is not exactly right, I > think. > > But instead of adding mInStyleRefresh, how about we check > ServoStyleSet::IsInServoTraversal(): > > TimeStamp > nsRefreshDriver::MostRecentRefresh() const > { > if (!ServoStyleSet::IsInServoTraversal()) { > const_cast<nsRefreshDriver*>(this)->EnsureTimerStarted(); > } > > return mMostRecentRefresh; > } Wow! Fantastic! I will do it. > For an assertion that we've correctly called EnsureTimerStarted, maybe we > can have a DEBUG only bool that we set at the top of EnsureTimerStarted, and > which we clear at the end of ServoRestyleManager::ProcessPendingRestyles? Thanks! I will do this too. Do you think we should add a setter function for the DebugOnly flag? I am going to make the flag in public for now.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28) > Thanks! I will do this too. Do you think we should add a setter function > for the DebugOnly flag? I am going to make the flag in public for now. I've changed my mind. Actually I did but the appearance was bad.

Comment on attachment 8839031 [details] Bug 1340340 - Stop mutating nsRefreshDriver during parallel traversal. https://reviewboard.mozilla.org/r/113788/#review115336 ::: layout/base/nsRefreshDriver.h:459 (Diff revision 1) > // next tick by the refresh driver. This flag will be reset at the > // start of every tick. > bool mResizeSuppressed; > > + // True only if we are processing pending restyles for stylo. > + mozilla::DebugOnly<bool> mInServoRestyles; DebugOnly is MOZ_STACK_CLASS, so you'll probably fail static analysis builds here. Just wrap it with #ifdef DEBUG. But... ::: layout/base/nsRefreshDriver.cpp:1329 (Diff revision 1) > + MOZ_ASSERT(!mInServoRestyles, > + "EnsureTimerStarted should be called only when we are not " > + "processing pending restyles for stylo"); ...I don't think this assertion achieves anything more than MOZ_ASSERT(!ServoStyleSet::IsInServoTraversal()), in which case we don't need mInServoRestyles.

Comment on attachment 8839031 [details] Bug 1340340 - Stop mutating nsRefreshDriver during parallel traversal. https://reviewboard.mozilla.org/r/113788/#review115338 So, r=me with mInServoRestyles removed and the assertion changed.

Thank you Cameron! I filed a new bug for the case of EffectSet. Bug 1340958.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/916e7ba3cd2b Stop mutating nsRefreshDriver during parallel traversal. r=heycam

(In reply to Brian Birtles (:birtles) from comment #21) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16) > > Can someone refresh my memory on what kind of writes we need to make to > > EffectSet during the traversal, and how frequently we need to do it? > > Currently we store up to 9 different things on node properties: > > - EffectSet{0..3} - Animations affecting the element, its ::before or > ::after pseudo elements > - CSSTransitionCollection{0..3} - Running / finished transitions on element, > its ::before or ::after pseudo elements > - CSSAnimationCollection{0..3} - Defined CSS animations on element, its > ::before or ::after pseudo elements > > We typically only touch the CSSTransitionCollection/CSSAnimationCollection > when either a new transition / CSS animation starts, or when one of the > transition-* or animation-* properties changes. > > For EffectSet, however, I believe the Stylo implementation currently updates > the refresh time stored on the EffectSet on each tick where the EffectSet, > i.e. only elements with animations (including transitions). We used to use > this to detect cases where we can skip updating the animation style, but I > believe it is now only used for transform animations that run on the > compositor where we use it to update the animation style on the main thread > every 200ms so that scrollbars update. I'm pretty sure we could find another > mechanism for doing this that doesn't require writing to the EffectSet, > however. > > In the Gecko implementation, we skip updating animation style unnecessarily > by tracking which EffectSets need updating in a document-level hashmap and > update that during restyling. That's a pretty important optimization which > had a positive impact on talos numbers when we introduced it (Tart tests > from memory) so I think we may want to do that for Stylo too. It's a > document-level data structure but we only need to read it during during the > parallel traversal (i.e. does this effect set need updating?). Perhaps we > could even just snapshot that data before we begin traversal. There's no need to snapshot it - hashmap reads should be totally fine to do during the traversal as long as they don't have side-effects. > At the end of > traversal, we could clear the hashmap as a sequential task I expect (it > wouldn't be an unconditional clear, but would be based on the type of > restyle). The type of restyle in what sense? Right now there's only a single traversal path in Servo, and I'd like to keep it that way. Is this something that would make sense to do from the Gecko side in ProcessPendingRestyles? > Perhaps more importantly, however, is that even if we only touch the > animation rule refresh time on EffectSets themselves, we do write to the > objects inside EffectSets during traversal (e.g. the mProgressOnLastCompose > and mCurrentIterationOnLastCompose members of KeyframeEffectReadOnly, as > well as mPreviousPhase / mPreviousIteration in CSSAnimation and similar in > CSSTransition). That seems like it might be harder to do as a sequential > task, or at least would require accumulating a bit more data and breaking > open the encapsulation of those objects a bit. I'm not totally sure I see the encapsulation problem. We can still define the storage struct and execution routine anywhere in style crate, and then just import it into context.rs and make it a SequentialTask variant. > But it's still doable. I'm > not sure I have a good sense for the trade-off here. AFAICT, the two reasons to avoid using a SequentialTask would be: (a) The operation is common or expensive enough that appending it to a vec during traversal could negatively impact performance (alternatively, that the work is expensive enough that performance would suffer by not parallelizing it). (b) Subsequent operations during the traversal (i.e. styling children) depend on this state having been set, i.e. it would be incorrect to delay it to the end of the servo traversal. Are either of these the case? If not (and assuming there isn't another case I haven't thought of), I'd think we should should use a SequentialTask for this to guard ourselves against subtle races.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #37) > (In reply to Brian Birtles (:birtles) from comment #21) > > At the end of > > traversal, we could clear the hashmap as a sequential task I expect (it > > wouldn't be an unconditional clear, but would be based on the type of > > restyle). > > The type of restyle in what sense? Right now there's only a single traversal > path in Servo, and I'd like to keep it that way. Is this something that > would make sense to do from the Gecko side in ProcessPendingRestyles? In Gecko we have the concept of animation-only styles. This comes about because of compositor/throttled animations and transitions. For animations running on the compositor or otherwise throttled, we don't post restyles for their changing style on most ticks (there are some exceptions, but generally we don't). As a result, the style for these animations on the main thread can lag behind the style for other animations. When we go to do a restyle that we know contains non-animation changes, and therefore could cause a transition to be generated, we *first* update all animations *including* throttled animations. Then we go ahead and process other restyles. This is so that we can produce the correct before-change and after-change style for generating transitions. You can see this setup in GeckoRestyleManager::ProcessPendingRestyles. Now, in the hashmap I mentioned, we store elements that need a restyle for animation along with a bool indicating if a restyle has been posted or not. For animations that are throttled we put them in the hashmap but don't post a restyle (i.e. the bool is false in that case). Then, when it comes time to flush throttled animations, we go through and post restyles for all those throttled animations so we know to update them (and update their bool to true at the same time). (You might wonder why we don't just drop animations from the hashmap as soon as we post a restyle, but keeping them in the hashmap until the restyle happens helps us avoid posting redundant restyles and is a significant optimization.) So, all I was trying to say is that we don't want to just nuke the hashmap after a restyle with stylo. We still only want to clear those elements in the hashmap whose 'posted restyle?' bool is true. > > Perhaps more importantly, however, is that even if we only touch the > > animation rule refresh time on EffectSets themselves, we do write to the > > objects inside EffectSets during traversal (e.g. the mProgressOnLastCompose > > and mCurrentIterationOnLastCompose members of KeyframeEffectReadOnly, as > > well as mPreviousPhase / mPreviousIteration in CSSAnimation and similar in > > CSSTransition). That seems like it might be harder to do as a sequential > > task, or at least would require accumulating a bit more data and breaking > > open the encapsulation of those objects a bit. > > I'm not totally sure I see the encapsulation problem. We can still define > the storage struct and execution routine anywhere in style crate, and then > just import it into context.rs and make it a SequentialTask variant. Sorry, I'm not talking about encapsulation in terms of crates, but in terms of C++ classes. Members like mProgressOnLastCompose ideally shouldn't be set from outside, they should be set as part of composing style (as the name suggests). > > But it's still doable. I'm > > not sure I have a good sense for the trade-off here. > > AFAICT, the two reasons to avoid using a SequentialTask would be: > (a) The operation is common or expensive enough that appending it to a vec > during traversal could negatively impact performance (alternatively, that > the work is expensive enough that performance would suffer by not > parallelizing it). I don't think that's the case. Our extreme cases are probably when people do confetti / bubble type animations where they have a 1000 DOM nodes moving around and then we'd need to stick all 1000 animations in the vec along with all the associated data that needs to be updated. None of those operations are particularly expensive, however. > (b) Subsequent operations during the traversal (i.e. styling children) > depend on this state having been set, i.e. it would be incorrect to delay it > to the end of the servo traversal. This is something I'm still digging into. For a new CSS animation, we do need to restyle the children with the new CSS animation applied but I'm still trying to work out if this would be suitable to do as a separate subsequent restyle. For CSS transitions we have almost the opposite case, where the after-change style is specifically *not* supposed to contain the style from transitions on ancestors that are stopping or starting right now. The trouble is I'm still trying to work out what the correct behavior here is. Gecko follows the spec but other browsers don't and the spec'ed behavior appears to be buggy in some cases. I suspect we'll need to change the spec but I'm not sure how much yet. (Best case scenario is that we end up changing it in a way that makes what stylo needs to do simpler.) > Are either of these the case? If not (and assuming there isn't another case > I haven't thought of), I'd think we should should use a SequentialTask for > this to guard ourselves against subtle races. So far I don't think so but I can't say for certain yet. I need a few more days to work this out. I know Hiro had trouble using SequentialTask but I'll let him ask you about that.