Closed Bug 1371450 Opened 2 years ago Closed 2 years ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rillian, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

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
Stability, P1.
Priority: -- → P1
CCind Boris, since the stack includes UpdateOnlyAnimationStyles().
Assignee: nobody → boris.chiou
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.
(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.
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.
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.
(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).
(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.
See Also: → 1377739
Duplicate of this bug: 1378646
See Also: → 1379425
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.
Assignee: boris.chiou → hikezoe
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.
Blocks: 1378987
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.
(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.
(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!
See Also: 1377739
Duplicate of this bug: 1377739
Duplicate of this bug: 1379425
(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.
You can use tools inside WindowSnapshot.js to do reftest in mochitest.
Wow! thank you Xidorn!
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 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)
Attachment #8886378 - Flags: review?(bbirtles) → review+
(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 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 :)
Attachment #8886378 - Flags: review?(emilio+bugs) → review+
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 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 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?
Attachment #8886380 - Flags: review?(emilio+bugs) → review+
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 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 on attachment 8886383 [details]
Bug 1371450 - Crash test.

https://reviewboard.mozilla.org/r/157134/#review162444
Attachment #8886383 - Flags: review?(emilio+bugs) → review+
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 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 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 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 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 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.
(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 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?
Blocks: 1381185
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
Attachment #8886379 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886381 [details]
Bug 1371450 - Preserve restyle hints after ForThrottledAnimationFlush restyle.

https://reviewboard.mozilla.org/r/157130/#review162676
Attachment #8886381 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8886382 - Flags: review?(emilio+bugs) → review+
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 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.
Attachment #8886379 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
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
Duplicate of this bug: 1376832
No longer blocks: 1380543
Duplicate of this bug: 1380543
See Also: → 1381327
Blocks: 1380957
You need to log in before you can comment on or make changes to this bug.