1371450 - stylo: panic in geckoservo::glue::Servo_ResolveStyle call from APZ

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

Visiting https://demo.bitmovin.com/public/firefox/av1/

(nightly build with --enable-stylo --enable-av1)

Opened devtools.
Selected network panel
Clicked play (may not work with a nightly build for another couple of days.)
After about 10 seconds of video segments downloading, I scrolled and the tab crashed. gdb says:

#0  0x00007f9d9debf81d in nanosleep () at /lib64/libc.so.6
#1  0x00007f9d9debf76a in sleep () at /lib64/libc.so.6
#2  0x00007f9d906be117 in ah_crap_handler(int) (signum=signum@entry=11) at /home/giles/firefox/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007f9d906be14d in child_ah_crap_handler(int) (signum=11) at /home/giles/firefox/toolkit/xre/nsSigHandlers.cpp:115
#4  0x00007f9d90ee7b76 in js::UnixExceptionHandler(int, siginfo_t*, void*) (signum=11, info=0x7ffdef530bf0, context=0x7ffdef530ac0)
    at /home/giles/firefox/js/src/ds/MemoryProtectionExceptionHandler.cpp:267
#5  0x00007f9d912a679d in WasmFaultHandler<(Signal)0>(int, siginfo_t*, void*) (signum=11, info=0x7ffdef530bf0, context=0x7ffdef530ac0)
    at /home/giles/firefox/js/src/wasm/WasmSignalHandlers.cpp:1313
#6  0x00007f9d9ec765c0 in <signal handler called> () at /lib64/libpthread.so.0
#7  0x00000000004061fe in mozalloc_abort(char const*) (msg=msg@entry=0x42b600 "Redirecting call to abort() to mozalloc_abort\n")
    at /home/giles/firefox/memory/mozalloc/mozalloc_abort.cpp:33
#8  0x00000000004061c1 in abort() () at /home/giles/firefox/memory/mozalloc/mozalloc_abort.cpp:80
#9  0x00007f9d91bf4b89 in panic_abort::__rust_start_panic::abort () at /checkout/src/libpanic_abort/lib.rs:61
#10 0x00007f9d91bf4b89 in panic_abort::__rust_start_panic () at /checkout/src/libpanic_abort/lib.rs:56
#11 0x00007f9d91bf05cd in std::panicking::rust_panic () at /checkout/src/libstd/panicking.rs:580
#12 0x00007f9d91bf057f in std::panicking::rust_panic_with_hook () at /checkout/src/libstd/panicking.rs:565
#13 0x00007f9d9150ca5b in std::panicking::begin_panic<&str> (msg=..., file_line=0x0) at /checkout/src/libstd/panicking.rs:511
#14 0x00007f9d91542ccc in geckoservo::glue::Servo_ResolveStyle (element=<optimized out>, 
    element@entry=0x7f9d49ce8550, raw_data=<optimized out>, allow_stale=false) at /home/giles/firefox/servo/ports/geckolib/glue.rs:2425
#15 0x00007f9d8ef4ebb8 in mozilla::ServoStyleSet::ResolveServoStyle(mozilla::dom::Element*) (this=this@entry=0x7f9d549beac0, aElement=aElement@entry=
    0x7f9d49ce8550) at /home/giles/firefox/layout/style/ServoStyleSet.cpp:1116
#16 0x00007f9d8f0be295 in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=this@entry=0x7f9d549beb60, aElement=0x7f9d49ce8550, aParentContext=aParentContext@entry=0x0, aStyleSet=aStyleSet@entry=0x7f9d549beac0, aChangeList=...) at /home/giles/firefox/layout/base/ServoRestyleManager.cpp:347
#17 0x00007f9d8f0cfda8 in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::TraversalRestyleBehavior) (this=this@entry=0x7f9d549beb60,
aRestyleBehavior=aRestyleBehavior@entry=mozilla::TraversalRestyleBehavior::ForAnimationOnly) at /home/giles/firefox/layout/base/ServoRestyleManager.cpp:593
#18 0x00007f9d8f0d0374 in mozilla::ServoRestyleManager::UpdateOnlyAnimationStyles() (this=0x7f9d549beb60)
     at /home/giles/firefox/layout/base/ServoRestyleManager.cpp:667

