From 2f6df874594524706ee2d13883ff889b4c9cd1d8 Mon Sep 17 00:00:00 2001
From: Dale Curtis <[email protected]>
Date: Thu, 05 Mar 2026 16:33:22 -0800
Subject: [PATCH] Always post destruction of WebMediaPlayer instances

There are lots of ways that re-entrant destruction of players can
happen. Fixing them all piecemeal is fragile and making WMP garbage
collected is difficult since it's a blink/public interface. For now
just add a Shutdown mechanism and post destruction such that we
never have re-entrant destruction.

Bug: 482958590, 459524033
Change-Id: I9ccdaeed448850a5133deb464dcaeafa7447fe94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7609443
Reviewed-by: Daniel Cheng <[email protected]>
Reviewed-by: Frank Liberato <[email protected]>
Auto-Submit: Dale Curtis <[email protected]>
Commit-Queue: Dale Curtis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1595039}
---

diff --git a/third_party/blink/public/platform/web_media_player.h b/third_party/blink/public/platform/web_media_player.h
index 389ac03..d91446fa 100644
--- a/third_party/blink/public/platform/web_media_player.h
+++ b/third_party/blink/public/platform/web_media_player.h
@@ -188,6 +188,11 @@
 
   virtual ~WebMediaPlayer() = default;
 
