[Backport] CVE-2023-0931: Use after free in Video (2/2)

Manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4225497:
[M102-LTS] Further simplify WebMediaPlayerMSCompositor lifetime.

M102 merge issues:
  third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc:
    - video_task_runner_ is named io_task_runner_ in 102
    - Conflicting arguments for CrossThreadBindOnce in ActivateSurfaceLayerForVideo()

  third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc:
    - The use_surface_layer check is different in 102
    - video_task_runner_ is named io_task_runner_ in 102 (conflict in
      ReplaceCurrentFrameWithACopy)

Due to the raw pointer held by VideoFrameSubmitter, there may be
tasks pending on the compositor task runner after the RefCounted
traits have "destructed" WebMediaPlayerMSCompositor. Through this
raw pointer VFS was invoking OnContextLost which attempts to use
the zero ref count compositor.

The solution here is again similar to VideoFrameCompositor, its
destruction should be explicit instead of a tangle of RefCounted
owners.

(cherry picked from commit 1622bffc6534a0cc4f53d07c43e0cd8f49975d10)

Fixed: 1407701, 1411601
Change-Id: Ic77294d1113d54ab83bc0f5b625a997edf57bf7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210508
Commit-Queue: Tony Herre <toprice@chromium.org>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1099726}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4225497
Reviewed-by: Michael Ershov <miersh@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Owners-Override: Michael Ershov <miersh@google.com>
Cr-Commit-Position: refs/branch-heads/5005@{#1436}
Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/462799
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

4 files changed, 62 insertions, 64 deletions

diff --git a/chromium/third_party/blink/public/web/modules/mediastream/webmediaplayer_ms.h b/chromium/third_party/blink/public/web/modules/mediastream/webmediaplayer_ms.h
index 7166dbc4a5a..d132628f592 100644
--- a/chromium/third_party/blink/public/web/modules/mediastream/webmediaplayer_ms.h
+++ b/chromium/third_party/blink/public/web/modules/mediastream/webmediaplayer_ms.h
@@ -253,6 +253,8 @@ class BLINK_MODULES_EXPORT WebMediaPlayerMS
   static const gfx::Size kUseGpuMemoryBufferVideoFramesMinResolution;
 #endif  // defined(OS_WIN)
 
+  void ReplaceCurrentFrameWithACopy();
+
   bool IsInPictureInPicture() const;
 
   // Switch to SurfaceLayer, either initially or from VideoLayer.
@@ -348,7 +350,7 @@ class BLINK_MODULES_EXPORT WebMediaPlayerMS
   // Used for DCHECKs to ensure methods calls executed in the correct thread.
   THREAD_CHECKER(thread_checker_);
 
-  scoped_refptr<WebMediaPlayerMSCompositor> compositor_;
+  std::unique_ptr<WebMediaPlayerMSCompositor> compositor_;
 
   const WebString initial_audio_output_device_id_;
 
diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc
index 6d035b2e55d..523ad73012d 100644
--- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc
+++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms.cc
@@ -366,8 +366,9 @@ WebMediaPlayerMS::~WebMediaPlayerMS() {
   SendLogMessage(
       String::Format("%s() [delegate_id=%d]", __func__, delegate_id_));
 
-  if (!web_stream_.IsNull())
+  if (!web_stream_.IsNull()) {
     web_stream_.RemoveObserver(this);
+  }
 
   // Destruct compositor resources in the proper order.
   get_client()->SetCcLayer(nullptr);
@@ -376,14 +377,31 @@ WebMediaPlayerMS::~WebMediaPlayerMS() {
     video_layer_->StopUsingProvider();
   }
 
-  if (frame_deliverer_)
-    io_task_runner_->DeleteSoon(FROM_HERE, frame_deliverer_.release());
+  if (frame_deliverer_) {
+    io_task_runner_->DeleteSoon(FROM_HERE, std::move(frame_deliverer_));
+  }
 
-  if (video_frame_provider_)
+  if (video_frame_provider_) {
     video_frame_provider_->Stop();
+  }
 
-  if (audio_renderer_)
+  if (compositor_) {
+    // `compositor_` receives frames on `io_task_runner_` from
+    // `frame_deliverer_` and operates on the `compositor_task_runner_`, so
+    // must trampoline through both to ensure a safe destruction.
+    PostCrossThreadTask(
+        *io_task_runner_, FROM_HERE,
+        WTF::CrossThreadBindOnce(
+            [](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+               std::unique_ptr<WebMediaPlayerMSCompositor> compositor) {
+              task_runner->DeleteSoon(FROM_HERE, std::move(compositor));
+            },
+            compositor_task_runner_, std::move(compositor_)));
+  }
+
+  if (audio_renderer_) {
     audio_renderer_->Stop();
+  }
 
   media_log_->AddEvent<media::MediaLogEvent::kWebMediaPlayerDestroyed>();
 
@@ -418,7 +436,7 @@ WebMediaPlayer::LoadTiming WebMediaPlayerMS::Load(
   if (!web_stream_.IsNull())
     web_stream_.AddObserver(this);
 
-  compositor_ = base::MakeRefCounted<WebMediaPlayerMSCompositor>(
+  compositor_ = std::make_unique<WebMediaPlayerMSCompositor>(
       compositor_task_runner_, io_task_runner_, web_stream_,
       std::move(submitter_), surface_layer_mode_, weak_this_);
 
@@ -441,7 +459,7 @@ WebMediaPlayer::LoadTiming WebMediaPlayerMS::Load(
   frame_deliverer_.reset(new WebMediaPlayerMS::FrameDeliverer(
       weak_this_,
       CrossThreadBindRepeating(&WebMediaPlayerMSCompositor::EnqueueFrame,
-                               compositor_),
+                               CrossThreadUnretained(compositor_.get())),
       media_task_runner_, worker_task_runner_, gpu_factories_));
   video_frame_provider_ = renderer_factory_->GetVideoRenderer(
       web_stream_,
@@ -740,7 +758,19 @@ void WebMediaPlayerMS::Pause() {
     video_frame_provider_->Pause();
 
   compositor_->StopRendering();
-  compositor_->ReplaceCurrentFrameWithACopy();
+
+  // Bounce this call off of video task runner to since there might still be
+  // frames passed on video task runner.
+  PostCrossThreadTask(
+      *io_task_runner_, FROM_HERE,
+      WTF::CrossThreadBindOnce(
+          [](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+             WTF::CrossThreadOnceClosure copy_cb) {
+            PostCrossThreadTask(*task_runner, FROM_HERE, std::move(copy_cb));
+          },
+          main_render_task_runner_,
+          WTF::CrossThreadBindOnce(
+              &WebMediaPlayerMS::ReplaceCurrentFrameWithACopy, weak_this_)));
 
   if (audio_renderer_)
     audio_renderer_->Pause();
@@ -751,6 +781,11 @@ void WebMediaPlayerMS::Pause() {
   paused_ = true;
 }
 
+void WebMediaPlayerMS::ReplaceCurrentFrameWithACopy() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  compositor_->ReplaceCurrentFrameWithACopy();
+}
+
 void WebMediaPlayerMS::Seek(double seconds) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 }
@@ -1217,8 +1252,9 @@ void WebMediaPlayerMS::ActivateSurfaceLayerForVideo() {
   PostCrossThreadTask(
       *compositor_task_runner_, FROM_HERE,
       CrossThreadBindOnce(&WebMediaPlayerMSCompositor::EnableSubmission,
-                          compositor_, bridge_->GetSurfaceId(),
-                          video_transformation_, IsInPictureInPicture()));
+                          CrossThreadUnretained(compositor_.get()),
+                          bridge_->GetSurfaceId(), video_transformation_,
+                          IsInPictureInPicture()));
 
   // If the element is already in Picture-in-Picture mode, it means that it
   // was set in this mode prior to this load, with a different
diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc
index 0763e167be4..6632ee8f037 100644
--- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc
+++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.cc
@@ -165,18 +165,19 @@ WebMediaPlayerMSCompositor::WebMediaPlayerMSCompositor(
       dropped_frame_count_(0),
       stopped_(true),
       render_started_(!stopped_) {
+  weak_this_ = weak_ptr_factory_.GetWeakPtr();
   if (surface_layer_mode != WebMediaPlayer::SurfaceLayerMode::kNever) {
     submitter_ = std::move(submitter);
 
     PostCrossThreadTask(
         *video_frame_compositor_task_runner_, FROM_HERE,
         CrossThreadBindOnce(&WebMediaPlayerMSCompositor::InitializeSubmitter,
-                            weak_ptr_factory_.GetWeakPtr()));
+                            weak_this_));
     update_submission_state_callback_ = media::BindToLoop(
         video_frame_compositor_task_runner_,
         ConvertToBaseRepeatingCallback(CrossThreadBindRepeating(
             &WebMediaPlayerMSCompositor::SetIsSurfaceVisible,
-            weak_ptr_factory_.GetWeakPtr())));
+            weak_this_)));
   }
 
   HeapVector<Member<MediaStreamComponent>> video_components;