Comment 1

[Bobby Holley (:bholley)]

Stability, P1.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

CCind Boris, since the stack includes UpdateOnlyAnimationStyles().

Comment 3

[Boris Chiou [:boris]]

While playing some videos in youtube, I also got a panic, like this:

thread '<unnamed>' panicked at 'Resolving style on element without current styles with lazy computation forbidden.', /Users/boris/projects/firefox/gecko/servo/ports/geckolib/glue.rs:2660
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic
   5: Servo_ResolveStyle
   6: _ZN7mozilla13ServoStyleSet17ResolveServoStyleEPNS_3dom7ElementE
   7: _ZN7mozilla19ServoRestyleManager20ProcessPostTraversalEPNS_3dom7ElementEP14nsStyleContextRNS_17ServoRestyleStateE
   8: _ZN7mozilla19ServoRestyleManager20ProcessPostTraversalEPNS_3dom7ElementEP14nsStyleContextRNS_17ServoRestyleStateE
   9: _ZN7mozilla19ServoRestyleManager20ProcessPostTraversalEPNS_3dom7ElementEP14nsStyleContextRNS_17ServoRestyleStateE
  10: _ZN7mozilla19ServoRestyleManager24DoProcessPendingRestylesENS_24TraversalRestyleBehaviorE
  11: _ZN7mozilla9PresShell11HandleEventEP8nsIFramePNS_14WidgetGUIEventEbP13nsEventStatusPP10nsIContent
  12: _ZN13nsViewManager13DispatchEventEPN7mozilla14WidgetGUIEventEP6nsViewP13nsEventStatus
  13: _ZN6nsView11HandleEventEPN7mozilla14WidgetGUIEventEb
  14: _ZN7mozilla6widget12PuppetWidget13DispatchEventEPNS_14WidgetGUIEventER13nsEventStatus
  15: _ZN7mozilla6layers18APZCCallbackHelper19DispatchWidgetEventERNS_14WidgetGUIEventE
  16: _ZN7mozilla3dom8TabChild24RecvRealMouseButtonEventERKNS_16WidgetMouseEventERKNS_6layers19ScrollableLayerGuidERKy
  17: _ZThn96_N7mozilla3dom8TabChild23RecvSynthMouseMoveEventERKNS_16WidgetMouseEventERKNS_6layers19ScrollableLayerGuidERKy
  18: _ZN7mozilla3dom13PBrowserChild17OnMessageReceivedERKN3IPC7MessageE
  19: _ZN7mozilla3dom13PContentChild17OnMessageReceivedERKN3IPC7MessageE
  20: _ZN7mozilla3ipc14MessageChannel20DispatchAsyncMessageERKN3IPC7MessageE
  21: _ZN7mozilla3ipc14MessageChannel15DispatchMessageEON3IPC7MessageE
  22: _ZN7mozilla3ipc14MessageChannel10RunMessageERNS1_11MessageTaskE
  23: _ZN7mozilla3ipc14MessageChannel11MessageTask3RunEv
  24: _ZN7mozilla14SchedulerGroup8Runnable3RunEv
  25: _ZN8nsThread16ProcessNextEventEbPb
  26: _Z23NS_ProcessPendingEventsP9nsIThreadj
  27: _ZN14nsBaseAppShell19NativeEventCallbackEv
  28: _ZN10nsAppShell18ProcessGeckoEventsEPv
  29: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
  30: __CFRunLoopDoSources0
  31: __CFRunLoopRun
  32: CFRunLoopRunSpecific
  33: RunCurrentEventLoopInMode
  34: ReceiveNextEventCommon
  35: _BlockUntilNextEventMatchingListInModeWithFilter
  36: _DPSNextEvent
  37: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
  38: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
  39: -[NSApplication run]
  40: _ZN10nsAppShell3RunEv
  41: _Z15XRE_RunAppShellv
  42: _ZN7mozilla3ipc26MessagePumpForChildProcess3RunEPN4base11MessagePump8DelegateE
  43: _ZN11MessageLoop3RunEv
  44: _Z20XRE_InitChildProcessiPPcPK12XREChildData
  45: main
