Closed Bug 1340938 Opened 7 years ago Closed 7 years ago

stylo: Threading issues when creating CSSAnimation/CSSTransition during parallel traversal

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox54 --- affected

People

(Reporter: hiro, Unassigned)

References

Details

When we start css animations/transitions during parallel traversal, we need to create CSSAnimation/CSSTransition instance. But there are things that are not thread safe.  I am listing them so far as I know.

1. AnimationCollection
  Migrate to use DOMSlots instead of node property (Bug 1340445 or a new one).

2. PendingAnimationTracker
  Can we use SequentialTask for this?

3. EffectCompositor::mElementsToRestyle
  Should we skip modifying mElementsToRestyle during parallel traversal?
  When we call RequestRestyle() during parallel traversal, we throw the request away?
  As far as I know, creating CSSAnimation/CSSTransition instance will request restyle.

4. nsAutoAnimationMutationBatch (for devtools)
  Use SequentialTask?
  Need a global nsAutoAnimationMutationBatch in ServoRestyleManager::ProcessPendingRestyles()?
  We should implement equivarent stuff of nsAutoAnimationMutationBatch in rust?

5. Animation::sNextAnimationIndex
  We increment this in CancelFromStyle().
  I guess we can change this value as a value per an element, but am not sure.

6. AsyncEventDispatcher from CancelFromStyle().
  Do we need to dispatch this event to the main-thread?

7. CycleCollectedJSContext in Animation::DoFinishNotification?
  Is this thread safe?
  I think we have to dispatch the runnable task(micro task) to the main thread.

8. mPresContext->PresShell()->SetNeedStyleFlush() (only for CSS animations)
  https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/layout/style/nsAnimationManager.cpp#l477
  I don't understand about this yet.

9. CSSAnimation might mutate other animations on a different element. (only for CSS animations)
  There is a case that we mutate other animations on a different element
  when we cancel CSS animation (e.g. style.animation = "";) if the target element
  of the CSS animation has been changed by Web Animation API.
  Please see bug 1336928 comment 13 how this happens.

9 is the biggest issue here, I think we should defer to call CancelFromStyle() after the traversal somehow.

I haven't yet checked transition case closely.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> 8. mPresContext->PresShell()->SetNeedStyleFlush() (only for CSS animations)
>  
> https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/layout/style/
> nsAnimationManager.cpp#l477
>   I don't understand about this yet.

OK. Now I understand this. This needs for dispatching animation events in a subsequent tick (SetNeedStyleFlush is somewhat misleading, I think refresh driver should dispatch queued events regardless of style flush, but it's another story). I don't know why nsTransitionManager does not do the same thing at all..

I think this should be also deferred to do on the main-thread.
10. mEventDispatcher.
  I missed that we are changing mEventDispatcher when we call CancelFromStyle().
Brian told me briefly about the last discussion with Cameron.

We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or Device?), and we insert element when we updated css animations on the element. After the parallel traversal, we enumerate this hash map and do a bunch of process. This should solve 2,5,6,8,9,10.  For this way AnimationCollection should have an array for canceled animations.
Blocks: 1339704
Blocks: 1341372
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Brian told me briefly about the last discussion with Cameron.
> 
> We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> Device?), and we insert element when we updated css animations on the
> element. After the parallel traversal, we enumerate this hash map and do a
> bunch of process. This should solve 2,5,6,8,9,10.  For this way
> AnimationCollection should have an array for canceled animations.

Why do we need a hashmap? Can't we just use SequentialTasks?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Brian told me briefly about the last discussion with Cameron.
> > 
> > We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> > Device?), and we insert element when we updated css animations on the
> > element. After the parallel traversal, we enumerate this hash map and do a
> > bunch of process. This should solve 2,5,6,8,9,10.  For this way
> > AnimationCollection should have an array for canceled animations.
> 
> Why do we need a hashmap? Can't we just use SequentialTasks?

Yes, I think we can use SequentialTasks other than 9 if we can separate 9 case from CancelFromStyle() cleanly.  I need to figure out it.

One thing is not clear to me is that SequentialTasks still runs on multiple threads? In my understandings, it's true, then we need to figure out 2 case differently, it's a per document object.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > > Brian told me briefly about the last discussion with Cameron.
> > > 
> > > We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> > > Device?), and we insert element when we updated css animations on the
> > > element. After the parallel traversal, we enumerate this hash map and do a
> > > bunch of process. This should solve 2,5,6,8,9,10.  For this way
> > > AnimationCollection should have an array for canceled animations.
> > 
> > Why do we need a hashmap? Can't we just use SequentialTasks?
> 
> Yes, I think we can use SequentialTasks other than 9 if we can separate 9
> case from CancelFromStyle() cleanly.  I need to figure out it.

