Closed Bug 1415781 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bevis, Assigned: xidorn)

References

Details

Attachments

(2 files)

This is a follow-up of the try result in bug 1193394 comment 48:
(c3 in linux64-debug-build: https://treeherder.mozilla.org/logviewer.html#?job_id=141558482&repo=try&lineNumber=7845)

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a005fb8aae235b850857ce9954c81f2f6e4ffc41&selectedJob=149019792
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)
Status: NEW → ASSIGNED
Comment on attachment 8938210 [details]
Bug 1415781 part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press.

https://reviewboard.mozilla.org/r/208954/#review216542

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

https://reviewboard.mozilla.org/r/208952/#review216544
Attachment #8938209 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa7d09e322e3
part 1 - Fix test_display_mode tests for local run. r=heycam
https://hg.mozilla.org/integration/autoland/rev/09f0935fcc43
part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press. r=heycam
Depends on: 1429990
https://hg.mozilla.org/mozilla-central/rev/fa7d09e322e3
https://hg.mozilla.org/mozilla-central/rev/09f0935fcc43
Status: ASSIGNED → RESOLVED
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.