Fix UaF in WebContentsFrameTracker

This patch fixes a use-after-free by moving to a base::WeakPtr
instead of a raw_ptr. Looking at the callstack in the referenced bug, what is clearly happening is that the frame tracker is deleted AFTER the capture device. I believe that this is due to the MouseCursorOverlayController being deleted through the DeleteOnUIThread destructor, which, if you are already on the UI thread, is synchronous:

https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/browser_thread.h;l=141?q=BrowserThread::DeleteOnThread&ss=chromium%2Fchromium%2Fsrc

In comparison, the WebContentsFrameTracker is implemented using base::SequenceBound, which ends up calling an internal destruct method that ALWAYS posts back a task:

https://source.chromium.org/chromium/chromium/src/+/main:base/threading/sequence_bound_internal.h;drc=f5bdc89c7395ed24f1b8d196a3bdd6232d5bf771;l=122

So, this bug is ultimately caused by the simple fact that base::SequenceBound does NOT have an optimization to not post a deletion task if we are already running on that sequence. There may be a good followup task here to change either DeleteOnThread or base::SequenceBound to have the same behavior, however I think this change a good first step.

Bug: 1480152
Change-Id: Iee2d41e66b10403d6c78547bcbe84d2454236d5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908770
Reviewed-by: Mark Foltz <mfoltz@chromium.org>
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1206698}

content/browser/media/capture/web_contents_frame_tracker.cc

@@ -134,17 +134,20 @@ WebContentsFrameTracker::WebContentsFrameTracker(
     base::WeakPtr<WebContentsVideoCaptureDevice> device,
     MouseCursorOverlayController* cursor_controller)
     : device_(std::move(device)),
-      device_task_runner_(std::move(device_task_runner)) {
+      device_task_runner_(std::move(device_task_runner))
+#if !BUILDFLAG(IS_ANDROID)
+      ,
+      cursor_controller_(cursor_controller->GetWeakPtr())
+#endif
+{
   // Verify on construction that this object is created on the UI thread.  After
   // this, depend on the sequence checker to ensure consistent execution.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  DCHECK(device_task_runner_);
+  CHECK(device_task_runner_);
 
 #if !BUILDFLAG(IS_ANDROID)
-  cursor_controller_ = cursor_controller;
-  DCHECK(cursor_controller_);
+  CHECK(cursor_controller_);
 #endif
 }
 
@@ -528,7 +531,9 @@ void WebContentsFrameTracker::SetTargetView(gfx::NativeView view) {
   }
   target_native_view_ = view;
 #if !BUILDFLAG(IS_ANDROID)
-  cursor_controller_->SetTargetView(view);
+  if (cursor_controller_) {
+    cursor_controller_->SetTargetView(view);
+  }
 #endif
 }
 

content/browser/media/capture/web_contents_frame_tracker.h

@@ -173,13 +173,12 @@ class CONTENT_EXPORT WebContentsFrameTracker final
   // The task runner to be used for device callbacks.
   const scoped_refptr<base::SequencedTaskRunner> device_task_runner_;
 
-  // Owned by FrameSinkVideoCaptureDevice. This will be valid for the life of
-  // WebContentsFrameTracker because the WebContentsFrameTracker deleter task
-  // will be posted to the UI thread before the MouseCursorOverlayController
-  // deleter task.
+  // Owned by FrameSinkVideoCaptureDevice.  This may only be accessed on the
+  // UI thread. This is not guaranteed to be valid and must be checked before
+  // use.
+  // https://crbug.com/1480152
 #if !BUILDFLAG(IS_ANDROID)
-  raw_ptr<MouseCursorOverlayController, AcrossTasksDanglingUntriaged>
-      cursor_controller_ = nullptr;
+  const base::WeakPtr<MouseCursorOverlayController> cursor_controller_;
 #endif
 
   // We may not have a frame sink ID target at all times.