I don't fully understand case 9 (and can't see the linked bug). Is the issue that, while traversing one element during styling, we may need to mutate the EffectSet of another element? If so, which other element? An ancestor, a sibling, a descendant, or something else?

> 
> One thing is not clear to me is that SequentialTasks still runs on multiple
> threads? In my understandings, it's true, then we need to figure out 2 case
> differently, it's a per document object.

SequentialTasks always run on the main thread once the parallel traversal completes - that's the whole point. :-)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > > > Brian told me briefly about the last discussion with Cameron.
> > > > 
> > > > We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> > > > Device?), and we insert element when we updated css animations on the
> > > > element. After the parallel traversal, we enumerate this hash map and do a
> > > > bunch of process. This should solve 2,5,6,8,9,10.  For this way
> > > > AnimationCollection should have an array for canceled animations.
> > > 
> > > Why do we need a hashmap? Can't we just use SequentialTasks?
> > 
> > Yes, I think we can use SequentialTasks other than 9 if we can separate 9
> > case from CancelFromStyle() cleanly.  I need to figure out it.
> 
> I don't fully understand case 9 (and can't see the linked bug). Is the issue
> that, while traversing one element during styling, we may need to mutate the
> EffectSet of another element? If so, which other element? An ancestor, a
> sibling, a descendant, or something else?

Unfortunately, whatever element. Also it mutates Animation object and related things. For example:

element.style.animation = "anim 10s";                  // (1) create a CSS animation
anotherElement.animate({color: 'red', 'blue'}, 10000); // (2) create a script animation on anotherElement
..
let animation = element.getAnimations()[0]; // get the animation of (1)
animation.effect.target = anotherElement;   // change the target element of (1), but its *owning* element is still 'element'
..
element.style.animation = "";  // When we process styles of 'element', we mutate the animation of (2), that means we need to calculate styles of the animation of (2).

So, we should this process after the traversal anyway.  In my understanding we should defer this to a next tick somehow.

> > One thing is not clear to me is that SequentialTasks still runs on multiple
> > threads? In my understandings, it's true, then we need to figure out 2 case
> > differently, it's a per document object.
> 
> SequentialTasks always run on the main thread once the parallel traversal
> completes - that's the whole point. :-)

Wow! Thanks! I really misunderstood it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > > > > Brian told me briefly about the last discussion with Cameron.
> > > > > 
> > > > > We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> > > > > Device?), and we insert element when we updated css animations on the
> > > > > element. After the parallel traversal, we enumerate this hash map and do a
> > > > > bunch of process. This should solve 2,5,6,8,9,10.  For this way
> > > > > AnimationCollection should have an array for canceled animations.
> > > > 
> > > > Why do we need a hashmap? Can't we just use SequentialTasks?
> > > 
> > > Yes, I think we can use SequentialTasks other than 9 if we can separate 9
> > > case from CancelFromStyle() cleanly.  I need to figure out it.
> > 
> > I don't fully understand case 9 (and can't see the linked bug). Is the issue
> > that, while traversing one element during styling, we may need to mutate the
> > EffectSet of another element? If so, which other element? An ancestor, a
> > sibling, a descendant, or something else?
> 
> Unfortunately, whatever element. Also it mutates Animation object and
> related things. For example:
> 
> element.style.animation = "anim 10s";                  // (1) create a CSS
> animation
> anotherElement.animate({color: 'red', 'blue'}, 10000); // (2) create a
> script animation on anotherElement
> ..
> let animation = element.getAnimations()[0]; // get the animation of (1)
> animation.effect.target = anotherElement;   // change the target element of
> (1), but its *owning* element is still 'element'
> ..
> element.style.animation = "";  // When we process styles of 'element', we
> mutate the animation of (2), that means we need to calculate styles of the
> animation of (2).

Is there a reason we need to process the retargeted animation while processing the owner, rather than while processing the target? I know very little about animation stuff, but I figured it was worth asking.
 
> So, we should this process after the traversal anyway.  In my understanding
> we should defer this to a next tick somehow.

Yeah, if we can defer it then it's not a problem for Stylo.