@@ -204,7 +205,6 @@ WebMediaPlayerMSCompositor::WebMediaPlayerMSCompositor(
 }
 
 WebMediaPlayerMSCompositor::~WebMediaPlayerMSCompositor() {
-  // Ensured by destructor traits.
   DCHECK(video_frame_compositor_task_runner_->BelongsToCurrentThread());
 
   if (video_frame_provider_client_) {
@@ -212,20 +212,6 @@ WebMediaPlayerMSCompositor::~WebMediaPlayerMSCompositor() {
   }
 }
 
-// static
-void WebMediaPlayerMSCompositorTraits::Destruct(
-    const WebMediaPlayerMSCompositor* compositor) {
-  if (!compositor->video_frame_compositor_task_runner_
-           ->BelongsToCurrentThread()) {
-    PostCrossThreadTask(
-        *compositor->video_frame_compositor_task_runner_, FROM_HERE,
-        CrossThreadBindOnce(&WebMediaPlayerMSCompositorTraits::Destruct,
-                            CrossThreadUnretained(compositor)));
-    return;
-  }
-  delete compositor;
-}
-
 void WebMediaPlayerMSCompositor::InitializeSubmitter() {
   DCHECK(video_frame_compositor_task_runner_->BelongsToCurrentThread());
   submitter_->Initialize(this, /* is_media_stream = */ true);
@@ -267,7 +253,7 @@ void WebMediaPlayerMSCompositor::SetForceBeginFrames(bool enable) {
     PostCrossThreadTask(
         *video_frame_compositor_task_runner_, FROM_HERE,
         CrossThreadBindOnce(&WebMediaPlayerMSCompositor::SetForceBeginFrames,
-                            weak_ptr_factory_.GetWeakPtr(), enable));
+                            weak_this_, enable));
     return;
   }
 
