1378064 - stylo: site issue: when clicking to make a video fullscreen, sometimes the website and not the video gets fullscreen

Status: VERIFIED FIXED

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

Description

[Darkspirit]

Attachment: broken-fullscreen.png

You always click on the button to make the video fullscreen and on the same button to leave fullscreen. Repeat and repeat and repeat. Sometimes this bug appear.

Seen on:
* https://i.4cdn.org/wsg/1493771772577.webm
* Sometimes, the button to make a video fullscreen disappear and the website gets fullscreen. You have to press escape. https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I cannot reproduce this issue.

With the first url (the .webm one), I hit bug 1378646, which may or may not cause this issue. And I cannot reproduce this with the second link in my local debug build.

Comment 2

[Darkspirit]

Attachment: Screenshot_20170706_113655.png

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> And I cannot reproduce this with the second link in my local debug build.

(In reply to Darkspirit from comment #0)
> * Sometimes, the button to make a video fullscreen disappear and the website gets fullscreen. You have to press escape.
> https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html

Maybe this is a bit like bug 1378208 comment 5:
I tried to reproduce with a fresh profile now, doesn't work.
Then I clicked to make fullscreen, but pressed escape multiple times, and then again fullscreen and back both via that button.
No.
Then I installed uBlock Origin (legacy) from AMO (I have a real webextension of uBlock Origin in my regular profile),
tried again a bit, and then this bug happened with the second link. It may have been a coincidence, I don't know.
You can see one this screenshot that the video fullscreen button is gone. No, I didn't pressed F11, I clicked only on that button to make the video fullscreen.

Comment 3

[Darkspirit]

Attachment: Screenshot_20170706_114648.png

Nope, #incompetent. This time it happened only with a fresh profile (without uBlock Origin).

Comment 4

[Darkspirit]

Hm, as I am currently not that helpful:
If "it hits the debug_assert in my local debug build" (bug 1378646 comment 0) would be just shown in the terminal, then it should be easy for me to test that for you. Building Nightly on Debian is fortunately so simple.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Now I am convinced this bug is also caused by bug 1378646, in the case where the panic happens on release build, we will get incorrect style values, so, in the case of this bug, the fullscreen button is rendered with the incorrect style.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Darkspirit, could you please try this binary to reproduce this issue?

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

This try has an ugly fix for bug 1378646, I did confirm that this binary fixed bug 1377739.
Thanks!

Comment 7

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Darkspirit, could you please try this binary to reproduce this issue?
> 
> https://treeherder.mozilla.org/#/ jobs?repo=try&revision=c012f37d68c90b9d74ec6f64e6a91f7358dd18bf
> 
> This try has an ugly fix for bug 1378646, I did confirm that this binary
> fixed bug 1377739.
> Thanks!

Linux x64 Stylo opt > clicked on tc[tier:2] > B > downloaded target.tar.bz2
This is Nightlx 56 20170706110101, stylo is enabled by default.
Created a fresh profile via -P, then ran firefox/firefox -P "cleanstylo3tmp" --no-remote
The language was english, so I was really using your build.
bug 1377739 seems to be fixed, but this one not: this bug can still happen in both cases exactly like on the screenshots in attachment 8883215 [details] and attachment 8883886 [details] (but with english firefox locales).

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thank you for the quick check!  Then I have no clue for this bug, I can't reproduce this locally at all, I haven't installed the addon yet.

Comment 9

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Thank you for the quick check! Then I have no clue for this bug, I can't reproduce this locally at all, I haven't installed the addon yet.

Which addon? I don't think ublock origin is the cause for this. (Haven't installed it in my fresh profiles I used for reproducing this bug.) Or do you mean an addon to analyse what's going on?

So you can't even reproduce this: Could I run a debug build or something else and what would I then have to do to maybe catch some logs or whatever is going on for you?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Oh I thought ublock origin needs to reproduce.

