1381431 - stylo: Assertion failure: !aElement->HasFlag(ELEMENT_HANDLED_SNAPSHOT), at /home/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:720

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

We sometimes hit this assertion, bug 1378064 comment 29 and bug 1380897 comment 4.

I got this assertion with the STR in bug 1378064 comment 29.

Here is a backtrace;

#0  0x00007fffe553bcf1 in mozilla::ServoRestyleManager::SnapshotFor (this=0x7fffc18e7240, aElement=0x7fffc171d4c0) at /home/ikezoe/stylo/layout/base/ServoRestyleManager.cpp:723
#1  0x00007fffe553c76c in mozilla::ServoRestyleManager::ContentStateChanged (this=0x7fffc18e7240, aContent=0x7fffc171d4c0, aChangedBits=...)
    at /home/ikezoe/stylo/layout/base/ServoRestyleManager.cpp:966
#2  0x00007fffe5546f40 in mozilla::RestyleManager::ContentStateChanged (this=0x7fffc18e7240, aContent=0x7fffc171d4c0, aStateMask=...)
    at /home/ikezoe/stylo/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:51
#3  0x00007fffe5518e70 in mozilla::PresShell::ContentStateChanged (this=0x7fffc7d3c000, aDocument=0x7fffb8880000, aContent=0x7fffc171d4c0, aStateMask=...)
    at /home/ikezoe/stylo/layout/base/PresShell.cpp:4302
#4  0x00007fffe301a81b in nsDocument::ContentStateChanged (this=0x7fffb8880000, aContent=0x7fffc171d4c0, aStateMask=...) at /home/ikezoe/stylo/dom/base/nsDocument.cpp:5415
#5  0x00007fffe2eefa60 in mozilla::dom::Element::NotifyStateChange (this=0x7fffc171d4c0, aStates=...) at /home/ikezoe/stylo/dom/base/Element.cpp:245
#6  0x00007fffe2f17294 in mozilla::dom::Element::RemoveStates (this=0x7fffc171d4c0, aStates=...) at /home/ikezoe/stylo/obj-firefox/dist/include/mozilla/dom/Element.h:601
#7  0x00007fffe4120eb1 in mozilla::EventStateManager::DoStateChange (aElement=0x7fffc171d4c0, aState=..., aAddState=false) at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4795
#8  0x00007fffe411acbc in mozilla::EventStateManager::UpdateAncestorState (aStartNode=0x7fffc171d4c0, aStopBefore=0x0, aState=..., aAddState=false)
    at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4825
#9  0x00007fffe411b60f in mozilla::EventStateManager::SetContentState (this=0x7fffb3b521e0, aContent=0x0, aState=...) at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4976
#10 0x00007fffe41182c7 in mozilla::EventStateManager::NotifyMouseOut (this=0x7fffb3b521e0, aMouseEvent=0x7fffffffc2c0, aMovingInto=0x0)
    at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4091
#11 0x00007fffe4118228 in mozilla::EventStateManager::NotifyMouseOut (this=0x7fffca1a8ae0, aMouseEvent=0x7fffffffc2c0, aMovingInto=0x0)
    at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4070
#12 0x00007fffe4118dca in mozilla::EventStateManager::GenerateMouseEnterExit (this=0x7fffca1a8ae0, aMouseEvent=0x7fffffffc2c0)
    at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:4316
#13 0x00007fffe410d9f3 in mozilla::EventStateManager::PreHandleEvent (this=0x7fffca1a8ae0, aPresContext=0x7fffc99a0800, aEvent=0x7fffffffc2c0, aTargetFrame=0x7fffc8e71118,-
    aTargetContent=0x0, aStatus=0x7fffffffc19c) at /home/ikezoe/stylo/dom/events/EventStateManager.cpp:730
#14 0x00007fffe552704f in mozilla::PresShell::HandleEventInternal (this=0x7fffc8e67000, aEvent=0x7fffffffc2c0, aStatus=0x7fffffffc19c, aIsHandlingNativeEvent=true)
    at /home/ikezoe/stylo/layout/base/PresShell.cpp:8147
#15 0x00007fffe5526607 in mozilla::PresShell::HandlePositionedEvent (this=0x7fffc8e67000, aTargetFrame=0x7fffc8e71118, aEvent=0x7fffffffc2c0, aEventStatus=0x7fffffffc19c)
    at /home/ikezoe/stylo/layout/base/PresShell.cpp:7944
#16 0x00007fffe552588e in mozilla::PresShell::HandleEvent (this=0x7fffc8e67000, aFrame=0x7fffc8e71118, aEvent=0x7fffffffc2c0, aDontRetargetEvents=false, aEventStatus=0x7fffffffc19c,-
    aTargetContent=0x0) at /home/ikezoe/stylo/layout/base/PresShell.cpp:7732