> 
> > > One thing is not clear to me is that SequentialTasks still runs on multiple
> > > threads? In my understandings, it's true, then we need to figure out 2 case
> > > differently, it's a per document object.
> > 
> > SequentialTasks always run on the main thread once the parallel traversal
> > completes - that's the whole point. :-)
> 
> Wow! Thanks! I really misunderstood it.
Depends on: 1341518
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> > > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > > > > > Brian told me briefly about the last discussion with Cameron.
> > > > > > 
> > > > > > We should have a hashmap of (pseudo-)Element somewhere in servo (Stylist or
> > > > > > Device?), and we insert element when we updated css animations on the
> > > > > > element. After the parallel traversal, we enumerate this hash map and do a
> > > > > > bunch of process. This should solve 2,5,6,8,9,10.  For this way
> > > > > > AnimationCollection should have an array for canceled animations.
> > > > > 
> > > > > Why do we need a hashmap? Can't we just use SequentialTasks?
> > > > 
> > > > Yes, I think we can use SequentialTasks other than 9 if we can separate 9
> > > > case from CancelFromStyle() cleanly.  I need to figure out it.
> > > 
> > > I don't fully understand case 9 (and can't see the linked bug). Is the issue
> > > that, while traversing one element during styling, we may need to mutate the
> > > EffectSet of another element? If so, which other element? An ancestor, a
> > > sibling, a descendant, or something else?
> > 
> > Unfortunately, whatever element. Also it mutates Animation object and
> > related things. For example:
> > 
> > element.style.animation = "anim 10s";                  // (1) create a CSS
> > animation
> > anotherElement.animate({color: 'red', 'blue'}, 10000); // (2) create a
> > script animation on anotherElement
> > ..
> > let animation = element.getAnimations()[0]; // get the animation of (1)
> > animation.effect.target = anotherElement;   // change the target element of
> > (1), but its *owning* element is still 'element'
> > ..
> > element.style.animation = "";  // When we process styles of 'element', we
> > mutate the animation of (2), that means we need to calculate styles of the
> > animation of (2).
> 
> Is there a reason we need to process the retargeted animation while
> processing the owner, rather than while processing the target? I know very
> little about animation stuff, but I figured it was worth asking.

Sorry for confusion.  The calculation styles of retargeted animation is processed on the retargeted element. What I wanted to say is that we might mutate the retargeted animation while it's processing and even if it's not happened simultaneously at this moment, as a result of the mutation we need to re-calculate animation styles on the retargeted animation.  But anyway we should defer whole of this process in later.
I am now convinced 8 in comment 0 is not useful.  we can drop it.  Filed bug 1341518.
On the other hand, I found another one.

11. AnimationTimeline/DocumentTimeline
  This has animation array and each animation is appened/removed when it's created/destroyed.
  Also this timeline touches nsRefreshDriver to be called this timeline from nsRefreshDriver.

In my current understandings, we need to pass a SequentialTasks whenever we create/destroy any CSS animations.

And for 4. nsAutoAnimationMutationBatch we need the task when we modify animation properties too? Or can we re-implement it in rust?

Cameron, what do think about nsAutoAnimationMutationBatch?
Flags: needinfo?(cam)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> 11. AnimationTimeline/DocumentTimeline
>   This has animation array and each animation is appened/removed when it's
> created/destroyed.

That sounds OK to use SequentialTask, as long as we don't need to look at the list of animations before the SequentialTask runs.

>   Also this timeline touches nsRefreshDriver to be called this timeline from
> nsRefreshDriver.

This is the stuff in DocumentTimeline::NotifyAnimationUpdated, to call AddRefreshObserver?  This is safe to do from the SequentialTask too, yes?

> And for 4. nsAutoAnimationMutationBatch we need the task when we modify
> animation properties too? Or can we re-implement it in rust?
> 
> Cameron, what do think about nsAutoAnimationMutationBatch?

Where are the nsAutoAnimationMutationBatch uses that we need to worry about during restyling: is it in KeyframeEffect::SetTarget, and maybe others?
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) (away 25 Feb–5 Mar) from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > 11. AnimationTimeline/DocumentTimeline
> >   This has animation array and each animation is appened/removed when it's
> > created/destroyed.
> 
> That sounds OK to use SequentialTask, as long as we don't need to look at
> the list of animations before the SequentialTask runs.

Yeah, I think there is no need to look at the list.  Brian knows better than me about DocumentTimeline.
He also knows well about PendingAnimationTracker.

> >   Also this timeline touches nsRefreshDriver to be called this timeline from
> > nsRefreshDriver.
> 
> This is the stuff in DocumentTimeline::NotifyAnimationUpdated, to call
> AddRefreshObserver?  This is safe to do from the SequentialTask too, yes?

Yes, right.  I think we can do this as the SequentialTask.

> > And for 4. nsAutoAnimationMutationBatch we need the task when we modify
> > animation properties too? Or can we re-implement it in rust?
> > 
> > Cameron, what do think about nsAutoAnimationMutationBatch?
> 
> Where are the nsAutoAnimationMutationBatch uses that we need to worry about
> during restyling: is it in KeyframeEffect::SetTarget, and maybe others?

