Bug 183611 – [GTK] Layout test media/context-menu-actions.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Michael Catanzaro]

Layout test media/context-menu-actions.html is failing since r229466 "webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness". I'll add a Failure expectation.

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/media/context-menu-actions-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/media/context-menu-actions-actual.txt
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: line 122: TypeError: undefined is not an object (evaluating 'this.captionMenu.style')
 Test the various actions available in the HTML5 media element context-menu.
 
 
@@ -21,7 +22,7 @@
 
 EXPECTED (video.webkitDisplayingFullscreen == 'false') OK
 Toggling fullscreen state
-EXPECTED (video.webkitDisplayingFullscreen == 'true') OK
+EXPECTED (video.webkitDisplayingFullscreen == 'true'), OBSERVED 'false' FAIL
 
 END OF TEST

[Jer Noble]

This test assumes that the webkitRequestFullscreen() implementation is entirely synchronous, which is most definitely isn't.  Each of these tests (or at least, each that has an associated event) should be waiting on that event, and not assuming that the results will change within the current run-loop.

[Michael Catanzaro]

I'll remove the REGRESSION tag then. It doesn't seem helpful here, as the behavior change was intended.

[Enrique Ocaña]

*** Bug 121742 has been marked as a duplicate of this bug. ***

[Enrique Ocaña]

Created attachment 402005 [details]
Patch

[Enrique Ocaña]

Created attachment 402012 [details]
Patch

[Michael Catanzaro]

Comment on attachment 402012 [details]
Patch

Problem is that 50ms delays are (a) racy, and (b) going to add up when running 60,000 layout tests all at once. If there's no way to check whether the window has been fullscreened, then this is the best we can do. But I bet there is a way to check? :)

[Xabier Rodríguez Calvar]

Comment on attachment 402012 [details]
Patch

I must agree with Michael here

[Enrique Ocaña]

(In reply to Michael Catanzaro from comment #6)

> Problem is that 50ms delays are (a) racy, and (b) going to add up when
> running 60,000 layout tests all at once. If there's no way to check whether
> the window has been fullscreened, then this is the best we can do. But I bet
> there is a way to check? :)

I understand, but the ChangeLog explains why the "proper" way to check if the window has been fullscreened (onwebkitfullscreenchange) isn't working:

Simply setting an event handler on video.onwebkitfullscreenchange wouldn't have been enough, since the event is triggered before the fullscreen operation has been completed and checking for video.webkitDisplayingFullscreen == true would still fail by then.

The extra 50ms delay was the only solution I found. I tried to set it to the lowest possible value that doesn't trigger errors on my local system (25ms was too low, 100ms seemed a waste).

[Michael Catanzaro]

OK, in that case, maybe a hardcoded delay is unavoidable.

Maybe "loop" the timeout say, 20 times, by reattaching the timeout if it's not fullscreen yet? That way we can reduce the race window and only fail the test if it's still not fullscreen after one second, without slowing down the test by more than 50ms more than required. I'm sure 50ms is enough for a typical laptop or desktop, but imagine the test is running in a container on a VM with limited resources on a host system that's doing 200 other things at the same time... or imagine the test passes when the system is doing 100 other things at the same time rather than 200... won't be fun to debug that.

[Enrique Ocaña]

(In reply to Michael Catanzaro from comment #9)

> Maybe "loop" the timeout say, 20 times, by reattaching the timeout if it's
> not fullscreen yet? That way we can reduce the race window and only fail the
> test if it's still not fullscreen after one second

That's a good suggestion, thanks!

[Xabier Rodríguez Calvar]

Comment on attachment 402012 [details]
Patch

There seems to be a beginfullscreen event. I think we should use this instead of just a timeout

[Enrique Ocaña]

(In reply to Xabier Rodríguez Calvar from comment #11)

> There seems to be a beginfullscreen event. I think we should use this
> instead of just a timeout

Neither that webkitbeginfullscreen event[1] nor its couterpart webkitendfullscreenevent is ever triggered when using FULLSCREEN_API[2], so I don't think they're an option unless we change (and potentially break) the HTMLMediaElement code.

I suspect that some sort of wait is needed after all, since that's how other tests deal with fullscreen changes. For instance, helper functions in media-fullscreen.js call testExpectedEventually()[3][4][5], which is implemented as a sleep-and-retry loop[6].

[1] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLMediaElement.cpp#L5974
[2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLMediaElement.cpp#L5953
[3] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L138
[4] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L36
[5] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/media-fullscreen.js#L43
[6] https://github.com/WebKit/webkit/blob/master/LayoutTests/media/video-test.js#L119

[Xabier Rodríguez Calvar]

(In reply to Enrique Ocaña from comment #12)
> Neither that webkitbeginfullscreen event[1] nor its couterpart
> webkitendfullscreenevent is ever triggered when using FULLSCREEN_API[2], so
> I don't think they're an option unless we change (and potentially break) the
> HTMLMediaElement code.

Sigh. Go for the loop Michael suggested then.

[Enrique Ocaña]

Created attachment 402211 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 402211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402211&action=review

r+ but you need to fix the changelog.

Then I have a couple of questions. First is if this patch breaks any other tests and need a a rebase line. The seconds would be why is the test still marked as crash.

> LayoutTests/ChangeLog:19
> +        would be lost) in case we wait forever for the comparison to evaluato to the expected value.

evaluate

[Enrique Ocaña]

(In reply to Xabier Rodríguez Calvar from comment #15)

> Then I have a couple of questions. First is if this patch breaks any other tests and need a a rebase line.

Indeed, some tests appear to be broken. I'll need to have a look.

> The seconds would be why is the test still marked as crash.

The crash issue belongs to bug 198830 and seems to be a GLX related problem out of the scope of bug 183611. I wanted to focus on one thing at a time instead of tackling two unrelated problems.

[Enrique Ocaña]

Created attachment 403297 [details]
Patch

[EWS]

Committed r263835: <https://trac.webkit.org/changeset/263835>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403297 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65023862>

[Philippe Normand]

Comment on attachment 403297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review

> LayoutTests/platform/gtk/TestExpectations:3964
> -webkit.org/b/183611 webkit.org/b/198830 media/context-menu-actions.html [ Failure Crash ]
> +webkit.org/b/198830 media/context-menu-actions.html [ Crash ]

So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now?

[Enrique Ocaña]

Comment on attachment 403297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review

>> LayoutTests/platform/gtk/TestExpectations:3964
>> +webkit.org/b/198830 media/context-menu-actions.html [ Crash ]
> 
> So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now?

I fixed the flakiness (the test no longer fails), but the crash still can happen sometimes (not always, we could call it a flaky crash) and is tracked on bug 198830, out of the scope of this bug.

[Philippe Normand]

Comment on attachment 403297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403297&action=review

>>> LayoutTests/platform/gtk/TestExpectations:3964
>>> +webkit.org/b/198830 media/context-menu-actions.html [ Crash ]
>> 
>> So now this test is no longer flaky on GTK? Was this an intended change? Is this expected to always crash now?
> 
> I fixed the flakiness (the test no longer fails), but the crash still can happen sometimes (not always, we could call it a flaky crash) and is tracked on bug 198830, out of the scope of this bug.

OK so the new expectation should be [ Pass Crash ] I think...

[Michael Catanzaro]

(In reply to Philippe Normand from comment #22)
> OK so the new expectation should be [ Pass Crash ] I think...

Yup.

[Enrique Ocaña]

For reference, I've corrected the TestExpectations line in bug 213877.