Redirecting call to abort() to mozalloc_abort

This also happened in Servo_ResolveStyle(), but is not related to UpdateOnlyAnimationStyles(), I think. Will keep trying to reproduce the call stack in comment 0. Hope we have an easier way to reproduce it.

Comment 4

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #3)
> This also happened in Servo_ResolveStyle(), but is not related to
> UpdateOnlyAnimationStyles().

I was wrong. This is the same call stack from UpdateOnlyAnimationStyles.

Comment 5

[Boris Chiou [:boris]]

This assertion says: "Assert that we have computed style in Servo_ResolveStyle when lazy computation is forbidden." Therefore, I think the easiest way to fix this is to avoid resolving style from APZ (i.e. UpdateAnimationOnlyStyles()) in this case because it seems we shouldn't do it.

Comment 6

[Boris Chiou [:boris]]

I think the root-cause is:

We don't handle/clear snapshots during animation-only restyle (without normal traversal) in UpdateOnlyAnimationStyles, and allow_stale is false in Servo_ResolveStyle() and element.has_current_styles(&*data) always checks the status of snapshots, so valid_styles is false if we have snapshots during animation-only restyle from APZ.

Comment 7

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #6)
> I think the root-cause is:
> 
> We don't handle/clear snapshots during animation-only restyle (without
> normal traversal) in UpdateOnlyAnimationStyles, and allow_stale is false in
> Servo_ResolveStyle() and element.has_current_styles(&*data) always checks
> the status of snapshots, so valid_styles is false if we have snapshots
> during animation-only restyle from APZ.

Not only the above root cause, but also we might post-traverse <script> element which is not restyled, from APZ animation-only restyle (this happens randomly).

Comment 8

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #7)
> (In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #6)
> > I think the root-cause is:
> > 
> > We don't handle/clear snapshots during animation-only restyle (without
> > normal traversal) in UpdateOnlyAnimationStyles, and allow_stale is false in
> > Servo_ResolveStyle() and element.has_current_styles(&*data) always checks
> > the status of snapshots, so valid_styles is false if we have snapshots
> > during animation-only restyle from APZ.
> 
> Not only the above root cause, but also we might post-traverse <script>
> element which is not restyled, from APZ animation-only restyle (this happens
> randomly).

Start to suspect youtube may dynamically add a script element, like "<script class="js-httpswwwyoutubecomytsjsbinwwwen_USvfloaxwl7ypc_bootstrapjs">", after we match & cascade the tree, but before post-traversal. It seems we didn't see this element during traverse_node.

Comment 10

[Bobby Holley (:bholley)]

Hiro thinks that this is different than bug 1379425 and is going to investigate it. He and Emilio might want to chat about it to make sure they're on the same page.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Oh, I misunderstood that the Emilio's refactor is bug 1379505.  But anyway, I will look this. 

Yes, bug 1379425 has a good test case to cause this panic, a solution for this panic is bug 1379203 comment 6, I believe.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7535b1519ef2bb6d2876dec1132f8c427c2b22d0

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2deb83ce288532ce5f2b1534ffe6623b6ad9a9b2
I haven't finished writing reftests but I think I am on the right track, so I will send review requests for now.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

ForAnimationOnly is somewhat misleading, it means actually we process
*only* animation-only restyle without normal restyle. The purpose of
ForAnimationOnly is for updating throttled animations to get correct position
of the animations when we need to handle events. Currently we do also update
unthrottled animations though.

