Closed Bug 1415781 Opened 2 years ago Closed 2 years ago

Fix failure in layout/style/test/chrome/ relying on non-comformant Promise handling


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




Tracking Status
firefox59 --- fixed


(Reporter: bevis, Assigned: xidorn)




(2 files)

This is a follow-up of the try result in bug 1193394 comment 48:
(c3 in linux64-debug-build:

More updated patches are attached in bug 1193394.

GECKO(1923) | Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Failed to exit fullscreen?), at /builds/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:7321
TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode.html | application terminated with exit code 11
PROCESS-CRASH | layout/style/test/chrome/test_display_mode.html | application crashed [@ nsGlobalWindow::FinishFullscreenChange]
Priority: -- → P3
layout/style/test/chrome/test_display_mode_reflow.html is also failed in linux64 opt build:
FAIL | layout/style/test/chrome/test_display_mode_reflow.html | offset top changes
Summary: Fix failure of layout/style/test/chrome/test_display_mode.html relying on non-comformant Promise handling → Fix failure in layout/style/test/chrome/ relying on non-comformant Promise handling
ni? myself for the fullscreen crash.

I don't have a Linux environment at the moment, though :/
Flags: needinfo?(xidorn+moz)
Ah it seems the log in comment 0 has expired...

Bevis, could you do a new push for the fullscreen failure?
Flags: needinfo?(xidorn+moz) → needinfo?(btseng)
I wasn't going to redirect my own ni? though...
Flags: needinfo?(xidorn+moz)
Thanks for looking into this!

The try result will be updated weekly in bug 1193394. So you can check bug 1193394 comment 71 for the latest result:
1. layout/style/test/chrome/test_display_mode.html on linux debug(x86/64)
2. layout/style/test/chrome/test_display_mode_reflow.html on linux opt(x86/64)
Flags: needinfo?(btseng)
So the problem for test_display_mode here is that, sizemodechange is dispatched before we finish fullscreen change, and in that event handler, we post a microtask to exit fullscreen, which triggers a nested fullscreen change.

The easiest way to fix it is to use next tick (setTimeout) before synthesizeKey, so that we don't trigger nested fullscreen change.

I spent some time to think about whether it indicates something more serious, but I don't think it does. sizemodechange is an internal event, and thus this issue is not web-exposing. There is always a small chance on GTK that nested fullscreen change can be triggered due to its async nature, but I don't really think that's a big problem (given that the chance is really small).

The problem in test_display_mode_reflow.html is the same, that when sizemodechange is dispatched, the fullscreen change hasn't completed, so it can get a wrong result.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Comment on attachment 8938210 [details]
Bug 1415781 part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press.

::: layout/style/test/chrome/test_display_mode.html:67
(Diff revision 1)
>    shouldNotApply("all and (display-mode: standalone)");
>    shouldNotApply("all and (display-mode: minimal-ui)");
>    // Test entering the OS's fullscreen mode.
>    var fullScreenEntered = waitOneEvent(win, "sizemodechange");
> +  await promiseNextTick();

Could you add a short comment explaining why we need to do the setTimeout(0), since it's not obvious?
Attachment #8938210 - Flags: review?(cam) → review+
Comment on attachment 8938209 [details]
Bug 1415781 part 1 - Fix test_display_mode tests for local run.
Attachment #8938209 - Flags: review?(cam) → review+
Pushed by
part 1 - Fix test_display_mode tests for local run. r=heycam
part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press. r=heycam
Depends on: 1429990
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Duplicate of this bug: 1430379
Duplicate of this bug: 1430380
You need to log in before you can comment on or make changes to this bug.