Yes, maybe.  One is KeyframeEffectReadOnly::SetKeyframes() and one is in UpdateOldAnimationPropertiesWithNew().
And some of nsNodeUtils::AnimationChanged() might be used (i.e. cases which are not caught by above cases. I am not really sure.)
Yes, SequentialTask should be suitable for the changes to DocumentTimeline and PendingAnimationTracker, I expect (although, boy, we're going to be do a lot of odd jobs in these sequential tasks. I'm starting to get worried about all the half-complete states we'll be generating. We'll need to do this very carefully.)

Animation mutations we may observe during restyling include the new/cancelled notifications from nsAnimationManager::UpdateAnimations and likewise for transitions. Apart from that there's also SetSpecifiedTiming/Play/Pause as Hiro mentioned, in UpdateOldAnimationPropertiesWithNew. (It looks like we don't dispatch notifications when the keyframes change? An existing bug because we previously didn't represent keyframes in the inspector?)
OK.  So I think we need to make sure that there is an nsAutoAnimationMutationBatch on the stack during the whole restyle process, so I guess near the top of ServoRestyleManager::ProcessPendingRestyles.  Then, we need to make sure that nsAutoAnimationMutationBatch::Init does nothing (and asserts that sCurrentBatch is non-null) when ServoStyleSet::InServoTraversal().  Then I think we need to make nsNodeUtils::AnimationMutated save up those IMPL_ANIMATION_NOTIFICATION calls to be run on the main thread once the traversal is finished.  Maybe we can add a Servo_XXX FFI function that will post a SequentialTask for this.

I wonder though whether the Animation object that we pass to nsNodeUtils::AnimationMutated is guaranteed to live long enough?  (We can't store a strong reference to the Animation in a SequentialTask, since Animation inherits from DOMEventTargetHelper, which only has main thread safe refcounting.)

Actually, during restyling, is it possible for an Animation object to have AddRef or Release called on it?  If so, that is a problem too.
(In reply to Cameron McCormack (:heycam) (away 25 Feb–5 Mar) from comment #14)
> OK.  So I think we need to make sure that there is an
> nsAutoAnimationMutationBatch on the stack during the whole restyle process,
> so I guess near the top of ServoRestyleManager::ProcessPendingRestyles. 
> Then, we need to make sure that nsAutoAnimationMutationBatch::Init does
> nothing (and asserts that sCurrentBatch is non-null) when
> ServoStyleSet::InServoTraversal().  Then I think we need to make
> nsNodeUtils::AnimationMutated save up those IMPL_ANIMATION_NOTIFICATION
> calls to be run on the main thread once the traversal is finished.  Maybe we
> can add a Servo_XXX FFI function that will post a SequentialTask for this.
> 
> I wonder though whether the Animation object that we pass to
> nsNodeUtils::AnimationMutated is guaranteed to live long enough?  (We can't
> store a strong reference to the Animation in a SequentialTask, since
> Animation inherits from DOMEventTargetHelper, which only has main thread
> safe refcounting.)
> 
> Actually, during restyling, is it possible for an Animation object to have
> AddRef or Release called on it?  If so, that is a problem too.

Urgh, CSS animation stuff AddRefes the Animation object.. actually it creates Animation object on the traversal thread.
Blocks: 1334036
No longer blocks: 1334036
(In reply to Brian Birtles (:birtles) from comment #13)
> Yes, SequentialTask should be suitable for the changes to DocumentTimeline
> and PendingAnimationTracker, I expect (although, boy, we're going to be do a
> lot of odd jobs in these sequential tasks. I'm starting to get worried about
> all the half-complete states we'll be generating. We'll need to do this very
> carefully.)

Note that the other alternative that would be acceptable (though less desirable) from a safety perspective would be to put the animation state we want to mutate into a C++-equivalent of AtomicRefCell [1]. So this would mean paying the cost of atomic operations whenever we want to access this state on the main thread, in exchange for the ability to mutate it from the worker thread (and crash safely if we ever screw up).

This only works if there are no pointers into the fields of whatever state we decide to protect this way. C++ can't enforce this the way Rust can, which is why I think this solution is less-desirable.


[1] https://docs.rs/atomic_refcell/0.1.0/atomic_refcell/
Summary: Threading issues when creating CSSAnimation/CSSTransition during parallel traversal → stylo: Threading issues when creating CSSAnimation/CSSTransition during parallel traversal
I am going to resolve this bug as invalid once bug 1341985 landed.
Now bug 1341985 landed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.