+  // Called just before the WebMediaPlayer is posted for destruction such that
+  // the WebMediaPlayer can clear any references to WebMediaPlayerClient and
+  // perform any other necessary cleanup.
+  virtual void Shutdown() = 0;
+
   virtual LoadTiming Load(LoadType,
                           const WebMediaPlayerSource&,
                           CorsMode,
diff --git a/third_party/blink/public/web/modules/mediastream/web_media_player_ms.h b/third_party/blink/public/web/modules/mediastream/web_media_player_ms.h
index ea7d538..431201f15 100644
--- a/third_party/blink/public/web/modules/mediastream/web_media_player_ms.h
+++ b/third_party/blink/public/web/modules/mediastream/web_media_player_ms.h
@@ -99,6 +99,8 @@
 
   ~WebMediaPlayerMS() override;
 
+  void Shutdown() override;
+
   WebMediaPlayer::LoadTiming Load(LoadType load_type,
                                   const WebMediaPlayerSource& source,
                                   CorsMode cors_mode,
@@ -279,7 +281,7 @@
 
   const WebTimeRanges buffered_;
 
-  const raw_ptr<MediaPlayerClient> client_;
+  raw_ptr<MediaPlayerClient> client_ = nullptr;
 
   // WebMediaPlayer notifies the |delegate_| of playback state changes using
   // |delegate_id_|; an id provided after registering with the delegate.  The
@@ -293,7 +295,7 @@
   // before the frame is destroyed). RenderFrameImpl owns of |delegate_|, and is
   // guaranteed to outlive |this|. It is therefore safe use a raw pointer
   // directly.
-  raw_ptr<WebMediaPlayerDelegate> delegate_;
+  raw_ptr<WebMediaPlayerDelegate> delegate_ = nullptr;
   int delegate_id_;
 
   const int player_id_;
@@ -327,7 +329,7 @@
   const scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
 
   const scoped_refptr<base::TaskRunner> worker_task_runner_;
-  raw_ptr<media::GpuVideoAcceleratorFactories> gpu_factories_;
+  raw_ptr<media::GpuVideoAcceleratorFactories> gpu_factories_ = nullptr;
 
   // Used for DCHECKs to ensure methods calls executed in the correct thread.
   THREAD_CHECKER(thread_checker_);
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 66a2bf80..ec320fe 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
@@ -395,6 +395,7 @@
     //
     // NOTE: This should be done before any other member variables are
     // destructed since WMPI may reference them during destruction.
+    wmpi_->Shutdown();
     wmpi_.reset();
 
     CycleThreads();
@@ -461,6 +462,7 @@
         media_thread_.task_runner());
     compositor_ = compositor.get();
 
+    CHECK(!wmpi_);
     wmpi_ = std::make_unique<WebMediaPlayerImpl>(
         GetWebLocalFrame(), &client_, &encrypted_client_, &delegate_,
         std::move(factory_selector), url_index_.get(), std::move(compositor),
@@ -2758,6 +2760,7 @@
   EXPECT_TRUE(dump_manager->IsDumpProviderRegisteredForTesting(media_dumper));
   CycleThreads();
 
+  wmpi_->Shutdown();
   wmpi_.reset();
   CycleThreads();
 
diff --git a/third_party/blink/renderer/core/html/media/html_media_element.cc b/third_party/blink/renderer/core/html/media/html_media_element.cc
index d6b34fb..1a0a13e 100644
--- a/third_party/blink/renderer/core/html/media/html_media_element.cc
+++ b/third_party/blink/renderer/core/html/media/html_media_element.cc
@@ -4181,7 +4181,12 @@
   GetAudioSourceProvider().SetClient(nullptr);
   if (web_media_player_) {
     audio_source_provider_.Wrap(nullptr);
-    web_media_player_.reset();
+    // Never destruct WMPI synchronously since it may be actively calling into
+    // the media element. Instead, post a task to delete it asynchronously.
+    web_media_player_->Shutdown();
+    GetDocument()
+        .GetTaskRunner(TaskType::kInternalMedia)
+        ->DeleteSoon(FROM_HERE, std::move(web_media_player_));
     // Do not clear `opener_document_` here; new players might still use it.
 
     // The lifetime of the mojo endpoints are tied to the WebMediaPlayer's, so
diff --git a/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc b/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
index a6f1128c..2a6beed 100644
--- a/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
+++ b/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
@@ -50,6 +50,7 @@
                   bool is_cache_disabled) override {
     return LoadTiming::kImmediate;
   }
+  void Shutdown() override {}
   void Play() override {}
   void Pause(PauseReason pause_reason) override {}
   void Seek(double seconds) override {}
diff --git a/third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc b/third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc
index 97498b7c..f827e74 100644
--- a/third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc
+++ b/third_party/blink/renderer/modules/mediastream/web_media_player_ms.cc
@@ -12,7 +12,6 @@
 #include <string>
 #include <utility>
 
-#include "base/debug/alias.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/raw_ptr.h"
@@ -65,19 +64,6 @@
 #include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
 
-// Put this macro in a scope to prevent `client_` from being GC'd.
-// This is important for any method that might be called from anywhere
-// where GC of the element is not prevented.  GC is prevented if the
-// call into `this` came from the element itself (directly or indirectly,
-// as long as the element's `this` is on the stack), or HasPendingActivation()
-// returns true.  In other cases, especially callbacks from the "outside
-// world", one should PREVENT_CLIENT_GC to keep the element from being
-// garbage collected.  Failure to do this can cause `this` to be destroyed
-// when the player is finalized.
-#define PREVENT_CLIENT_GC      \
-  auto client_copy_ = client_; \
-  base::debug::Alias(&client_copy_)
-
 namespace blink {
 
 namespace {
@@ -410,6 +396,12 @@
 
 WebMediaPlayerMS::~WebMediaPlayerMS() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  // Ensure Shutdown() has been called.
+  CHECK(!client_);
+}
+
+void WebMediaPlayerMS::Shutdown() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   SendLogMessage(
       String::Format("%s() [delegate_id=%d]", __func__, delegate_id_));
 
@@ -458,11 +450,14 @@
 
   delegate_->PlayerGone(delegate_id_);
   delegate_->RemoveObserver(delegate_id_);
+  delegate_ = nullptr;
+  client_ = nullptr;
+  gpu_factories_ = nullptr;
+  weak_factory_.InvalidateWeakPtrsAndDoom();
 }
 
 void WebMediaPlayerMS::OnAudioRenderErrorCallback() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  PREVENT_CLIENT_GC;
 
   if (watch_time_reporter_)
     watch_time_reporter_->OnError(media::AUDIO_RENDERER_ERROR);
@@ -1322,7 +1317,6 @@
     bool is_opaque) {
   DVLOG(1) << __func__;
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

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 66a2bf80..ec320fe 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
@@ -395,6 +395,7 @@
     //
     // NOTE: This should be done before any other member variables are
     // destructed since WMPI may reference them during destruction.
+    wmpi_->Shutdown();
     wmpi_.reset();
 
     CycleThreads();
@@ -461,6 +462,7 @@
         media_thread_.task_runner());
     compositor_ = compositor.get();
 
+    CHECK(!wmpi_);
     wmpi_ = std::make_unique<WebMediaPlayerImpl>(
         GetWebLocalFrame(), &client_, &encrypted_client_, &delegate_,
         std::move(factory_selector), url_index_.get(), std::move(compositor),
@@ -2758,6 +2760,7 @@
   EXPECT_TRUE(dump_manager->IsDumpProviderRegisteredForTesting(media_dumper));
   CycleThreads();
 
+  wmpi_->Shutdown();
   wmpi_.reset();
   CycleThreads();
diff --git a/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc b/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
index a6f1128c..2a6beed 100644
--- a/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
+++ b/third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
@@ -50,6 +50,7 @@
                   bool is_cache_disabled) override {
     return LoadTiming::kImmediate;
   }
+  void Shutdown() override {}
   void Play() override {}
   void Pause(PauseReason pause_reason) override {}
   void Seek(double seconds) override {}
diff --git a/third_party/blink/renderer/modules/mediastream/web_media_player_ms_test.cc b/third_party/blink/renderer/modules/mediastream/web_media_player_ms_test.cc
index 8668de84..fefe39e 100644
--- a/third_party/blink/renderer/modules/mediastream/web_media_player_ms_test.cc
+++ b/third_party/blink/renderer/modules/mediastream/web_media_player_ms_test.cc
@@ -545,6 +545,7 @@
     submitter_ptr_ = submitter_.get();
   }
   ~WebMediaPlayerMSTest() override {
+    player_->Shutdown();
     player_.reset();
     base::RunLoop().RunUntilIdle();
   }
@@ -698,6 +699,7 @@
 
 void WebMediaPlayerMSTest::InitializeWebMediaPlayerMS() {
   enable_surface_layer_for_video_ = testing::get<0>(GetParam());
+  CHECK(!player_);
   player_ = std::make_unique<WebMediaPlayerMS>(
       nullptr, this, &delegate_, std::make_unique<media::NullMediaLog>(),
       scheduler::GetSingleThreadTaskRunnerForTesting(),
diff --git a/third_party/blink/renderer/platform/testing/empty_web_media_player.h b/third_party/blink/renderer/platform/testing/empty_web_media_player.h
index 7156660f..cd186443 100644
--- a/third_party/blink/renderer/platform/testing/empty_web_media_player.h
+++ b/third_party/blink/renderer/platform/testing/empty_web_media_player.h
@@ -28,6 +28,7 @@
                   const WebMediaPlayerSource&,
                   CorsMode,
                   bool is_cache_disabled) override;
+  void Shutdown() override { weak_ptr_factory_.InvalidateWeakPtrsAndDoom(); }
   void Play() override {}
   void Pause(PauseReason pause_reason) override {}
   void Seek(double seconds) override {}