Safari: audio elements with event listeners are not getting garbage c…

…ollected https://bugs.webkit.org/show_bug.cgi?id=262485 rdar://116347723 Reviewed by Youenn Fablet. Clarify the implementation of HTMLMediaElement::virtualHasPendingActivity() to explicitly handle the cases when an element may have eventListeners but will never fire an event without its state being mutated through JavaScript. * LayoutTests/media/media-garbage-collection-expected.txt: Added. * LayoutTests/media/media-garbage-collection.html: Added. * LayoutTests/media/video-test.js: * Source/WebCore/html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::virtualHasPendingActivity const): (WebCore::HTMLMediaElement::hasLiveSource const): Deleted. * Source/WebCore/html/HTMLMediaElement.h: Canonical link: https://commits.webkit.org/269165@main
WebKit · Oct 10, 2023 · a2ddc40 · a2ddc40
1 parent ca84d41
commit a2ddc40
Show file tree

Hide file tree

Showing 7 changed files with 304 additions and 11 deletions.
diff --git a/LayoutTests/media/media-garbage-collection-expected.txt b/LayoutTests/media/media-garbage-collection-expected.txt
@@ -0,0 +1,44 @@
+Test 1:
+Test that a never loaded media element can be collected
+EXPECTED (weakVideo.wasCollected == 'true') OK
+
+Test 2:
+Test that the media element is not collected between load() and "loadstart"
+EVENT(loadstart)
+EXPECTED (weakVideo.wasCollected == 'false') OK
+
+Test 3:
+Test that the media element is not collected during playback
+EVENT(playing)
+EVENT(timeupdate)
+EXPECTED (weakVideo.wasCollected == 'false') OK
+
+Test 4:
+Test that a paused media element will be collected, even if it has an event listener
+EVENT(suspend)
+EXPECTED (weakVideo.wasCollected == 'true') OK
+
+Test 5:
+Test that an ended media element will be collected, even if it has an event listener
+EVENT(canplay)
+EVENT(seeked)
+EVENT(ended)
+EXPECTED (weakVideo.wasCollected == 'true') OK
+
+Test 6:
+Test that an interrupted media element will not be collected
+EVENT(canplay)
+EVENT(timeupdate)
+RUN(internals.beginMediaSessionInterruption("System"))
+EVENT(pause)
+EXPECTED (weakVideo.wasCollected == 'false') OK
+RUN(internals.endMediaSessionInterruption("System"))
+
+Test 7:
+Test that a media element will be collected after it is unloaded
+EVENT(canplay)
+EVENT(emptied)
+EXPECTED (weakVideo.wasCollected == 'true') OK
+
+END OF TEST
+
diff --git a/LayoutTests/media/media-garbage-collection.html b/LayoutTests/media/media-garbage-collection.html
@@ -0,0 +1,183 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1">
+    <title>media-garbage-collection</title>
+    <script src="video-test.js"></script>
+    <script src="media-file.js"></script>
+    <script src=../resources/gc.js></script>
+    <script>
+        class WeakVideoRef {
+            #weakRef;
+
+            constructor(video) {
+                this.#weakRef = new WeakRef(video);
+            }
+
+            get wasCollected() { return !this.#weakRef.deref(); }
+            get video() { return this.#weakRef.deref() }
+
+            waitFor(event) { return waitFor(this.video, event); }
+            addEventListener(event, callback) { return this.video.addEventListener(event, callback); } 
+            unload() {
+                if (this.wasCollected)
+                    return;
+                this.video.pause();
+                this.video.src = '';
+                this.video.load();
+            }
+        };
+        var weakVideo;
+
+        function testExpectedEventuallyWhileGCing(testFuncString, expected, comparison) {
+            return testExpectedEventuallyWhileRunningBetweenTests(testFuncString, expected, comparison, 1000, gc);
+        }
+
+        let tests = [
+            async () => {
+                consoleWrite('Test that a never loaded media element can be collected');
+                weakVideo = new WeakVideoRef(document.createElement('video'));
+                await sleepFor(1);
+
+                gc();
+
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', true, '==');
+            },
+            async () => {
+                consoleWrite('Test that the media element is not collected between load() and "loadstart"')
+                var video = document.createElement("video");
+                video.src = "null";
+                video.load();
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                let loadstartPromise = weakVideo.waitFor('loadstart');
+                await sleepFor(1);
+
+                gc();
+
+                video = (await loadstartPromise).target;
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', false, '==');
+            },
+            async () => {
+                consoleWrite('Test that the media element is not collected during playback')
+                var video = document.createElement("video");
+                video.src = findMediaFile('video', 'content/test');
+                video.muted = true;
+                video.play();
+                await waitFor(video, 'playing');
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                let timeupdatePromise = weakVideo.waitFor('timeupdate');
+                await sleepFor(1);
+
+                gc();
+
+                video = (await timeupdatePromise).target;
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', false, '==');
+            },
+            async () => {
+                consoleWrite('Test that a paused media element will be collected, even if it has an event listener')
+
+                video = document.createElement("video");
+                video.src = findMediaFile('video', 'content/test');
+                video.muted = true;
+                await waitFor(video, 'suspend'),
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                weakVideo.addEventListener('ended', () => { });
+                await sleepFor(1);
+
+                gc();
+
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', true, '==');
+            },
+            async () => {
+                consoleWrite('Test that an ended media element will be collected, even if it has an event listener')
+
+                video = document.createElement("video");
+                video.src = findMediaFile('video', 'content/test');
+                await waitFor(video, 'canplay');
+                video.currentTime = video.duration - 0.1;
+                await waitFor(video, 'seeked');
+                video.play();
+                await waitFor(video, 'ended');
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                weakVideo.addEventListener('playing', () => { });
+                await sleepFor(1);
+
+                gc();
+
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', true, '==');
+            },
+            async () => {
+                consoleWrite('Test that an interrupted media element will not be collected')
+
+                video = document.createElement("video");
+                video.src = findMediaFile('video', 'content/test');
+                await waitFor(video, 'canplay');
+                video.play();
+                await waitFor(video, 'timeupdate');
+
+                run('internals.beginMediaSessionInterruption("System")');
+                await waitFor(video, 'pause')
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                weakVideo.addEventListener('playing', () => { });
+                await sleepFor(1);
+
+                gc();
+
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', false, '==');
+                run('internals.endMediaSessionInterruption("System")');
+            },
+            async () => {
+                consoleWrite('Test that a media element will be collected after it is unloaded')
+
+                video = document.createElement("video");
+                video.src = findMediaFile('video', 'content/test');
+                await waitFor(video, 'canplay');
+                video.src = '';
+                video.load();
+                await waitFor(video, 'emptied');
+
+                weakVideo = new WeakVideoRef(video);
+                video = null;
+
+                weakVideo.addEventListener('playing', () => { });
+                await sleepFor(1);
+
+                gc();
+
+                await testExpectedEventuallyWhileGCing('weakVideo.wasCollected', true, '==');
+            },
+        ];
+
+        async function runTests() {
+            let index = 0;
+            for (test of tests) {
+                consoleWrite(`Test ${++index}:`)
+                try {
+                    await test();
+                    video = null;
+                    weakVideo.unload();
+                    weakVideo = null;
+                } catch(e) { logResult(Failed, e); }
+                consoleWrite('')
+            };
+        }
+
+        window.addEventListener('load', event => {
+            runTests().then(endTest).catch(failTest);
+        })
+    </script>
+</head>
+<body>
+</body>
+</html>
diff --git a/LayoutTests/media/video-test.js b/LayoutTests/media/video-test.js
@@ -113,6 +113,11 @@ function sleepFor(duration) {
 }
 
 function testExpectedEventually(testFuncString, expected, comparison, timeout)
+{
+    return testExpectedEventuallyWhileRunningBetweenTests(testFuncString, expected, comparison, timeout, null);
+}
+
+function testExpectedEventuallyWhileRunningBetweenTests(testFuncString, expected, comparison, timeout, work)
 {
     return new Promise(async resolve => {
         var success;
@@ -122,14 +127,16 @@ function testExpectedEventually(testFuncString, expected, comparison, timeout)
             comparison = '==';
         while (timeout === undefined || timeSlept < timeout) {
             try {
-                let {success, observed} = compare(testFuncString, expected, comparison);
+                ({success, observed} = compare(testFuncString, expected, comparison));
                 if (success) {
                     reportExpected(success, testFuncString, comparison, expected, observed);
                     resolve();
                     return;
                 }
                 await sleepFor(1);
                 timeSlept++;
+                if (work)
+                    work();
             } catch (ex) {
                 consoleWrite(ex);
                 resolve();

diff --git a/LayoutTests/platform/glib/TestExpectations b/LayoutTests/platform/glib/TestExpectations
@@ -3535,6 +3535,9 @@ imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/restri
 imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/restrict-properties/iframe-popup.https.html?1-2 [ Slow ]
 imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/restrict-properties/iframe-popup.https.html?3-4 [ Slow ]
 
+// Flakily crashing on GTK and WPE bots
+webkit.org/b/262949 media/media-garbage-collection.html [ Pass Crash ]
+
 # End: Common failures between GTK and WPE.
 
 #////////////////////////////////////////////////////////////////////////////////////////

diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
@@ -6385,17 +6385,71 @@ void HTMLMediaElement::resume()
     updateRenderer();
 }
 
-bool HTMLMediaElement::hasLiveSource() const
-{
-    // FIXME: Handle the case of an ended media stream as srcObject.
-    return m_player && m_player->hasMediaEngine() && (!ended() || seeking() || m_networkState >= NETWORK_IDLE);
-}
-
 bool HTMLMediaElement::virtualHasPendingActivity() const
 {
-    return m_controlsState == ControlsState::Initializing
-        || (hasAudio() && isPlaying())
-        || (hasLiveSource() && hasEventListeners());
+    // NOTE: This method will be called from a non-main thread.
+
+    // A media element has pending activity if:
+    // * It is initializing its media controls
+    if (m_controlsState == ControlsState::Initializing)
+        return true;
+
+    // A paused media element may become a playing media element
+    // if it was paused due to an interruption:
+    bool isPlayingOrPossbilyCouldPlay = [&] {
+        if (isPlaying())
+            return true;
+
+        auto* mediaSession = this->mediaSessionIfExists();
+        if (!mediaSession)
+            return false;
+
+        if (mediaSession->state() != PlatformMediaSession::Interrupted)
+            return false;
+
+        auto stateToRestore = mediaSession->stateToRestore();
+        return stateToRestore == PlatformMediaSession::Autoplaying
+            || stateToRestore == PlatformMediaSession::Playing;
+    }();
+
+    // * It is playing, and is audible to the user:
+    if (isPlayingOrPossbilyCouldPlay && canProduceAudio())
+        return true;
+
+    // If a media element is not directly observable by the user, it cannot
+    // have pending activity if it does not have event listeners:
+    if (!hasEventListeners())
+        return false;
+
+    // A media element has pending activity if it has event listeners and:
+    // * The load algorithm is pending, and will thus fire "loadstart" events:
+    if (m_resourceSelectionTaskCancellationGroup.hasPendingTask())
+        return true;
+
+    // * It has a media engine and:
+    if (m_player && m_player->hasMediaEngine()) {
+        // * It is playing, and will thus fire "timeupdate" and "ended" events:
+        if (isPlayingOrPossbilyCouldPlay)
+            return true;
+
+        // * It is seeking, and will thus fire "seeked" events:
+        if (seeking())
+            return true;
+
+        // * It is loading, and will thus fire "progress" or "stalled" events:
+        if (m_networkState == NETWORK_LOADING)
+            return true;
+
+#if ENABLE(MEDIA_STREAM)
+        // * It has a live MediaStream object:
+        if (m_mediaStreamSrcObject)
+            return true;
+#endif
+    }
+
+    // Otherwise, the media element will fire no events at event listeners, and
+    // thus does not have observable pending activity.
+    return false;
 }
 
 void HTMLMediaElement::mediaVolumeDidChange()

diff --git a/Source/WebCore/html/HTMLMediaElement.h b/Source/WebCore/html/HTMLMediaElement.h
@@ -529,6 +529,7 @@ class HTMLMediaElement
     RefPtr<VideoPlaybackQuality> getVideoPlaybackQuality();
 
     MediaPlayer::Preload preloadValue() const { return m_preload; }
+    MediaElementSession* mediaSessionIfExists() const { return m_mediaSession.get(); }
     WEBCORE_EXPORT MediaElementSession& mediaSession() const;
 
     void pageScaleFactorChanged();
@@ -1019,7 +1020,6 @@ class HTMLMediaElement
     void setInActiveDocument(bool);
 
     void checkForAudioAndVideo();
-    bool hasLiveSource() const;
 
     void playPlayer();
     void pausePlayer();

diff --git a/Source/WebCore/platform/audio/PlatformMediaSession.h b/Source/WebCore/platform/audio/PlatformMediaSession.h
@@ -108,6 +108,8 @@ class PlatformMediaSession
     State state() const { return m_state; }
     void setState(State);
 
+    State stateToRestore() const { return m_stateToRestore; }
+
     enum InterruptionType : uint8_t {
         NoInterruption,
         SystemSleep,