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

Status: RESOLVED FIXED

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

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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]

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

ni? myself for the fullscreen crash.

I don't have a Linux environment at the moment, though :/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Ah it seems the log in comment 0 has expired...

Bevis, could you do a new push for the fullscreen failure?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I wasn't going to redirect my own ni? though...

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

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)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1415781 part 1 - Fix test_display_mode tests for local run.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1415781 part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press.

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

Comment 9

[Cameron McCormack (:heycam)]

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?

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8938209 [details]
Bug 1415781 part 1 - Fix test_display_mode tests for local run.

https://reviewboard.mozilla.org/r/208952/#review216544

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8938209 [details]
Bug 1415781 part 1 - Fix test_display_mode tests for local run.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208952/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8938210 [details]
Bug 1415781 part 2 - Make test_display_mode tests wait for the next tick before synthesizing F11 key press.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208954/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fa7d09e322e3
https://hg.mozilla.org/mozilla-central/rev/09f0935fcc43