Browse Source

LibWeb: Stop the video decoder thread when the video element is GC'd

Otherwise, the thread will continue to run and access the media data
buffer, which will have been freed.

The test here is a bit strange, but the issue would only consistently
repro after several GC runs.
Timothy Flynn 1 year ago
parent
commit
f6407276f7

+ 1 - 0
Tests/LibWeb/Text/data/video-gc-frame.html

@@ -0,0 +1 @@
+<video autoplay src="../../../../Base/home/anon/Videos/test-webm.webm"></video>

+ 1 - 0
Tests/LibWeb/Text/expected/video-gc.txt

@@ -0,0 +1 @@
+   PASS (didn't crash)

+ 25 - 0
Tests/LibWeb/Text/input/video-gc.html

@@ -0,0 +1,25 @@
+<iframe id=iframe></iframe>
+<script src="include.js"></script>
+<script>
+    function navigateIframe(src) {
+        return new Promise(resolve => {
+            iframe.addEventListener("load", () => {
+                resolve();
+            });
+            iframe.src = src;
+        });
+    }
+
+    asyncTest(async done => {
+        await navigateIframe("../data/video-gc-frame.html");
+        await navigateIframe("../data/iframe-test-content-1.html");
+        iframe.remove();
+
+        for (let i = 0; i < 5; ++i) {
+            internals.gc();
+        }
+
+        println("PASS (didn't crash)");
+        done();
+    });
+</script>

+ 9 - 0
Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp

@@ -15,6 +15,7 @@
 #include <LibWeb/Fetch/Infrastructure/HTTP/Responses.h>
 #include <LibWeb/Fetch/Infrastructure/HTTP/Responses.h>
 #include <LibWeb/HTML/HTMLVideoElement.h>
 #include <LibWeb/HTML/HTMLVideoElement.h>
 #include <LibWeb/HTML/VideoTrack.h>
 #include <LibWeb/HTML/VideoTrack.h>
+#include <LibWeb/HTML/VideoTrackList.h>
 #include <LibWeb/Layout/VideoBox.h>
 #include <LibWeb/Layout/VideoBox.h>
 #include <LibWeb/Painting/Paintable.h>
 #include <LibWeb/Painting/Paintable.h>
 #include <LibWeb/Platform/ImageCodecPlugin.h>
 #include <LibWeb/Platform/ImageCodecPlugin.h>
@@ -36,6 +37,14 @@ void HTMLVideoElement::initialize(JS::Realm& realm)
     WEB_SET_PROTOTYPE_FOR_INTERFACE(HTMLVideoElement);
     WEB_SET_PROTOTYPE_FOR_INTERFACE(HTMLVideoElement);
 }
 }
 
 
+void HTMLVideoElement::finalize()
+{
+    Base::finalize();
+
+    for (auto video_track : video_tracks()->video_tracks())
+        video_track->stop_video({});
+}
+
 void HTMLVideoElement::visit_edges(Cell::Visitor& visitor)
 void HTMLVideoElement::visit_edges(Cell::Visitor& visitor)
 {
 {
     Base::visit_edges(visitor);
     Base::visit_edges(visitor);

+ 1 - 0
Userland/Libraries/LibWeb/HTML/HTMLVideoElement.h

@@ -46,6 +46,7 @@ private:
     HTMLVideoElement(DOM::Document&, DOM::QualifiedName);
     HTMLVideoElement(DOM::Document&, DOM::QualifiedName);
 
 
     virtual void initialize(JS::Realm&) override;
     virtual void initialize(JS::Realm&) override;
+    virtual void finalize() override;
     virtual void visit_edges(Cell::Visitor&) override;
     virtual void visit_edges(Cell::Visitor&) override;
 
 
     virtual void attribute_changed(FlyString const& name, Optional<String> const& value) override;
     virtual void attribute_changed(FlyString const& name, Optional<String> const& value) override;

+ 6 - 1
Userland/Libraries/LibWeb/HTML/VideoTrack.cpp

@@ -98,6 +98,11 @@ void VideoTrack::pause_video(Badge<HTMLVideoElement>)
     m_playback_manager->pause_playback();
     m_playback_manager->pause_playback();
 }
 }
 
 
+void VideoTrack::stop_video(Badge<HTMLVideoElement>)
+{
+    m_playback_manager->terminate_playback();
+}
+
 Duration VideoTrack::position() const
 Duration VideoTrack::position() const
 {
 {
     return m_playback_manager->current_playback_time();
     return m_playback_manager->current_playback_time();
@@ -141,7 +146,7 @@ void VideoTrack::set_selected(bool selected)
     // no longer in a VideoTrackList object, then the track being selected or unselected has no effect beyond changing the value of
     // no longer in a VideoTrackList object, then the track being selected or unselected has no effect beyond changing the value of
     // the attribute on the VideoTrack object.)
     // the attribute on the VideoTrack object.)
     if (m_video_track_list) {
     if (m_video_track_list) {
-        for (auto video_track : m_video_track_list->video_tracks({})) {
+        for (auto video_track : m_video_track_list->video_tracks()) {
             if (video_track.ptr() != this)
             if (video_track.ptr() != this)
                 video_track->m_selected = false;
                 video_track->m_selected = false;
         }
         }

+ 1 - 0
Userland/Libraries/LibWeb/HTML/VideoTrack.h

@@ -25,6 +25,7 @@ public:
 
 
     void play_video(Badge<HTMLVideoElement>);
     void play_video(Badge<HTMLVideoElement>);
     void pause_video(Badge<HTMLVideoElement>);
     void pause_video(Badge<HTMLVideoElement>);
+    void stop_video(Badge<HTMLVideoElement>);
 
 
     Duration position() const;
     Duration position() const;
     Duration duration() const;
     Duration duration() const;

+ 1 - 1
Userland/Libraries/LibWeb/HTML/VideoTrackList.h

@@ -22,7 +22,7 @@ public:
     ErrorOr<void> add_track(Badge<HTMLMediaElement>, JS::NonnullGCPtr<VideoTrack>);
     ErrorOr<void> add_track(Badge<HTMLMediaElement>, JS::NonnullGCPtr<VideoTrack>);
     void remove_all_tracks(Badge<HTMLMediaElement>);
     void remove_all_tracks(Badge<HTMLMediaElement>);
 
 
-    Span<JS::NonnullGCPtr<VideoTrack>> video_tracks(Badge<VideoTrack>) { return m_video_tracks; }
+    Span<JS::NonnullGCPtr<VideoTrack>> video_tracks() { return m_video_tracks; }
 
 
     // https://html.spec.whatwg.org/multipage/media.html#dom-videotracklist-length
     // https://html.spec.whatwg.org/multipage/media.html#dom-videotracklist-length
     size_t length() const { return m_video_tracks.size(); }
     size_t length() const { return m_video_tracks.size(); }