From 4399ef090615497888215ca0a04a9b03d4c2afae Mon Sep 17 00:00:00 2001
From: Dale Curtis <[email protected]>
Date: Thu, 08 Oct 2026 11:28:30 -0700
Subject: [PATCH] Avoid synchronous PipelineController destruction in WMPI::Shutdown()

WebMediaPlayerImpl::OnPipelineSuspended() reports memory usage to V8 via
external_memory_accounter_.Update(), which can synchronously trigger a
V8 GC. If the HTMLMediaElement is unreachable, its pre-finalizer runs
ClearMediaPlayer() -> WebMediaPlayerImpl::Shutdown(). Resetting
pipeline_controller_ inside Shutdown() destroys PipelineController while
PipelineController::OnPipelineStatus() is still on the call stack,
causing a use-after-free when OnPipelineStatus() subsequently calls
Dispatch().

pipeline_controller_.reset() was originally added in Shutdown() (in
1be6d5215b1d) because PipelineImpl::RendererWrapper::DestroyRenderer()
did not reset default_renderer_. If the pipeline was stopped before
renderer initialization, default_renderer_ stayed alive until
~RendererWrapper() (triggered by ~PipelineImpl()), outliving
DestructionHelper.

Fix this by resetting default_renderer_ in
RendererWrapper::DestroyRenderer() (and when discarding a mismatched
default_renderer_ in CreateRendererInternal()), and removing
pipeline_controller_.reset() from WebMediaPlayerImpl::Shutdown() so
pipeline_controller_ is destroyed with ~WebMediaPlayerImpl().

Bug: 570638834, 341254296
Change-Id: Iafbebc701219c097f37d9d92e1b497c98e44834c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8524036
Reviewed-by: Frank Liberato <[email protected]>
Commit-Queue: Dale Curtis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1714751}
---
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index 8ea3b48049..2195d4c8 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -739,6 +739,8 @@
   if (!default_renderer_ ||
       (renderer_type &&
        default_renderer_->GetRendererType() != renderer_type.value())) {
+    default_renderer_.reset();
+
     // Create the Renderer asynchronously on the main task runner. Use
     // base::BindPostTaskToCurrentDefault to call OnRendererCreated() on the
     // media task runner.
@@ -1275,6 +1277,8 @@
 void PipelineImpl::RendererWrapper::DestroyRenderer() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(shared_state_.media_sequence_checker);
 
+  default_renderer_.reset();
+
   // Destroy the renderer outside the lock scope to avoid holding the lock
   // while renderer is being destroyed (in case Renderer destructor is costly).
   std::unique_ptr<Renderer> renderer;
diff --git a/third_party/blink/renderer/platform/media/web_media_player_impl.cc b/third_party/blink/renderer/platform/media/web_media_player_impl.cc
index 96a73ad..643e67a 100644
--- a/third_party/blink/renderer/platform/media/web_media_player_impl.cc
+++ b/third_party/blink/renderer/platform/media/web_media_player_impl.cc
@@ -718,13 +718,6 @@
   if (observer_)
     observer_->SetClient(nullptr);
 
-  // Explicitly reset `pipeline_controller_` to guarantee its destruction
-  // before DestructionHelper runs on `media_task_runner_`.
-  // This prevents possible dangling ptr's if `compositor` is destroyed
-  // before `pipeline_controller_`, which holds a VideoRendererSink
-  // in MediaFoundationRendererClient.
-  pipeline_controller_.reset();
-
   client_ = nullptr;
   encrypted_client_ = nullptr;
   frame_scheduler_ = nullptr;
@@ -734,7 +727,6 @@
 
   // Handle destruction of things that need to be destructed after the pipeline
   // completes stopping on the media thread.
-  // TODO(crbug.com/482958590): This may not be necessary anymore.
   PostCrossThreadTask(
       *media_task_runner_, FROM_HERE,
       CrossThreadBindOnce(
@@ -3458,7 +3450,7 @@
 
   const auto stats = GetPipelineStatistics();
   const int64_t data_source_memory_usage =
-      demuxer_manager_->GetDataSourceMemoryUsage();
+      demuxer_manager_ ? demuxer_manager_->GetDataSourceMemoryUsage() : 0;
 
   // If we have video and no video memory usage and we've rendered the first
   // frame, assume the VideoFrameCompositor is holding onto the last frame after
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index 3fbbc2d..d2dfb00 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -434,6 +434,8 @@
   base::RunLoop().RunUntilIdle();
 
   pipeline_->Stop();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(renderer_);
 }
 
 TEST_F(PipelineImplTest, StartSuspendedAndResumeAudioOnly) {
diff --git a/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc b/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
index e16cd79..fa28b63 100644
--- a/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
+++ b/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
@@ -1175,6 +1175,28 @@
             data_source_size);
 }
 
+// Verify that re-entrant Shutdown() during OnPipelineSuspended() does not
+// destroy `pipeline_controller_` while it is still on the call stack.
+TEST_F(WebMediaPlayerImplTest, ShutdownDuringOnPipelineSuspended) {
+  InitializeWebMediaPlayerImpl();
+  base::RunLoop loop;
+  EXPECT_CALL(*client_, CouldPlayIfEnoughData())
+      .WillOnce(Return(false))
+      .WillOnce([&]() {
+        EXPECT_CALL(*client_, SetCcLayer(nullptr));
+        EXPECT_CALL(*client_, MediaRemotingStopped(_));
+        wmpi_->Shutdown();
+        loop.Quit();
+        return true;
+      });
+  wmpi_->SetPreload(WebMediaPlayer::kPreloadMetaData);
+  Load(kAudioOnlyTestFile);
+  loop.Run();
+
+  wmpi_.reset();
+  CycleThreads();
+}
+
 // Verify that Play() before kReadyStateHaveEnough doesn't increase buffer size.
 TEST_F(WebMediaPlayerImplTest, NoBufferSizeIncreaseUntilHaveEnough) {
   InitializeWebMediaPlayerImpl();

diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index 3fbbc2d..d2dfb00 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -434,6 +434,8 @@
   base::RunLoop().RunUntilIdle();
 
   pipeline_->Stop();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(renderer_);
 }
 
 TEST_F(PipelineImplTest, StartSuspendedAndResumeAudioOnly) {
diff --git a/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc b/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
index e16cd79..fa28b63 100644
--- a/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
+++ b/third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
@@ -1175,6 +1175,28 @@
             data_source_size);
 }
 
+// Verify that re-entrant Shutdown() during OnPipelineSuspended() does not
+// destroy `pipeline_controller_` while it is still on the call stack.
+TEST_F(WebMediaPlayerImplTest, ShutdownDuringOnPipelineSuspended) {
+  InitializeWebMediaPlayerImpl();
+  base::RunLoop loop;
+  EXPECT_CALL(*client_, CouldPlayIfEnoughData())
+      .WillOnce(Return(false))
+      .WillOnce([&]() {
+        EXPECT_CALL(*client_, SetCcLayer(nullptr));
+        EXPECT_CALL(*client_, MediaRemotingStopped(_));
+        wmpi_->Shutdown();
+        loop.Quit();
+        return true;
+      });
+  wmpi_->SetPreload(WebMediaPlayer::kPreloadMetaData);
+  Load(kAudioOnlyTestFile);
+  loop.Run();
+
+  wmpi_.reset();
+  CycleThreads();
+}
+
 // Verify that Play() before kReadyStateHaveEnough doesn't increase buffer size.
 TEST_F(WebMediaPlayerImplTest, NoBufferSizeIncreaseUntilHaveEnough) {
   InitializeWebMediaPlayerImpl();