Bug 223762

Summary: [ macOS wk2 ] media/pip-video-going-into-fullscreen.html is a flakey timeout
Product: WebKit Reporter: Robert Jenner <jenner>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test list used to reproduce crash.
none
Patch
none
Rebase the patch
eric.carlson: review+
Revise the patch based on Eric's comment ews-feeder: commit-queue-

Description Robert Jenner 2021-03-25 13:47:44 PDT
media/pip-video-going-into-fullscreen.html

is a flakey timeout in macOS release and debug for Catalina and BigSur in wk2. 

HISTORY URL:
https://results.webkit.org/?suite=layout-tests&test=media%2Fpip-video-going-into-fullscreen.html

DIFF TEXT:
-Tests pip video going into fullscreen should exit pip
+#PID UNRESPONSIVE - WebKitTestRunner (pid 90418)
+FAIL: Timed out waiting for notifyDone to be called
 
-Going into Picture-in-Picture
-EXPECTED (video.webkitPresentationMode == 'picture-in-picture') OK
-Going into Full Screen from Picture-in-Picture
-EXPECTED (document.webkitCurrentFullScreenElement == '[object HTMLVideoElement]') OK
-EXPECTED (video.webkitPresentationMode == 'fullscreen') OK
-END OF TEST
-
+#EOF
+#EOF
Comment 1 Robert Jenner 2021-03-25 14:56:58 PDT
I was able to reproduce the timeout by generating a test list and running it as the following test:

run-webkit-test --test-list <path to test list> --child-process=1

I have attached the test list I used for reproduction to this bug.
Comment 2 Robert Jenner 2021-03-25 14:57:38 PDT
Created attachment 424283 [details]
Test list used to reproduce crash.

Attaching test list used to reproduce crash. Currently working on narrowing down root cause.
Comment 3 Radar WebKit Bug Importer 2021-03-25 14:57:54 PDT
<rdar://problem/75854906>
Comment 4 Robert Jenner 2021-03-25 14:59:47 PDT
(In reply to Robert Jenner from comment #2)
> Created attachment 424283 [details]
> Test list used to reproduce crash.
> 
> Attaching test list used to reproduce crash. Currently working on narrowing
> down root cause.


And by Crash, I mean Timeout. I was able to reproduce the timeout.
Comment 5 Robert Jenner 2021-03-25 15:23:54 PDT
I was able to bisect it down to a range of revisions, as that was all that was available. 

Running the test on r274796 did not produce the timeout. Running at r274822 did produce the timeout. 

It does look like changes at r274810 do deal with media controls and PIP tests. So that may hav caused the timeouts. 
 
https://trac.webkit.org/changeset/274810/webkit
Comment 6 Robert Jenner 2021-03-29 12:21:00 PDT
Updated test expectations to Pass Timeout while test is being reviewed:

https://trac.webkit.org/changeset/275167/webkit
Comment 7 Peng Liu 2021-03-31 12:38:21 PDT
Created attachment 424802 [details]
Patch
Comment 8 Peng Liu 2021-03-31 12:46:46 PDT
Created attachment 424805 [details]
Rebase the patch
Comment 9 Eric Carlson 2021-03-31 13:00:22 PDT
Comment on attachment 424805 [details]
Rebase the patch

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

> LayoutTests/media/pip-video-going-into-fullscreen.html:44
> +            video.addEventListener('webkitfullscreenchange', onbeginfullscreen);

Nit: you use `video.addEventListener('webkitfullscreenchange', onbeginfullscreen, {once: true})` you won't have to remove the listener below.
Comment 10 Peng Liu 2021-03-31 13:19:54 PDT
Comment on attachment 424805 [details]
Rebase the patch

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

>> LayoutTests/media/pip-video-going-into-fullscreen.html:44
>> +            video.addEventListener('webkitfullscreenchange', onbeginfullscreen);
> 
> Nit: you use `video.addEventListener('webkitfullscreenchange', onbeginfullscreen, {once: true})` you won't have to remove the listener below.

Good idea! Will fix it. Thanks!
Comment 11 Peng Liu 2021-03-31 15:14:16 PDT
Created attachment 424826 [details]
Revise the patch based on Eric's comment
Comment 12 EWS 2021-03-31 17:47:40 PDT
Committed r275328: <https://commits.webkit.org/r275328>

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