#17 0x00007fffe50b48a5 in nsViewManager::DispatchEvent (this=0x7fffc8e65300, aEvent=0x7fffffffc2c0, aView=0x7fffc8e56800, aStatus=0x7fffffffc19c)
    at /home/ikezoe/stylo/view/nsViewManager.cpp:804
#18 0x00007fffe50b2049 in nsView::HandleEvent (this=0x7fffc8e56800, aEvent=0x7fffffffc2c0, aUseAttachedEvents=true) at /home/ikezoe/stylo/view/nsView.cpp:1150
#19 0x00007fffe5125ed2 in nsWindow::DispatchEvent (this=0x7fffca1f2000, aEvent=0x7fffffffc2c0, aStatus=@0x7fffffffc250: nsEventStatus_eIgnore)
    at /home/ikezoe/stylo/widget/gtk/nsWindow.cpp:571
#20 0x00007fffe50c0bb7 in nsBaseWidget::DispatchInputEvent (this=0x7fffca1f2000, aEvent=0x7fffffffc2c0) at /home/ikezoe/stylo/widget/nsBaseWidget.cpp:1206
#21 0x00007fffe512beeb in nsWindow::OnLeaveNotifyEvent (this=0x7fffca1f2000, aEvent=0x7fffab399940) at /home/ikezoe/stylo/widget/gtk/nsWindow.cpp:2526

This is definitely a cause of bug 1378064, but now I have no idea what's going on, and this is really hard to reproduce. So I'd like handle this assertion case apart from bug 1378064. Firstly I'd like to know whether this assertion is a result of flushing throttled animations or not.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Quick question: Can we flush throttled animations without a normal style flush? If so, this assertion can trivially happen.

I suppose we should not do the snapshot business when we're on a throttled animation restyle.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Quick question: Can we flush throttled animations without a normal style
> flush? If so, this assertion can trivially happen.

As far as I can tell, yes.

> I suppose we should not do the snapshot business when we're on a throttled
> animation restyle.

OK, thanks as usual. I will try to do it.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

A try, seems green so far.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c71a397c49bd3d1d64ee1ccf8848f10e451e08f

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4146d71516c164616d8b796cf611961ddf0818
this try looks good.
I am hoping the patches in the try fix this bug but I am not yet sure because I can't reproduce this assertion any more (I saw the assertion just once that is comment 0). Also I am trying to write a test case but haven't succeeded yet.

Darkspirit,  would you mind take time to check whether the assertion still happens with the binary on the try? You may see another assertion (bug 1381731) though.

Comment 5

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4146d71516c164616d8b796cf611961ddf0818
> this try looks good.
> I am hoping the patches in the try fix this bug but I am not yet sure because I can't reproduce this assertion any more (I saw the assertion just once that is comment 0). Also I am trying to write a test case but haven't succeeded yet.
> 
> Darkspirit,  would you mind take time to check whether the assertion still happens with the binary on the try? You may see another assertion (bug 1381731) though.

Can't reproduce bug 1378064 in "Linux x64 Stylo opt", seems to be fixed.
But bug 1378064 blocks bug 1380897, which is not fixed in "Linux x64 Stylo opt".
Nothing relevant shown in my terminal.

Will try the debug build now:
> ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/worker/workspace/build/src/layout/base/RestyleManager.cpp, line 1186
multiple times, but no tab crash

Fullscreen button is always there, only the videos are fullscreen.
All in all it seems fine and would qualify to mark bug 1378064 as fixed (then) in my eyes.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Darkspirit from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4146d71516c164616d8b796cf611961ddf0818
> > this try looks good.
> > I am hoping the patches in the try fix this bug but I am not yet sure because I can't reproduce this assertion any more (I saw the assertion just once that is comment 0). Also I am trying to write a test case but haven't succeeded yet.
> > 
> > Darkspirit,  would you mind take time to check whether the assertion still happens with the binary on the try? You may see another assertion (bug 1381731) though.
> 
> Can't reproduce bug 1378064 in "Linux x64 Stylo opt", seems to be fixed.
> But bug 1378064 blocks bug 1380897, which is not fixed in "Linux x64 Stylo
> opt".
> Nothing relevant shown in my terminal.
> 
> Will try the debug build now:
> > ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/worker/workspace/build/src/layout/base/RestyleManager.cpp, line 1186
> multiple times, but no tab crash

This assertion is tracked in bug 1374966.

