1464908 - test_fullscreen-api-race.html doesn't wait for MozAfterPaint on new windows before triggering fullscreen

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There's a race condition in test_fullscreen-api-race.html that I can reproduce with my attempts to land bug 1452845. What's happening is that the openNewWindow tests open a new window and then wait for focus on the window, and then request fullscreen on the root element.

However, the machinery for requesting fullscreen in the content process involves sending a message at [1] which is received at [2]. But the registration for the listener at [2] only happens after a MozAfterPaint on the window (chain of calls from [3], [4], [5]).

So I think the test should also be waiting for a MozAfterPaint before it requests fullscreen, otherwise there's no guarantee that the fullscreen request will actually work.

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/tab-content.js#428
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/browser-fullScreenAndPointerLock.js#397
[3] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/browser-fullScreenAndPointerLock.js#250,268
[4] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/browser.js#1530
[5] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/browser.js#1390

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f214b7f4e6c6d38bf4f552ca926e159237465c48. The patch fixes it for me locally so I'll put it up for review while awaiting the try results.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1464908 - Wait for MozAfterPaint on new windows before requesting fullscreen.

New windows cannot execute fullscreen requests until after the first
MozAfterPaint event has been fired on the window, because some of the
machinery in browser-fullScreenAndPointerLock.js isn't initialized until
that point. This test exercises that behaviour, and therefore should also
wait until the first MozAfterPaint before requesting fullscreen.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8981260 [details]
Bug 1464908 - Wait for MozAfterPaint on new windows before requesting fullscreen.

https://reviewboard.mozilla.org/r/247348/#review253398

I would suggest that we rewrite this test in async/await which should make it clearer. But this patch is fine.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

The try push shows that that are are still other problems, but at least the original problem I was seeing (where -api-race.html times out) isn't there any more. Here's a try push with just this patch on m-c, and that's totally green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c9b7ba92832a2fa8bccb2042c3d812d88ef777e, so I think it's good for landing.

I agree that rewriting the test as async/await would make it a *lot* clearer. If I have to fiddle with this test more I might do that.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2d1728f92ad
Wait for MozAfterPaint on new windows before requesting fullscreen. r=xidorn

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b2d1728f92ad