@@ -460,7 +446,7 @@ void WebMediaPlayerMSCompositor::StartRendering() {
   PostCrossThreadTask(
       *video_frame_compositor_task_runner_, FROM_HERE,
       CrossThreadBindOnce(&WebMediaPlayerMSCompositor::StartRenderingInternal,
-                          WrapRefCounted(this)));
+                          weak_this_));
 }
 
 void WebMediaPlayerMSCompositor::StopRendering() {
@@ -468,18 +454,7 @@ void WebMediaPlayerMSCompositor::StopRendering() {
   PostCrossThreadTask(
       *video_frame_compositor_task_runner_, FROM_HERE,
       CrossThreadBindOnce(&WebMediaPlayerMSCompositor::StopRenderingInternal,
-                          WrapRefCounted(this)));
-}
-
-void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  // Bounce this call off of IO thread to since there might still be frames
-  // passed on IO thread.
-  io_task_runner_->PostTask(
-      FROM_HERE,
-      media::BindToCurrentLoop(WTF::Bind(
-          &WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopyInternal,
-          WrapRefCounted(this))));
+                          weak_this_));
 }
 
 bool WebMediaPlayerMSCompositor::MapTimestampsToRenderTimeTicks(
@@ -549,7 +524,7 @@ void WebMediaPlayerMSCompositor::RenderWithoutAlgorithm(
       *video_frame_compositor_task_runner_, FROM_HERE,
       CrossThreadBindOnce(
           &WebMediaPlayerMSCompositor::RenderWithoutAlgorithmOnCompositor,
-          WrapRefCounted(this), std::move(frame), is_copy));
+          weak_this_, std::move(frame), is_copy));
 }
 
 void WebMediaPlayerMSCompositor::RenderWithoutAlgorithmOnCompositor(
@@ -637,9 +612,8 @@ void WebMediaPlayerMSCompositor::SetCurrentFrame(
   PostCrossThreadTask(
       *video_frame_compositor_task_runner_, FROM_HERE,
       CrossThreadBindOnce(&WebMediaPlayerMSCompositor::CheckForFrameChanges,
-                          WrapRefCounted(this), is_first_frame,
-                          has_frame_size_changed, std::move(new_rotation),
-                          std::move(new_opacity)));
+                          weak_this_, is_first_frame, has_frame_size_changed,
+                          std::move(new_rotation), std::move(new_opacity)));
 }
 
 void WebMediaPlayerMSCompositor::CheckForFrameChanges(
@@ -706,7 +680,7 @@ void WebMediaPlayerMSCompositor::StopRenderingInternal() {
     video_frame_provider_client_->StopRendering();
 }
 
-void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopyInternal() {
+void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   scoped_refptr<media::VideoFrame> current_frame_ref;
   {
diff --git a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h
index ae0a1a39f9f..2baccc05cce 100644
--- a/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h
+++ b/chromium/third_party/blink/renderer/modules/mediastream/webmediaplayer_ms_compositor.h
@@ -22,7 +22,6 @@
 #include "third_party/blink/public/platform/web_video_frame_submitter.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
 #include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
-#include "third_party/blink/renderer/platform/wtf/thread_safe_ref_counted.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -43,7 +42,6 @@ class SurfaceId;
 namespace blink {
 class MediaStreamDescriptor;
 class WebMediaPlayerMS;
-struct WebMediaPlayerMSCompositorTraits;
 
 // This class is designed to handle the work load on compositor thread for
 // WebMediaPlayerMS. It will be instantiated on the main thread, but destroyed
@@ -55,9 +53,7 @@ struct WebMediaPlayerMSCompositorTraits;
 // Otherwise, WebMediaPlayerMSCompositor will simply store the most recent
 // frame, and submit it whenever asked by the compositor.
 class MODULES_EXPORT WebMediaPlayerMSCompositor
-    : public cc::VideoFrameProvider,
-      public WTF::ThreadSafeRefCounted<WebMediaPlayerMSCompositor,
-                                       WebMediaPlayerMSCompositorTraits> {
+    : public cc::VideoFrameProvider {
  public:
   using OnNewFramePresentedCB = base::OnceClosure;
 
@@ -68,6 +64,7 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor
       std::unique_ptr<WebVideoFrameSubmitter> submitter,
       WebMediaPlayer::SurfaceLayerMode surface_layer_mode,
       const base::WeakPtr<WebMediaPlayerMS>& player);
+  ~WebMediaPlayerMSCompositor() override;
 
   // Can be called from any thread.
   cc::UpdateSubmissionStateCB GetUpdateSubmissionStateCallback() {
@@ -130,10 +127,7 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor
   void SetForceBeginFrames(bool enable);
 
  private:
-  friend class WTF::ThreadSafeRefCounted<WebMediaPlayerMSCompositor,
-                                         WebMediaPlayerMSCompositorTraits>;
   friend class WebMediaPlayerMSTest;
-  friend struct WebMediaPlayerMSCompositorTraits;
 
   // Struct used to keep information about frames pending in
   // |rendering_frame_buffer_|.
@@ -144,8 +138,6 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor
     bool is_copy;
   };
 
-  ~WebMediaPlayerMSCompositor() override;
-
   // Ran on the |video_frame_compositor_task_runner_| to initialize
   // |submitter_|
   void InitializeSubmitter();
@@ -191,7 +183,6 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor
 
   void StartRenderingInternal();
   void StopRenderingInternal();
-  void ReplaceCurrentFrameWithACopyInternal();
 
   void SetAlgorithmEnabledForTesting(bool algorithm_enabled);
 
@@ -281,17 +272,12 @@ class MODULES_EXPORT WebMediaPlayerMSCompositor
   // |dropped_frame_count_|, and |render_started_|.
   base::Lock current_frame_lock_;
 
+  base::WeakPtr<WebMediaPlayerMSCompositor> weak_this_;
   base::WeakPtrFactory<WebMediaPlayerMSCompositor> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(WebMediaPlayerMSCompositor);
 };
 
-struct WebMediaPlayerMSCompositorTraits {
-  // Ensure destruction occurs on main thread so that "Web" and other resources
-  // are destroyed on the correct thread.
-  static void Destruct(const WebMediaPlayerMSCompositor* player);
-};
-
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_MODULES_MEDIASTREAM_WEBMEDIAPLAYER_MS_COMPOSITOR_H_