> Fullscreen button is always there, only the videos are fullscreen.
> All in all it seems fine and would qualify to mark bug 1378064 as fixed
> (then) in my eyes.

Thank you!

Comment 7

[Darkspirit]

(In reply to Darkspirit from comment #5)
> But bug 1378064 blocks bug 1380897, which is not fixed in "Linux x64 Stylo opt".
> Nothing relevant shown in my terminal.

But I have an (maybe not related) assertion with the debug build there: bug 1380897 comment 8
Please order full debug logs with you want tome.

Comment 8

[Darkspirit]

I want to apologize for clicking to fast: *some

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381431 - Factor out restyle_kind_for_animation.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381431 - Add has_current_styles_for_traversal().

In animation-only restyle, we just need to check the element
has animation restyle hints or has recascade self which is a result
of animation-only restyle for ancestors.
has_current_styles() in Servo_ResolveStyle() is intentionally left there,
it will be change in a subsequent patch.

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381431 - Skip snapshot handling during animation-only restyle.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381431 - Allow that restyle hints other than animation hints remain for flushing throttle animations.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1381431 - Crashtest that takes snapshots.

This test case takes snapshots so that the snapshots were handled during
animation-only restyle without prior patches in this patch series. That means
this test crashed without those prior patches.
To be clear, below assertions were hit respectively.

  debug_assert!(!shared_context.traversal_flags.for_animation_only()) in
  invalidate_style_if_needed()

  debug_assert!(element.has_current_styles(&*data) in Servo_ResolveStyle()

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887395 [details]
Bug 1381431 - Factor out restyle_kind_for_animation.

https://reviewboard.mozilla.org/r/158252/#review163418

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I can't provide any test cases that cause this '!aElement->HasFlag(ELEMENT_HANDLED_SNAPSHOT)' assertion, but attachment 8887399 [details] (which is taken from 1378064) causes another assertion checks it's not animation-only restyle in invalidate_style_if_needed() without other parts of these patches. The function, invalidate_style_if_needed, is the only place to call set_handled_snapshot(), so I think we ensure that we don't touch snapshot during animation-only restyle.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887396 [details]
Bug 1381431 - Add has_current_styles_for_traversal().

https://reviewboard.mozilla.org/r/158254/#review163420

Looks fine, though I don't understand the comment, mind explaining/elaborating and re-requesting review?

::: servo/components/style/dom.rs:488
(Diff revision 1)
> +    fn has_current_styles_for_traversal(&self,
> +                                        data: &ElementData,
> +                                        traversal_flags: TraversalFlags) -> bool {
> +        if traversal_flags.for_animation_only() {
> +            // In animation-only restyle we never touch snapshots and don't
> +            // care them. But we can't assert 'self.handled_snapshot()' here

nit: care about, presumably.

::: servo/components/style/dom.rs:493
(Diff revision 1)
> +            // care them. But we can't assert 'self.handled_snapshot()' here
> +            // since there are some cases that a second animation-only restyle
> +            // which is a result of normal restyle (e.g. setting animation-name
> +            // in normal restyle and creating a new CSS animation in a
> +            // SequentialTask) is processed after the normal traversal in that
> +            // we had elements that handled snapshot.

I don't really get this comment, mind elaborating?

If we have an animation-only traversal after the normal one, there we are able to say that we should've handled all snapshots.

But for animation traversals that happen in a standalone fashion we can't. Is that what you mean?

::: servo/components/style/dom.rs:494
(Diff revision 1)
> +            // since there are some cases that a second animation-only restyle
> +            // which is a result of normal restyle (e.g. setting animation-name
> +            // in normal restyle and creating a new CSS animation in a
> +            // SequentialTask) is processed after the normal traversal in that
> +            // we had elements that handled snapshot.
> +

nit: extra newline.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887397 [details]
Bug 1381431 - Skip snapshot handling during animation-only restyle.

https://reviewboard.mozilla.org/r/158256/#review163424

::: servo/components/style/data.rs:244
(Diff revision 1)
>      pub fn invalidate_style_if_needed<'a, E: TElement>(
>          &mut self,
>          element: E,
>          shared_context: &SharedStyleContext)
>      {
> +        debug_assert!(!shared_context.traversal_flags.for_animation_only());

I think it'd be cleaner to just return early here instead, and adding a comment about why.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887398 [details]
Bug 1381431 - Allow that restyle hints other than animation hints remain for flushing throttle animations.

https://reviewboard.mozilla.org/r/158258/#review163426

::: servo/ports/geckolib/glue.rs:2817
(Diff revision 1)
>  
>      // TODO(emilio): Downgrade to debug assertions when close to release.
>      assert!(data.has_styles(), "Resolving style on unstyled element");
> -    debug_assert!(element.has_current_styles(&*data),
> +    // In the case where we process for throttled animation, there remaings
> +    // restyle hints other than animation hints.
> +    debug_assert!(

indentation here looks quite weird... I'd just do:

```
let flags = if restyle_behavior { .. } else { .. };
debug_assert!(element.has_current_styles_for_traversal(&*data, flags), ...);
```

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887399 [details]
Bug 1381431 - Crashtest that takes snapshots.

https://reviewboard.mozilla.org/r/158260/#review163428

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> ::: servo/components/style/dom.rs:493
> (Diff revision 1)
> > +            // care them. But we can't assert 'self.handled_snapshot()' here
> > +            // since there are some cases that a second animation-only restyle
> > +            // which is a result of normal restyle (e.g. setting animation-name
> > +            // in normal restyle and creating a new CSS animation in a
> > +            // SequentialTask) is processed after the normal traversal in that
> > +            // we had elements that handled snapshot.
> 
> I don't really get this comment, mind elaborating?
> 
> If we have an animation-only traversal after the normal one, there we are
> able to say that we should've handled all snapshots.
> 
> But for animation traversals that happen in a standalone fashion we can't.
> Is that what you mean?

A second animation-only restyle which is subsequent to normal restyle, we may have elements which were marked as 'ELEMENT_HANDLED_SNAPSHOT', in other words the ELEMENT_HANDLED_SNAPSHOT flag has not yet cleared since they are cleared in a post traversal.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #18)
> Comment on attachment 8887398 [details]
> Bug 1381431 - Allow that restyle hints other than animation hints remain for
> flushing throttle animations.
> 
> https://reviewboard.mozilla.org/r/158258/#review163426
> 
> ::: servo/ports/geckolib/glue.rs:2817
> (Diff revision 1)
> >  
> >      // TODO(emilio): Downgrade to debug assertions when close to release.
> >      assert!(data.has_styles(), "Resolving style on unstyled element");
> > -    debug_assert!(element.has_current_styles(&*data),
> > +    // In the case where we process for throttled animation, there remaings
> > +    // restyle hints other than animation hints.
> > +    debug_assert!(
> 
> indentation here looks quite weird... I'd just do:
> 
> ```
> let flags = if restyle_behavior { .. } else { .. };
> debug_assert!(element.has_current_styles_for_traversal(&*data, flags), ...);
> ```

Ah, great. I thought we should avoid such temporary variables outside debug_assert. :)

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8887396 [details]
Bug 1381431 - Add has_current_styles_for_traversal().

https://reviewboard.mozilla.org/r/158254/#review163436

r=me with the comment fixed

::: servo/components/style/dom.rs:488
(Diff revision 1)
> +    fn has_current_styles_for_traversal(&self,
> +                                        data: &ElementData,
> +                                        traversal_flags: TraversalFlags) -> bool {
> +        if traversal_flags.for_animation_only() {
> +            // In animation-only restyle we never touch snapshots and don't
> +            // care them. But we can't assert 'self.handled_snapshot()' here

Oh, so what you want to assert is `!self.handled_snapshot()`, the comment says the opposite. With that, the comment looks reasonable :)

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> Comment on attachment 8887396 [details]
> Bug 1381431 - Add has_current_styles_for_traversal().
> 
> https://reviewboard.mozilla.org/r/158254/#review163436
> 
> r=me with the comment fixed
> 
> ::: servo/components/style/dom.rs:488
> (Diff revision 1)
> > +    fn has_current_styles_for_traversal(&self,
> > +                                        data: &ElementData,
> > +                                        traversal_flags: TraversalFlags) -> bool {
> > +        if traversal_flags.for_animation_only() {
> > +            // In animation-only restyle we never touch snapshots and don't
> > +            // care them. But we can't assert 'self.handled_snapshot()' here
> 
> Oh, so what you want to assert is `!self.handled_snapshot()`, the comment
> says the opposite. With that, the comment looks reasonable :)

Oh yes exactly! sorry for my idiot mistake, assertions in comment never catches anything.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8887398 [details]
Bug 1381431 - Allow that restyle hints other than animation hints remain for flushing throttle animations.

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

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8887399 [details]
Bug 1381431 - Crashtest that takes snapshots.

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

Comment 26

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/17770

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb3468c3d642
Allow that restyle hints other than animation hints remain for flushing throttle animations. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6c15e40e23ef
Crashtest that takes snapshots. r=emilio

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb3468c3d642
https://hg.mozilla.org/mozilla-central/rev/6c15e40e23ef