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

VERIFIED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
VERIFIED FIXED
20 days ago
4 days ago

People

(Reporter: Darkspirit, Assigned: hiro)

Tracking

(Blocks: 1 bug, {nightly-community})

56 Branch
x86_64
Linux
nightly-community
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

(Reporter)

Description

20 days ago
Created attachment 8883215 [details]
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
(Reporter)

Updated

20 days ago
Blocks: 1375906
Has STR: --- → yes
Keywords: nightly-community
Depends on: 1378646
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.
(Reporter)

Comment 2

18 days ago
Created attachment 8883885 [details]
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.
(Reporter)

Comment 3

18 days ago
Created attachment 8883886 [details]
Screenshot_20170706_114648.png

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

Comment 4

18 days ago
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.
(Assignee)

Comment 5

18 days ago
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.
(Assignee)

Comment 6

18 days ago
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!
Flags: needinfo?(bugzilla)
(Reporter)

Comment 7

18 days ago
(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).
Flags: needinfo?(bugzilla)
(Assignee)

Comment 8

18 days ago
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.
(Reporter)

Comment 9

18 days ago
(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?
(Assignee)

Comment 10

18 days ago
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.
(Reporter)

Comment 11

18 days ago
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]
(Reporter)

Comment 12

18 days ago
(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.
(Reporter)

Comment 13

18 days ago
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.
(Assignee)

Comment 14

18 days ago
CCing Cameron and Emilio since this might be related to snapshot thing as per comment 11.
(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.
Flags: needinfo?(hikezoe)
(Assignee)

Comment 16

17 days ago
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
Flags: needinfo?(hikezoe)
(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.
Flags: needinfo?(hikezoe)
See Also: → bug 1379425
(Assignee)

Comment 18

15 days ago
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
(Assignee)

Comment 19

15 days ago
(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.
Priority: -- → P1
(Reporter)

Comment 20

11 days ago
Created attachment 8886441 [details]
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.
(Reporter)

Comment 21

11 days ago
(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
(Reporter)

Comment 22

11 days ago
(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)
RestyleManager is a superclass of both ServoRestyleManager and GeckoRestyleManager, and has some common functionality that both use.
(Assignee)

Comment 24

9 days ago
Created attachment 8886771 [details]
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.
(Assignee)

Comment 25

9 days ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 28

8 days ago
Created attachment 8886850 [details]
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
(Reporter)

Comment 29

8 days ago
(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
(Reporter)

Comment 30

8 days ago
(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

8 days ago
mozreview-review
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

8 days ago
mozreview-review
Comment on attachment 8886834 [details]
Bug 1378064 - Crashtest.

https://reviewboard.mozilla.org/r/157592/#review162712
Attachment #8886834 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 33

8 days ago
(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?
(Assignee)

Comment 34

8 days ago
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.
Attachment #8886833 - Flags: review?(emilio+bugs)
(Reporter)

Comment 35

8 days ago
(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")
Blocks: 1380897
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?
That definitely shouldn't be failing, no matter what...
Filed bug 1381420 for that.
(Assignee)

Comment 39

7 days ago
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.

[1] https://hg.mozilla.org/mozilla-central/file/aff336ac161d/layout/base/ServoRestyleManager.cpp#l510
(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...
(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.
(Assignee)

Comment 42

7 days ago
Created attachment 8887003 [details]
Crash test with an animation might produces reconstruct frame

I did attach incorrect one. Here is the correct one.
Attachment #8886999 - Attachment is obsolete: true
Blocks: 1379218
Depends on: 1381431
Blocks: 1381475
(Assignee)

Updated

7 days ago
See Also: → bug 1381327
(Assignee)

Comment 43

5 days ago
Closing as per bug 1381431 comment 5.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 days ago
Resolution: --- → FIXED
(Reporter)

Comment 44

5 days ago
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.
(Assignee)

Comment 45

5 days ago
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.
(Assignee)

Comment 46

5 days ago
(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.
(Reporter)

Comment 47

4 days ago
Can no longer reproduce in Nightly 56 x64 20170720100139 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480).
Status: RESOLVED → VERIFIED
No longer blocks: 1380897
You need to log in before you can comment on or make changes to this bug.