Review commit: https://reviewboard.mozilla.org/r/157124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157124/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

Review commit: https://reviewboard.mozilla.org/r/157126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157126/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

When we process throttled animations restyle for event handling, we skip normal
traversal at all, so after Servo_TraverseSubtree, normally there remains
unstyled elements which have to be processed in normal traversal later. These
elements should be skipped in ProcessPostTraversal too since it's not yet styled!

Review commit: https://reviewboard.mozilla.org/r/157128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157128/

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

Those remaining restyle hints are needed for normal traversal later.

Review commit: https://reviewboard.mozilla.org/r/157130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157130/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

Review commit: https://reviewboard.mozilla.org/r/157132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157132/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1371450 - Crash test.

The crash test causes a panic in Servo_ResolveStyle on debug build without this
patch series.  The reason why we send mouse move event twice is that one for
E10S and the other one is for non-E10S. It seems to be caused by being
requestIdleCallback processed on different timing.

Review commit: https://reviewboard.mozilla.org/r/157134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157134/

Comment 20

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2deb83ce288532ce5f2b1534ffe6623b6ad9a9b2

This build fixed bug 1380543 and bug 1379425 for me (only tested those so far), where today's 20170713100259 is buggy.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Darkspirit from comment #20)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2deb83ce288532ce5f2b1534ffe6623b6ad9a9b2
> 
> This build fixed bug 1380543 and bug 1379425 for me (only tested those so
> far), where today's 20170713100259 is buggy.

That's great to know, especially about bug 1380543!

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2deb83ce288532ce5f2b1534ffe6623b6ad9a9b2
> I haven't finished writing reftests but I think I am on the right track, so
> I will send review requests for now.

It seems that we can't write reftest for this. Currently we don't support SpecialPowers in reftest (bug 655622), so I tried to use dispatchEvent, but it did not hit the panic at all. I guess it leads different code paths.  Also as for mochitest it seems to me that we have no way to get incorrect styles, I tried getComputedStyle() in the crash test in attachment 8886383 [details] on a release build without those patches, unfortunately it returns correct style values.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

You can use tools inside WindowSnapshot.js to do reftest in mochitest.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Wow! thank you Xidorn!

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: A mochitest does not work

Unfortunately WindowSnapshot.js did not help either.

With the attaching mochitest on a release build, I did confirm that element.has_current_styles() check in Servo_ResolveStyle() fails. Also I did confirm that the mochitest sometimes draws both of targets as 'red' (acutually either of two should be rendered black) if SimpleTest.finish() is dropped. But even so, assertSnapshots() still succeeds.  I am suspecting the failure case when SimpleTest.finish() is dropped is caused by keypress events which is happened by Ctrl+r to reload the mochitest.  no luck..

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8886378 [details]
Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

https://reviewboard.mozilla.org/r/157124/#review162414

Sorry, my comments are a bit back-to-front (I reviewed from the bottom up since the TraversalRestyleBehavior enum value seemed to be the most important name to get right).

::: layout/base/ServoRestyleManager.cpp:769
(Diff revision 1)
>    // of the restyling process.
>    AnimationsWithDestroyedFrame animationsWithDestroyedFrame(this);
>  
>    ServoStyleSet* styleSet = StyleSet();
>    nsIDocument* doc = PresContext()->Document();
> -  bool animationOnly = aRestyleBehavior ==
> +  bool forThrottledAnimations =

See comments below but this might be better as |forThrottledAnimationFlush|.

::: layout/style/ServoStyleSet.cpp:363
(Diff revision 1)
> -  bool forAnimationOnly =
> -    aRestyleBehavior == TraversalRestyleBehavior::ForAnimationOnly;
> +  bool forThrottledAnimations =
> +    aRestyleBehavior == TraversalRestyleBehavior::ForFlushThrottledAnimations;

As below, forThrottledAnimationFlush.

