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.

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.

(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.

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

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.

(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.

(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 on attachment 8887395 [details] Bug 1381431 - Factor out restyle_kind_for_animation. https://reviewboard.mozilla.org/r/158252/#review163418

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 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 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 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 on attachment 8887399 [details] Bug 1381431 - Crashtest that takes snapshots. https://reviewboard.mozilla.org/r/158260/#review163428

(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.

(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 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 :)

(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.

Pushed by hikezoe@mozilla.com: 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

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