From f6407276f7f707564492db790589e26b70164e2c Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Thu, 25 Apr 2024 08:39:19 -0400 Subject: [PATCH] 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. --- Tests/LibWeb/Text/data/video-gc-frame.html | 1 + Tests/LibWeb/Text/expected/video-gc.txt | 1 + Tests/LibWeb/Text/input/video-gc.html | 25 +++++++++++++++++++ .../LibWeb/HTML/HTMLVideoElement.cpp | 9 +++++++ .../Libraries/LibWeb/HTML/HTMLVideoElement.h | 1 + Userland/Libraries/LibWeb/HTML/VideoTrack.cpp | 7 +++++- Userland/Libraries/LibWeb/HTML/VideoTrack.h | 1 + .../Libraries/LibWeb/HTML/VideoTrackList.h | 2 +- 8 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 Tests/LibWeb/Text/data/video-gc-frame.html create mode 100644 Tests/LibWeb/Text/expected/video-gc.txt create mode 100644 Tests/LibWeb/Text/input/video-gc.html diff --git a/Tests/LibWeb/Text/data/video-gc-frame.html b/Tests/LibWeb/Text/data/video-gc-frame.html new file mode 100644 index 00000000000..9f7a69a30ac --- /dev/null +++ b/Tests/LibWeb/Text/data/video-gc-frame.html @@ -0,0 +1 @@ + diff --git a/Tests/LibWeb/Text/expected/video-gc.txt b/Tests/LibWeb/Text/expected/video-gc.txt new file mode 100644 index 00000000000..39701378c55 --- /dev/null +++ b/Tests/LibWeb/Text/expected/video-gc.txt @@ -0,0 +1 @@ + PASS (didn't crash) diff --git a/Tests/LibWeb/Text/input/video-gc.html b/Tests/LibWeb/Text/input/video-gc.html new file mode 100644 index 00000000000..a456996e4c6 --- /dev/null +++ b/Tests/LibWeb/Text/input/video-gc.html @@ -0,0 +1,25 @@ + + + diff --git a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp index ae3d4da3cb7..414499dbc31 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,14 @@ void HTMLVideoElement::initialize(JS::Realm& realm) 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) { Base::visit_edges(visitor); diff --git a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.h b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.h index cf55a6f7bc4..720a78e0a04 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.h +++ b/Userland/Libraries/LibWeb/HTML/HTMLVideoElement.h @@ -46,6 +46,7 @@ private: HTMLVideoElement(DOM::Document&, DOM::QualifiedName); virtual void initialize(JS::Realm&) override; + virtual void finalize() override; virtual void visit_edges(Cell::Visitor&) override; virtual void attribute_changed(FlyString const& name, Optional const& value) override; diff --git a/Userland/Libraries/LibWeb/HTML/VideoTrack.cpp b/Userland/Libraries/LibWeb/HTML/VideoTrack.cpp index d6f598f53a0..3ca06c52858 100644 --- a/Userland/Libraries/LibWeb/HTML/VideoTrack.cpp +++ b/Userland/Libraries/LibWeb/HTML/VideoTrack.cpp @@ -98,6 +98,11 @@ void VideoTrack::pause_video(Badge) m_playback_manager->pause_playback(); } +void VideoTrack::stop_video(Badge) +{ + m_playback_manager->terminate_playback(); +} + Duration VideoTrack::position() const { 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 // the attribute on the VideoTrack object.) 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) video_track->m_selected = false; } diff --git a/Userland/Libraries/LibWeb/HTML/VideoTrack.h b/Userland/Libraries/LibWeb/HTML/VideoTrack.h index ad6f48ab261..5efbe86f625 100644 --- a/Userland/Libraries/LibWeb/HTML/VideoTrack.h +++ b/Userland/Libraries/LibWeb/HTML/VideoTrack.h @@ -25,6 +25,7 @@ public: void play_video(Badge); void pause_video(Badge); + void stop_video(Badge); Duration position() const; Duration duration() const; diff --git a/Userland/Libraries/LibWeb/HTML/VideoTrackList.h b/Userland/Libraries/LibWeb/HTML/VideoTrackList.h index 607d6e0d76a..dab77300643 100644 --- a/Userland/Libraries/LibWeb/HTML/VideoTrackList.h +++ b/Userland/Libraries/LibWeb/HTML/VideoTrackList.h @@ -22,7 +22,7 @@ public: ErrorOr add_track(Badge, JS::NonnullGCPtr); void remove_all_tracks(Badge); - Span> video_tracks(Badge) { return m_video_tracks; } + Span> video_tracks() { return m_video_tracks; } // https://html.spec.whatwg.org/multipage/media.html#dom-videotracklist-length size_t length() const { return m_video_tracks.size(); }