::: layout/style/ServoStyleSet.cpp:391
(Diff revision 1)
> -  // Don't need to trigger a second traversal if this restyle only needs
> -  // animation-only restyle.
> -  if (forAnimationOnly) {
> +  // Don't need to trigger a second traversal if this restyle only need to flush
> +  // throttled animation restyle. That's because it does not involve normal
> +  // traversal so that there is no SequentialTask for updating animation's state.

We don't need to trigger a second traversal if this restyle is only for flushing throttled animations. That's because the first traversal only performs the animation-only restyle, skipping the normal restyle, and so will not generate any SequentialTask that could update animation state requiring a subsequent traversal.

::: layout/style/ServoStyleSet.cpp:928
(Diff revision 1)
>    }
>    return postTraversalRequired;
>  }
>  
>  bool
> -ServoStyleSet::StyleDocumentForAnimationOnly()
> +ServoStyleSet::StyleDocumentForFlushThrottledAnimations()

See below but this should be either:

  StyleDocumentForFlushingThrottledAnimations

or:

  StyleDocumentForThrottledAnimationFlush

::: layout/style/ServoTypes.h:74
(Diff revision 1)
> -  // Processes animation-only restyle.
> -  ForAnimationOnly,
> +  // Processes only `animation-only` restyle that means normal restyle is
> +  // skipped.
> +  // This is used for getting throttled animations up-to-date when we need to
> +  // get correct position for throttled animations (e.g. transform animations
> +  // on the compositor).
> +  ForFlushThrottledAnimations,

Processes just the traversal for animation-only restyles and skips the normal traversal for other restyles unrelated to animation.
This is used to bring throttled animations up-to-date such as when we need to get the correct position of transform animations that are throttled because they are running on the compositor.

::: layout/style/ServoTypes.h:79
(Diff revision 1)
> -  // Processes animation-only restyle.
> -  ForAnimationOnly,
> +  // Processes only `animation-only` restyle that means normal restyle is
> +  // skipped.
> +  // This is used for getting throttled animations up-to-date when we need to
> +  // get correct position for throttled animations (e.g. transform animations
> +  // on the compositor).
> +  ForFlushThrottledAnimations,

Can we call this ForThrottledAnimationFlush?

(Or, if you prefer, ForFlushingThrottledAnimations)

Comment 29

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #28)

> ::: layout/style/ServoTypes.h:79
> (Diff revision 1)
> > -  // Processes animation-only restyle.
> > -  ForAnimationOnly,
> > +  // Processes only `animation-only` restyle that means normal restyle is
> > +  // skipped.
> > +  // This is used for getting throttled animations up-to-date when we need to
> > +  // get correct position for throttled animations (e.g. transform animations
> > +  // on the compositor).
> > +  ForFlushThrottledAnimations,
> 
> Can we call this ForThrottledAnimationFlush?
> 
> (Or, if you prefer, ForFlushingThrottledAnimations)

I will take ForThrottledAnimationFlush.  Thank you!

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886378 [details]
Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

https://reviewboard.mozilla.org/r/157124/#review162432

r=me with Brian's comments :)

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886379 [details]
Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

https://reviewboard.mozilla.org/r/157126/#review162434