(In reply to Darkspirit from comment #9)
> So you can't even reproduce this: Could I run a debug build or something
> else and what would I then have to do to maybe catch some logs or whatever
> is going on for you?

Yeah, some assertions might happen there, or if no assertions happen you can get the log with RUST_LOG=debug, the log will be huge but might be useful.

Comment 11

[Darkspirit]

My first try (before I got your message):

this one I get very often while making the video fullscreen and back via click:
[Child 5023] WARNING: stylo: HasStateDependentStyle always returns zero!: file /home/worker/workspace/build/src/layout/style/ServoStyleSet.cpp, line 865

tab crash:
Gah. Your tab just crashed.
We can help you!
Choose Restore This Tab to reload page content.

Assertion failure: !aElement->HasFlag(ELEMENT_HANDLED_SNAPSHOT), at /home/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:696
#01: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a564ce]
#02: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a63fcc]
#03: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1959373]
#04: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x18ba8ec]
#05: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20ccd93]
#06: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d0b1c]
#07: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d59f6]
#08: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d5f1a]
#09: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d93cc]
#10: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a7483b]
#11: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a751a3]
#12: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a76eb3]
#13: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2813e4f]
#14: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x28160be]
#15: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2837026]
#16: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x16b19cc]
#17: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x260fbe8]
#18: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x12fa8ab]
#19: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1106cf3]
#20: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x111180d]
#21: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1113bf4]
#22: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1113d68]
#23: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd414a6]
#24: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd57c39]
#25: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd5389b]
#26: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1107fa8]
#27: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc03]
#28: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc2a]
#29: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x28388a9]
#30: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x37e30c5]
#31: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1108093]
#32: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc03]
#33: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc2a]
#34: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x37e35a1]
#35: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x629d]
#36: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x5cba]
#37: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x202b1]
#38: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x5ecd]

Program /home/darkspirit/Downloads/tmp/firefox/firefox (pid = 5023) received signal 11.
Stack:
#01: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x3e7cd6d]
#02: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x41ada60]
#03: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x110c0]
#04: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a56266]
#05: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a564ce]
#06: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a63fcc]
#07: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1959373]
#08: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x18ba8ec]
#09: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20ccd93]
#10: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d0b1c]
#11: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d59f6]
#12: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d5f1a]
#13: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x20d93cc]
#14: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a7483b]
#15: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a751a3]
#16: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2a76eb3]
#17: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2813e4f]
#18: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x28160be]
#19: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x2837026]
#20: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x16b19cc]
#21: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x260fbe8]
#22: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x12fa8ab]
#23: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1106cf3]
#24: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x111180d]
#25: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1113bf4]
#26: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1113d68]
#27: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd414a6]
#28: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd57c39]
#29: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0xd5389b]
#30: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1107fa8]
#31: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc03]
#32: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc2a]
#33: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x28388a9]
#34: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x37e30c5]
#35: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x1108093]
#36: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc03]
#37: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x10dbc2a]
#38: ???[/home/darkspirit/Downloads/tmp/firefox/libxul.so +0x37e35a1]
#39: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x629d]
#40: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x5cba]
#41: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x202b1]
#42: ???[/home/darkspirit/Downloads/tmp/firefox/firefox +0x5ecd]
Sleeping for 300 seconds.
Type 'gdb /home/darkspirit/Downloads/tmp/firefox/firefox 5023' to attach your debugger to this thread.

###!!! [Parent][MessageChannel] Error: (msgtype=0x280066,name=PBrowser::Msg_UpdateDimensions) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x28006E,name=PBrowser::Msg_RealMouseButtonEvent) Channel error: cannot send/recv

[Parent 4735] WARNING: SendRealMouseButtonEvent() failed: 'ret', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1133

###!!! [Parent][MessageChannel] Error: (msgtype=0x28006D,name=PBrowser::Msg_SynthMouseMoveEvent) Channel error: cannot send/recv

[Parent 4735] WARNING: SendSynthMouseMoveEvent() failed: 'ret', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1122

###!!! [Parent][MessageChannel] Error: (msgtype=0x28006C,name=PBrowser::Msg_RealMouseMoveEvent) Channel error: cannot send/recv

[Parent 4735] WARNING: SendRealMouseMoveEvent() failed: 'ret', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1127

###!!! [Parent][MessageChannel] Error: (msgtype=0x28006C,name=PBrowser::Msg_RealMouseMoveEvent) Channel error: cannot send/recv

[Parent 4735] WARNING: SendRealMouseMoveEvent() failed: 'ret', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1127

###!!! [Parent][MessageChannel] Error: (msgtype=0x28006C,name=PBrowser::Msg_RealMouseMoveEvent) Channel error: cannot send/recv

[Parent 4735] WARNING: SendRealMouseMoveEvent() failed: 'ret', file /home/worker/workspace/build/src/dom/ipc/TabParent.cpp, line 1127

###!!! [Parent][MessageChannel] Error: (msgtype=0x28007E,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

++DOCSHELL 0x7f6c774e4000 == 8 [pid = 4735] [id = {1c50e285-e0df-4b4b-9389-71f0e3e61a48}]
++DOMWINDOW == 15 (0x7f6c8fd41000) [pid = 4735] [serial = 35] [outer = (nil)]
[Parent 4735] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13176
++DOMWINDOW == 16 (0x7f6c91d26800) [pid = 4735] [serial = 36] [outer = 0x7f6c8fd41000]
++DOMWINDOW == 17 (0x7f6c93d31800) [pid = 4735] [serial = 37] [outer = 0x7f6c8fd41000]
--DOMWINDOW == 16 (0x7f6c91d26800) [pid = 4735] [serial = 36] [outer = (nil)] [url = about:blank]

Comment 12

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Oh I thought ublock origin needs to reproduce.
It seemed partially so.

(In reply to Darkspirit from comment #2)
> Maybe this is a bit like bug 1378208 comment 5
This was my perception. I tried and tried, but I could only reproduce the first (4chan) link in a fresh profile. Then I installed that addon and it was possible for me to reproduce the zdf.de link in a fresh profile, too. 

> Yeah, some assertions might happen there, or if no assertions happen you can get the log with RUST_LOG=debug, the log will be huge but might be useful.

I will try that.

Comment 13

[Darkspirit]

RUST_LOG=debug firefox/firefox -P "cleanstylo3tmp" --no-remote &> 4chan.log
= https://cloud.terrax.net/s/j1AFmoH86l76ldk

Normally I don't get a tab crash while clicking to make the video fullscreen and back. I hope this tab crash in the debug build is the same bug.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

CCing Cameron and Emilio since this might be related to snapshot thing as per comment 11.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> CCing Cameron and Emilio since this might be related to snapshot thing as
> per comment 11.

So, sure it is, but I'm heavily confused about all the stuff the animation restyles are doing... That assertion shouldn't happen, but I just looked and this _can_ happen with animation-only restyles.

In particular, on an animation-only restyle, we don't clear the snapshot table (because we're supposedly only handling animation restyles).

However, there's nothing preventing us to handle snapshot stuff in an animation only restyle on the servo side (see ElementStyleData::invalidate_style_if_needed).

I'm not sure what the animation code is trying to do here, but I suppose it shouldn't handle snapshots. In particular, all the animation-only restyle seems quite buggy at the moment due to everything that happens but shouldn't (like changehints on the initial restyle, see bug 1374175, which landed a pretty terrible hack which is really a workaround which I feel is buggy).

So I think we should get this clear, get which parts of the restyling process should be handled by the animation code and which ones shouldn't, and either:

 * Add stronger assertions about what an animation-only restyle may or may not do (and make them hold).
 * Split the "animation-only restyle" into something completely different from the normal style traversal, sharing code when appropriate.

But over-all, stop trying to workaround stuff, and make all this stuff sound, because I fear right now it isn't.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Yes, fair enough.  As for animation-only restyle, as far as I can tell, there is nothing special other than not triggering transitions.

Your comments makes me re-think this bug is relevant to bug 1371450.

In the case where we call DoProcessPendingRestyles from UpdateAnimationOnlyStyles (This name is somewhat misleading, the only one purpose of this function is to sync transform animations running on the compositor to get correct element positions for event handling) we skip to call ClearSnapShot [1]. The stack in comment 11 does not include useful symbols, so it's just a guess, but I think bug 1371450 will fix this bug too.

[1] https://hg.mozilla.org/mozilla-central/file/5eeee16f1659/layout/base/ServoRestyleManager.cpp#l783

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Yes, fair enough.  As for animation-only restyle, as far as I can tell,
> there is nothing special other than not triggering transitions.

Any reason it shouldn't clear the snapshot table then? I believe this can be the cause of some missing restyle bugs like bug 1379203 (building now with a patch to verify it).

I suppose it is because we don't necessarily follow the normal dirty descendants bit (instead of the animation-only dirty descendants bit), and thus we don't arrive to the element properly. But, if that's the case, it seems to me that we shouldn't actually be doing the snapshot stuff there, what do you think?

We really really need to make this transitions stuff sound, otherwise those nasty little bugs super-hard to repro and track down will keep coming, I suppose.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Taking this too.  As I commented bug 1379203 comment 4, yes, we shouldn't clear snapshot if we don't need to do normal traversal when we flush style.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Yes, fair enough.  As for animation-only restyle, as far as I can tell,
> there is nothing special other than not triggering transitions.
> 
> Your comments makes me re-think this bug is relevant to bug 1371450.
> 
> In the case where we call DoProcessPendingRestyles from
> UpdateAnimationOnlyStyles (This name is somewhat misleading, the only one
> purpose of this function is to sync transform animations running on the
> compositor to get correct element positions for event handling)

Filed bug 1379534 for this confusion.

Comment 20

[Darkspirit]

Attachment: Screenshot_20170714_025727.png

@hiro

This bug seems to be partially fixed in the try build from bug 1371450 comment 13:

* URL from attached screenshot: https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html

Now, not the whole website is fullscreen sometimes, but only the video (in the size it was displayed on the website) and there are no video controls on the video or inside the black at the bottom.

* The 4chan webm url where the native video player is used seems to be fine now.

Comment 21

[Darkspirit]

(The log from comment 13 has a new link that doesn't expire: https://cloud.terrax.net/s/jngbcnN4sRvdisz)


(In reply to Darkspirit from comment #20)
Tested now with the debug (!) try build from bug 1371450 comment 13.

RUST_LOG=debug firefox_apz-debug/firefox -P "cleanstylo6" --no-remote &> zdf3.log
= https://cloud.terrax.net/s/Y429eYh34EDw0JO
Played the video, then paused it because fullscreen and back would be too slow, then made it fullscreen and back via click and again and again, then a tab crash, clicked on "Close this tab" button, then the main window closed, but the firefox process and one Web process don't want to end, so pressed Ctrl+C in my terminal, that killed "firefox", but there is still one "Web" process from that apz debug build running, I had to kill it.

assertions:
> [Child 29416] ###!!! ASSERTION: Convert method should only be passed a single family name: 'aFamilyOrGenericName.FindChar(',') == -1', file /home/worker/workspace/build/src/gfx/thebes/gfxFontFamilyList.h, line 133
2 times

> [Child 29416] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/worker/workspace/build/src/layout/base/RestyleManager.cpp, line 1186
1 time

Comment 22

[Darkspirit]

(In reply to Darkspirit from comment #21)
> > [Child 29416] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/worker/workspace/build/src/layout/base/RestyleManager.cpp, line 1186
> 1 time
I wonder why it's "RestyleManager.cpp" and not "ServoRestyleManager.cpp", but stylo is active:
about:support > Stylo: true (enabled by default)

Comment 23

[Cameron McCormack (:heycam)]

RestyleManager is a superclass of both ServoRestyleManager and GeckoRestyleManager, and has some common functionality that both use.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Crash test

Thank you Darkspirit. I could finally reproduce the panic locally.
In this bug's case, we modify an element style that the element itself has an animation. We need to tweak element.has_current_styles() check in Servo_ResolveStyle().

Attaching file is a crash test for this issue. An interesting point is that, unlike bug 1371450, we need to use requestAnimationFrame() instead of requestIdleCallback(). Whereas, IIRC, bug 1371450 needs requestIdlCallback() instead of requestAnimationFrame().  I don't know why.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

The only way I can think of to avoid the panic is we allow restyle hints other than animation's in  Servo_ResolveStyle().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffdb95ba304b6cbd71ad5db91ae695ce68c055dd

Comment 26

[Hiroyuki Ikezoe (:hiro)]

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

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

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1378064 - Crashtest.

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

Comment 28

[Darkspirit]

Attachment: Screenshot_20170716_143817.png

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> The only way I can think of to avoid the panic is we allow restyle hints
> other than animation's in  Servo_ResolveStyle().
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffdb95ba304b6cbd71ad5db91ae695ce68c055dd

* 4chan seems to be still fixed

* zdf.de is a bit more fixed, but not completely.
See screenshot: The custom player is now fullscreen every time, but sometimes the fullscreen button is gone.

https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html

Comment 29

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> The only way I can think of to avoid the panic is we allow restyle hints
> other than animation's in  Servo_ResolveStyle().
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffdb95ba304b6cbd71ad5db91ae695ce68c055dd

Tried with "Linux x64 Stylo debug" from above link:

> [Child 17363] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/worker/workspace/build/src/layout/base/RestyleManager.cpp, line 1186

I think the tab crash happened after this:
> Assertion failure: !aElement->HasFlag(ELEMENT_HANDLED_SNAPSHOT), at /home/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:720

Same procedure as before:
"Played the video, then paused it because fullscreen and back would be too slow, then made it fullscreen and back via click and again and again, then a tab crash, clicked on "Close this tab" button, then the main window closed, but the firefox process and one Web process don't want to end, so pressed Ctrl+C in my terminal, that killed "firefox", but there is still one "Web" process from that apz debug build running, I had to kill it."

https://cloud.terrax.net/s/dyn4If5xfHN8QgM

Comment 30

[Darkspirit]

(In reply to Darkspirit from comment #29)
Those quotes are from the Opt try build, but starting with "assertion" everything is the same in the debug log.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

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

https://reviewboard.mozilla.org/r/157590/#review162710

So, this looks fine, but what happens if we happen to reframe the element? We'd go into frame construction, which passes `TraversalRestyleBehavior::Normal`, wouldn't we?

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886834 [details]
Bug 1378064 - Crashtest.

https://reviewboard.mozilla.org/r/157592/#review162712

Comment 33

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> Comment on attachment 8886833 [details]
> Bug 1378064 - Allow that restyle hints other than animation hints remain for
> flushing throttle animations.
> 
> https://reviewboard.mozilla.org/r/157590/#review162710
> 
> So, this looks fine, but what happens if we happen to reframe the element?
> We'd go into frame construction, which passes
> `TraversalRestyleBehavior::Normal`, wouldn't we?

Oh right.  The reframe case causes the assertion in comment 29?

Comment 34

[Hiroyuki Ikezoe (:hiro)]

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

Clearing review request.  Today is a national holiday in Japan, I will be mostly off-line, so I can't revise this patch soon.

Comment 35

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> The reframe case causes the assertion in comment 29?
As a note to that assertion: bug 1371450 (or something that landed today) may have caused a regression: tab crash bug 1381327 (since today's nightly) + a spike of bug 1379218 ("seem to be relating to interaction with video elements")

Comment 36

[Cameron McCormack (:heycam)]

When I applied this patch to test whether it fixed a case of unstyled elements I was encountering on Fastmail, I ran into:

thread '<unnamed>' panicked at 'Restyle damage should be empty if the element was not restyled', /z/moz/d/servo/ports/geckolib/glue.rs:2759

in Servo_TakeChangeHint.  Is it possible that assertion needs updating too, or is this a separate issue?

Comment 37

[Emilio Cobos Álvarez (:emilio)]

That definitely shouldn't be failing, no matter what...

Comment 38

[Cameron McCormack (:heycam)]

Filed bug 1381420 for that.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Crash test with an animation might produces reconstruct frame

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> > Comment on attachment 8886833 [details]
> > Bug 1378064 - Allow that restyle hints other than animation hints remain for
> > flushing throttle animations.
> > 
> > https://reviewboard.mozilla.org/r/157590/#review162710
> > 
> > So, this looks fine, but what happens if we happen to reframe the element?
> > We'd go into frame construction, which passes
> > `TraversalRestyleBehavior::Normal`, wouldn't we?
> 
> Oh right.  The reframe case causes the assertion in comment 29?

Filed bug 1381431 for the assertion. It seems to me that the assertion is not related to reframe case.
Attaching test case has column-count animation which should produce nsChangeHint_ReconstructFrame but it doesn't for now. I guess once bug 1367904 is fixed, we will get the change hint.  Anyway, in case of nsChangeHint_ReconstructFrame we return early [1] in ProcessPostTraversal() and clear restyle data, so it should not affect Servo_ResolveStyle, I think.

[1] https://hg.mozilla.org/mozilla-central/file/aff336ac161d/layout/base/ServoRestyleManager.cpp#l510

Comment 40

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #39)
> Created attachment 8886999 [details] [diff] [review]
> Crash test with an animation might produces reconstruct frame
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> > > Comment on attachment 8886833 [details]
> > > Bug 1378064 - Allow that restyle hints other than animation hints remain for
> > > flushing throttle animations.
> > > 
> > > https://reviewboard.mozilla.org/r/157590/#review162710
> > > 
> > > So, this looks fine, but what happens if we happen to reframe the element?
> > > We'd go into frame construction, which passes
> > > `TraversalRestyleBehavior::Normal`, wouldn't we?
> > 
> > Oh right.  The reframe case causes the assertion in comment 29?
> 
> Filed bug 1381431 for the assertion. It seems to me that the assertion is
> not related to reframe case.
> Attaching test case has column-count animation which should produce
> nsChangeHint_ReconstructFrame but it doesn't for now. I guess once bug
> 1367904 is fixed, we will get the change hint.  Anyway, in case of
> nsChangeHint_ReconstructFrame we return early [1] in ProcessPostTraversal()
> and clear restyle data, so it should not affect Servo_ResolveStyle, I think.

Hmm, yeah, I guess clearing the restyle data saves the day here, I didn't remember us doing that... Is that actually correct, wouldn't it make us miss restyles when coming from an animation-only restyle? I think it could...

Comment 41

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #40) 
> Hmm, yeah, I guess clearing the restyle data saves the day here, I didn't
> remember us doing that... Is that actually correct, wouldn't it make us miss
> restyles when coming from an animation-only restyle? I think it could...

I mean, that makes us drop both animation restyle state, _and_ normal restyle state, so it's unsound I think if we arrive there from an animation restyle.

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Attachment: Crash test with an animation might produces reconstruct frame

I did attach incorrect one. Here is the correct one.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Closing as per bug 1381431 comment 5.

Comment 44

[Darkspirit]

Not fixed in today's Nightly 56 x64 20170719100147 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480).
I will try to verify it tomorrow.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Oh, then this bug should depend on bug 1381420, it's the last problem (as far as I can tell) for mis-styled by triggered mouse events.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(In reply to Darkspirit from comment #44)
> Not fixed in today's Nightly 56 x64 20170719100147 @ Debian Testing (Linux
> 4.11.0-1-amd64, Radeon RX480).
> I will try to verify it tomorrow.

Oops, sorry.  I did not read your comment carefully.  Right, today's nightly does not have the fix for bug 1381431 yet.

Comment 47

[Darkspirit]

Can no longer reproduce in Nightly 56 x64 20170720100139 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480).