From 9586a7500c3e549c44ae0f3174d80da4b0bd2162 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 14 Mar 2025 09:19:03 -0400 Subject: [PATCH] LibWeb: Protect audio codec callbacks against its own destruction The plugin may go out of scope before the callbacks we've queued have fired. It is undefined behavior for these callbacks to access the plugin data after that has occurred. This patch protects these callbacks with weak pointers. Note that the plugin is not ref-counted, so we cannot use `strong_ref` here. --- Libraries/LibWeb/Platform/AudioCodecPlugin.h | 3 +- .../Platform/AudioCodecPluginAgnostic.cpp | 70 ++++++++++++------- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/Libraries/LibWeb/Platform/AudioCodecPlugin.h b/Libraries/LibWeb/Platform/AudioCodecPlugin.h index 23f44942bb2..85a2a31a5f5 100644 --- a/Libraries/LibWeb/Platform/AudioCodecPlugin.h +++ b/Libraries/LibWeb/Platform/AudioCodecPlugin.h @@ -10,11 +10,12 @@ #include #include #include +#include #include namespace Web::Platform { -class AudioCodecPlugin { +class AudioCodecPlugin : public Weakable { public: using AudioCodecPluginCreator = Function>(NonnullRefPtr)>; diff --git a/Libraries/LibWeb/Platform/AudioCodecPluginAgnostic.cpp b/Libraries/LibWeb/Platform/AudioCodecPluginAgnostic.cpp index 7c454e2b0c1..7845425c38a 100644 --- a/Libraries/LibWeb/Platform/AudioCodecPluginAgnostic.cpp +++ b/Libraries/LibWeb/Platform/AudioCodecPluginAgnostic.cpp @@ -87,8 +87,9 @@ AudioCodecPluginAgnostic::AudioCodecPluginAgnostic(NonnullRefPtr , m_main_thread_event_loop(Core::EventLoop::current()) , m_update_timer(move(update_timer)) { - m_update_timer->on_timeout = [this]() { - update_timestamp(); + m_update_timer->on_timeout = [self = make_weak_ptr()]() { + if (self) + self->update_timestamp(); }; } @@ -96,10 +97,16 @@ void AudioCodecPluginAgnostic::resume_playback() { m_paused = false; m_output->resume() - ->when_resolved([this](AK::Duration new_device_time) { - m_main_thread_event_loop.deferred_invoke([this, new_device_time]() { - m_last_resume_in_device_time = new_device_time; - m_update_timer->start(); + ->when_resolved([self = make_weak_ptr()](AK::Duration new_device_time) { + if (!self) + return; + + self->m_main_thread_event_loop.deferred_invoke([self, new_device_time]() { + if (!self) + return; + + self->m_last_resume_in_device_time = new_device_time; + self->m_update_timer->start(); }); }) .when_rejected([](Error&&) { @@ -111,15 +118,23 @@ void AudioCodecPluginAgnostic::pause_playback() { m_paused = true; m_output->drain_buffer_and_suspend() - ->when_resolved([this]() -> ErrorOr { - auto new_media_time = timestamp_from_samples(m_loader->loaded_samples(), m_loader->sample_rate()); - auto new_device_time = TRY(m_output->total_time_played()); - m_main_thread_event_loop.deferred_invoke([this, new_media_time, new_device_time]() { - m_last_resume_in_media_time = new_media_time; - m_last_resume_in_device_time = new_device_time; - m_update_timer->stop(); - update_timestamp(); + ->when_resolved([self = make_weak_ptr()]() -> ErrorOr { + if (!self) + return {}; + + auto new_media_time = timestamp_from_samples(self->m_loader->loaded_samples(), self->m_loader->sample_rate()); + auto new_device_time = TRY(self->m_output->total_time_played()); + + self->m_main_thread_event_loop.deferred_invoke([self, new_media_time, new_device_time]() { + if (!self) + return; + + self->m_last_resume_in_media_time = new_media_time; + self->m_last_resume_in_device_time = new_device_time; + self->m_update_timer->stop(); + self->update_timestamp(); }); + return {}; }) .when_rejected([](Error&&) { @@ -137,22 +152,29 @@ void AudioCodecPluginAgnostic::set_volume(double volume) void AudioCodecPluginAgnostic::seek(double position) { m_output->discard_buffer_and_suspend() - ->when_resolved([this, position, was_paused = m_paused]() -> ErrorOr { - auto sample_position = static_cast(position * m_loader->sample_rate()); - auto seek_result = m_loader->seek(sample_position); + ->when_resolved([self = make_weak_ptr(), position, was_paused = m_paused]() -> ErrorOr { + if (!self) + return {}; + + auto sample_position = static_cast(position * self->m_loader->sample_rate()); + auto seek_result = self->m_loader->seek(sample_position); if (seek_result.is_error()) return Error::from_string_literal("Seeking in audio loader failed"); - auto new_media_time = get_loader_timestamp(m_loader); - auto new_device_time = m_output->total_time_played().release_value_but_fixme_should_propagate_errors(); + auto new_media_time = get_loader_timestamp(self->m_loader); + auto new_device_time = self->m_output->total_time_played().release_value_but_fixme_should_propagate_errors(); + + self->m_main_thread_event_loop.deferred_invoke([self, was_paused, new_device_time, new_media_time]() { + if (!self) + return; + + self->m_last_resume_in_device_time = new_device_time; + self->m_last_resume_in_media_time = new_media_time; - m_main_thread_event_loop.deferred_invoke([this, was_paused, new_device_time, new_media_time]() { - m_last_resume_in_device_time = new_device_time; - m_last_resume_in_media_time = new_media_time; if (was_paused) { - update_timestamp(); + self->update_timestamp(); } else { - m_output->resume()->when_rejected([](Error&&) { + self->m_output->resume()->when_rejected([](Error&&) { // FIXME: Propagate errors. }); }