::: servo/ports/geckolib/glue.rs:286
(Diff revision 1)
>                           traversal_flags | ANIMATION_ONLY,
>                           unsafe { &*snapshots });
>      }
>  
>      if restyle_behavior == Restyle::ForFlushThrottledAnimations {
> -        return needs_animation_only_restyle;
> +        return element.has_animation_only_dirty_descendants();

What if the animation is on the root? We presumably need to check that too, don't we?

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

https://reviewboard.mozilla.org/r/157128/#review162436

So I want a stronger indication that this patch is correct... As far as I can tell, this will restyle both elements restyled by the animation restyle, _and_ elements that weren't, which looks fishy...

::: commit-message-1a7ba:1
(Diff revision 1)
> +Bug 1371450 - Don't traverse normal ditry elements in ProcessPostTraversal when we process throttled animations restyle for event handling. r?emilio

nit: In the commit, s/ditry/dirty

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

https://reviewboard.mozilla.org/r/157128/#review162438

r=me, given my concern is addressed in following commits.

::: layout/base/ServoRestyleManager.cpp:604
(Diff revision 1)
>  
> +  const bool forThrottledAnimations =
> +    aRestyleBehavior == TraversalRestyleBehavior::ForFlushThrottledAnimations;
>    const bool descendantsNeedFrames =
>      aElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
>    const bool traverseElementChildren =

What about moving this to an inline function like:

```
static inline bool
NeedsToTraverseElementChildren(const Element& aParent, TraversalRestyleBehavior aRestyleBehavior)
{
  if (aParent.HasAnimationOnlyDirtyDescendantsForServo())
    return true;
  
  if (aRestyleBehavior != TraversalRestyleBehavior::ForAnimationOnly)
    return aParent.HasDirtyDescendantsForServo() || aParent.HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
  
  return false;
}
```

I think that'd make it easier to reason about, and then just doing:

```
const bool traverseElementChildren = NeedToTraverseElementChildren(*aElement, aRestyleBehavior);
```

looks slightly nicer, wdyt?

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

https://reviewboard.mozilla.org/r/157130/#review162440

::: servo/components/style/data.rs:28
(Diff revision 1)
>          /// Whether we reframed/reconstructed any ancestor or self.
>          const ANCESTOR_WAS_RECONSTRUCTED = 1 << 1,
>      }
>  }
>  
> +impl RestyleFlags {

I think that this doesn't need to be public, and there's no need to add the new functions to RestyleDamage either.

::: servo/components/style/data.rs:73
(Diff revision 1)
>      fn clear(&mut self) {
>          *self = Self::new();
>      }
>  
> +    /// Clear restyle flags and damage.
> +    fn clear_flags_and_damage(&mut self) {

Instead, you can just:

```
self.damage = RestyleDamage::empty();
self.flags = RestyleFlags::empty();
```

And you don't need to add a bunch of one-off methods.

::: servo/ports/geckolib/glue.rs:2726
(Diff revision 1)
> +                // In the case where we call this function for post traversal for
> +                // flusing throttled animations (i.e. without normal restyle
> +                // traversal), we need to preserve restyle hints for normal restyle
> +                // traversal. Restyle hints for animations have been already
> +                // removed during animation-only traversal.
> +                data.clear_restyle_flags_and_damage();

Shouldn't we clear the animation hint here too?

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162442

::: layout/base/ServoRestyleManager.cpp:452
(Diff revision 1)
>    const bool isOutOfFlow =
>      aElement->GetPrimaryFrame() &&
>      aElement->GetPrimaryFrame()->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW);
>  
> +  // NOTE: Servo_WasRestyled() should be called before Servo_TakeChangeHint()
> +  // since Servo_TakeChangeHint() discards WAS_RESTYLED flag.

I think this is fine, but given this function needs to be called here and doesn't make much sense otherwise, it'd be nice to instead get it as an outparam of `Servo_TakeChangeHint`, wdyt?

::: layout/base/ServoRestyleManager.cpp:533
(Diff revision 1)
>    // expensive checks in the common case.
>    //
>    // Also, we're going to need to check for pseudos of display: contents
>    // elements, though that is buggy right now even in non-stylo mode, see
>    // bug 1251799.
> -  const bool recreateContext = oldStyleContext &&
> +  const bool recreateContext = wasRestyled && oldStyleContext &&

I think you should be able to:

```
MOZ_ASSERT_IF(wasRestyled && oldStyleContext, oldStyleContext->ComputedValues() != computedValues);
```

Thus, you can just use the `wasRestyled` variable instead of recreateContext, and move the declaration and usage of `computedValues` inside the current `recreateContext` block.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886383 [details]
Bug 1371450 - Crash test.

https://reviewboard.mozilla.org/r/157134/#review162444

Comment 37

[Emilio Cobos Álvarez (:emilio)]

The approach in general looks great Hiro, thanks for fixing this!

My comments are mostly nits, though I still want to see a version with the revised patches. Thanks!

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886378 [details]
Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157124/diff/1-2/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886379 [details]
Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157126/diff/1-2/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157128/diff/1-2/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157130/diff/1-2/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157132/diff/1-2/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886383 [details]
Bug 1371450 - Crash test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157134/diff/1-2/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886379 [details]
Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

https://reviewboard.mozilla.org/r/157126/#review162434

> What if the animation is on the root? We presumably need to check that too, don't we?

Right, we should check it here. I did add is_restyle() check here.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

https://reviewboard.mozilla.org/r/157128/#review162438

> What about moving this to an inline function like:
> 
> ```
> static inline bool
> NeedsToTraverseElementChildren(const Element& aParent, TraversalRestyleBehavior aRestyleBehavior)
> {
>   if (aParent.HasAnimationOnlyDirtyDescendantsForServo())
>     return true;
>   
>   if (aRestyleBehavior != TraversalRestyleBehavior::ForAnimationOnly)
>     return aParent.HasDirtyDescendantsForServo() || aParent.HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
>   
>   return false;
> }
> ```
> 
> I think that'd make it easier to reason about, and then just doing:
> 
> ```
> const bool traverseElementChildren = NeedToTraverseElementChildren(*aElement, aRestyleBehavior);
> ```
> 
> looks slightly nicer, wdyt?

Thanks!  I added this inline function.
Actually we still independently need forThrottledAnimationFlush for other places, but I hope compilers can optimize it.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

https://reviewboard.mozilla.org/r/157130/#review162440

> Shouldn't we clear the animation hint here too?

I think animation hints should have removed during animation-only restyle. So, just added an additional assertion that checks we have no animation hints here.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162442

> I think this is fine, but given this function needs to be called here and doesn't make much sense otherwise, it'd be nice to instead get it as an outparam of `Servo_TakeChangeHint`, wdyt?

Thanks, did it.

> I think you should be able to:
> 
> ```
> MOZ_ASSERT_IF(wasRestyled && oldStyleContext, oldStyleContext->ComputedValues() != computedValues);
> ```
> 
> Thus, you can just use the `wasRestyled` variable instead of recreateContext, and move the declaration and usage of `computedValues` inside the current `recreateContext` block.

Thanks for the suggestion!  I could move computedValues declaration into the 'if (recreateContext)' block.  But wasRestyle could not place recreateContext since recreateContext involves oldStyleContext, whereas wasRestyled is true even if there is no oldStyleContext.

Comment 48

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162442

> Thanks for the suggestion!  I could move computedValues declaration into the 'if (recreateContext)' block.  But wasRestyle could not place recreateContext since recreateContext involves oldStyleContext, whereas wasRestyled is true even if there is no oldStyleContext.

I don't think it should involve recreateContext necessarily, that's the point. wasRestyled is more accurate than checking oldStyleContext.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #48)
> Comment on attachment 8886382 [details]
> Bug 1371450 - Recreate style context only if the element was restyled during
> the traversal.
> 
> https://reviewboard.mozilla.org/r/157132/#review162442
> 
> > Thanks for the suggestion!  I could move computedValues declaration into the 'if (recreateContext)' block.  But wasRestyle could not place recreateContext since recreateContext involves oldStyleContext, whereas wasRestyled is true even if there is no oldStyleContext.
> 
> I don't think it should involve recreateContext necessarily, that's the
> point. wasRestyled is more accurate than checking oldStyleContext.

OK. An obstacle to do that is 'MOZ_ASSERT(styleFrame || displayContentsNode)' in the recreateContext block. I have no idea what the appropriate assertion is for wasRestyled flag. So I will just drop it in the patch.  (I was about to ask it on IRC but quit since I don't want to disturb your precious sleep).

Comment 50

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886378 [details]
Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157124/diff/2-3/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886379 [details]
Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157126/diff/2-3/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157128/diff/2-3/

Comment 53

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157130/diff/2-3/

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157132/diff/2-3/

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886383 [details]
Bug 1371450 - Crash test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157134/diff/2-3/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162652

::: layout/base/ServoRestyleManager.cpp:535
(Diff revision 3)
>    // Note that we rely in the fact that we don't cascade pseudo-element styles
>    // separately right now (that is, if a pseudo style changes, the normal style
>    // changes too).
>    //
>    // Otherwise we should probably encode that information somehow to avoid
>    // expensive checks in the common case.

I am wondering these comments are no longer needed?

Comment 57

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b981201789a797ea8a0dd0b65aeeb49fb6bc66

Comment 58

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886379 [details]
Bug 1371450 - Return whether the root element has animation-only dirty bit from Servo_TraverseSubtree for flushing throttled animations.

https://reviewboard.mozilla.org/r/157126/#review162674

Comment 59

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

https://reviewboard.mozilla.org/r/157130/#review162676

Comment 60

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162680

r=me, thanks again for fixing this!

::: layout/base/ServoRestyleManager.cpp:561
(Diff revision 3)
>    // other elements without frames).
>    ServoRestyleState& childrenRestyleState =
>      thisFrameRestyleState ? *thisFrameRestyleState : aRestyleState;
>  
>    RefPtr<ServoStyleContext> newContext = nullptr;
> -  if (recreateContext) {
> +  if (wasRestyled) {

Given we don't do anything interesting in this block if there was no frame nor display: contents node, I think you can gate on `wasRestyled && oldStyleContext`, and keep the old assertion.

Comment 61

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162652

> I am wondering these comments are no longer needed?

Yeah, this comment is totally out of date since bug 1331047, you can rip it off.

Comment 62

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

https://reviewboard.mozilla.org/r/157132/#review162652

> I am wondering these comments are no longer needed?

Yeah, this comment is totally out of date since bug 1331047, you can rip it off.

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Thank you for the review!
https://github.com/servo/servo/pull/17740
A final try;
https://hg.mozilla.org/try/pushloghtml?changeset=b2034c442481b528e1a2116323c5d83f0255ca6e

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886378 [details]
Bug 1371450 - Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157124/diff/3-4/

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886380 [details]
Bug 1371450 - Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157128/diff/3-4/

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157130/diff/3-4/

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157132/diff/3-4/

Comment 68

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886383 [details]
Bug 1371450 - Crash test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157134/diff/3-4/

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886382 [details]
Bug 1371450 - Recreate style context only if the element was restyled during the traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157132/diff/4-5/

Comment 70

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8886383 [details]
Bug 1371450 - Crash test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157134/diff/4-5/

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d59bdf8a91ba
Rename TraversalRestyleBehavior::ForAnimationOnly to TraversalRestyleBehavior::ForThrottledAnimationFlush. r=birtles,emilio
https://hg.mozilla.org/integration/autoland/rev/98a02686d1ba
Don't traverse normal dirty elements in ProcessPostTraversal when we process throttled animations restyle for event handling. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8bca516a8bd2
Preserve restyle hints after ForThrottledAnimationFlush restyle. r=emilio
https://hg.mozilla.org/integration/autoland/rev/827b15ee4c8d
Recreate style context only if the element was restyled during the traversal. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3298b85ae1dc
Crash test. r=emilio

Comment 73

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d59bdf8a91ba
https://hg.mozilla.org/mozilla-central/rev/98a02686d1ba
https://hg.mozilla.org/mozilla-central/rev/8bca516a8bd2
https://hg.mozilla.org/mozilla-central/rev/827b15ee4c8d
https://hg.mozilla.org/mozilla-central/rev/3298b85ae1dc