Make UserMediaProcessor GC-ed by Oilpan

Now that UserMediaClient is GC'ed (see [1]), this
CL moves UserMediaProcessor to be managed by Oilpan.

[1] crrev.com/c/1757492

BUG=704136
R=guidou@chromium.org, haraken@chromium.org

Change-Id: Icfe0579b32c98283f1a340cb86dd7c95f8de3b96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1758604
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689436}

third_party/blink/renderer/modules/mediastream/user_media_client.cc

@@ -100,10 +100,10 @@ UserMediaClient::Request::MoveUserMediaRequest() {
 
 UserMediaClient::UserMediaClient(
     LocalFrame* frame,
-    std::unique_ptr<UserMediaProcessor> user_media_processor,
+    UserMediaProcessor* user_media_processor,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : frame_(frame),
-      user_media_processor_(std::move(user_media_processor)),
+      user_media_processor_(user_media_processor),
       // WrapWeakPersistent is safe because UserMediaClient owns
       // ApplyConstraintsProcessor.
       apply_constraints_processor_(new ApplyConstraintsProcessor(
@@ -130,7 +130,7 @@ UserMediaClient::UserMediaClient(
           frame,
           // WrapWeakPersistent is safe because UserMediaClient owns
           // UserMediaProcessor.
-          std::make_unique<UserMediaProcessor>(
+          MakeGarbageCollected<UserMediaProcessor>(
               frame,
               WTF::BindRepeating(
                   [](UserMediaClient* client)
@@ -307,6 +307,7 @@ void UserMediaClient::ContextDestroyed() {
 
 void UserMediaClient::Trace(Visitor* visitor) {
   visitor->Trace(frame_);
+  visitor->Trace(user_media_processor_);
 }
 
 void UserMediaClient::SetMediaDevicesDispatcherForTesting(

third_party/blink/renderer/modules/mediastream/user_media_client.h

@@ -41,7 +41,7 @@ class MODULES_EXPORT UserMediaClient
   UserMediaClient(LocalFrame* frame,
                   scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   UserMediaClient(LocalFrame* frame,
-                  std::unique_ptr<UserMediaProcessor> user_media_processor,
+                  UserMediaProcessor* user_media_processor,
                   scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   virtual ~UserMediaClient();
 
@@ -106,7 +106,8 @@ class MODULES_EXPORT UserMediaClient
   WeakMember<LocalFrame> frame_;
 
   // |user_media_processor_| is a unique_ptr for testing purposes.
-  std::unique_ptr<UserMediaProcessor> user_media_processor_;
+  Member<UserMediaProcessor> user_media_processor_;
+
   // |user_media_processor_| is a unique_ptr in order to avoid compilation
   // problems in builds that do not include WebRTC.
   std::unique_ptr<ApplyConstraintsProcessor> apply_constraints_processor_;

third_party/blink/renderer/modules/mediastream/user_media_client_test.cc

@@ -449,7 +449,7 @@ class UserMediaClientUnderTest : public UserMediaClient {
                            RequestState* state)
       : UserMediaClient(
             nullptr,
-            base::WrapUnique(user_media_processor),
+            user_media_processor,
             blink::scheduler::GetSingleThreadTaskRunnerForTesting()),
         state_(state) {}
 
@@ -487,7 +487,7 @@ class UserMediaClientTest : public ::testing::Test {
         user_media_processor_host_proxy;
     binding_user_media_processor_.Bind(
         mojo::MakeRequest(&user_media_processor_host_proxy));
-    user_media_processor_ = new UserMediaProcessorUnderTest(
+    user_media_processor_ = MakeGarbageCollected<UserMediaProcessorUnderTest>(
         base::WrapUnique(msd_observer),
         std::move(user_media_processor_host_proxy), &state_);
     blink::mojom::blink::MediaStreamDispatcherHostPtr dispatcher_host =
@@ -652,8 +652,7 @@ class UserMediaClientTest : public ::testing::Test {
   mojo::Binding<blink::mojom::blink::MediaDevicesDispatcherHost>
       binding_user_media_client_;
 
-  UserMediaProcessorUnderTest* user_media_processor_ =
-      nullptr;  // Owned by |user_media_client_impl_|
+  WeakPersistent<UserMediaProcessorUnderTest> user_media_processor_;
   Persistent<UserMediaClientUnderTest> user_media_client_impl_;
   RequestState state_ = REQUEST_NOT_STARTED;
 };

third_party/blink/renderer/modules/mediastream/user_media_processor.cc

@@ -497,10 +497,10 @@ UserMediaProcessor::UserMediaProcessor(
 
 UserMediaProcessor::~UserMediaProcessor() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  // Force-close all outstanding user media requests and local sources here,
-  // before the outstanding WeakPtrs are invalidated, to ensure a clean
-  // shutdown.
-  StopAllProcessing();
+
+  // Ensure StopAllProcessing() has been called by UserMediaClient.
+  DCHECK(!current_request_info_ && !request_completed_cb_ &&
+         !local_sources_.size());
 }
 
 UserMediaRequestInfo* UserMediaProcessor::CurrentRequest() {
@@ -540,7 +540,7 @@ void UserMediaProcessor::SetupAudioInput() {
   if (blink::IsDeviceMediaType(audio_controls.stream_type)) {
     GetMediaDevicesDispatcher()->GetAudioInputCapabilities(WTF::Bind(
         &UserMediaProcessor::SelectAudioDeviceSettings,
-        weak_factory_.GetWeakPtr(), current_request_info_->web_request()));
+        WrapWeakPersistent(this), current_request_info_->web_request()));
   } else {
     if (!blink::IsAudioInputMediaType(audio_controls.stream_type)) {
       String failed_constraint_name =
@@ -686,7 +686,7 @@ void UserMediaProcessor::SetupVideoInput() {
   if (blink::IsDeviceMediaType(video_controls.stream_type)) {
     GetMediaDevicesDispatcher()->GetVideoInputCapabilities(WTF::Bind(
         &UserMediaProcessor::SelectVideoDeviceSettings,
-        weak_factory_.GetWeakPtr(), current_request_info_->web_request()));
+        WrapWeakPersistent(this), current_request_info_->web_request()));
   } else {
     if (!blink::IsVideoInputMediaType(video_controls.stream_type)) {
       String failed_constraint_name =
@@ -794,8 +794,7 @@ void UserMediaProcessor::GenerateStreamForCurrentRequestInfo(
       blink::mojom::blink::StreamSelectionInfo::New(
           strategy, requested_audio_capture_session_id),
       WTF::Bind(&UserMediaProcessor::OnStreamGenerated,
-                weak_factory_.GetWeakPtr(),
-                current_request_info_->request_id()));
+                WrapWeakPersistent(this), current_request_info_->request_id()));
 }
 
 WebMediaStreamDeviceObserver*
@@ -872,7 +871,7 @@ void UserMediaProcessor::OnStreamGenerated(
     GetMediaDevicesDispatcher()->GetAllVideoInputDeviceFormats(
         video_device_id,
         WTF::Bind(&UserMediaProcessor::GotAllVideoInputFormatsForDevice,
-                  weak_factory_.GetWeakPtr(),
+                  WrapWeakPersistent(this),
                   current_request_info_->web_request(), label,
                   video_device_id));
   }
@@ -929,15 +928,16 @@ void UserMediaProcessor::OnStreamGeneratedForCancelledRequest(
 // static
 void UserMediaProcessor::OnAudioSourceStartedOnAudioThread(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    base::WeakPtr<UserMediaProcessor> weak_ptr,
+    UserMediaProcessor* weak_ptr,
     blink::WebPlatformMediaStreamSource* source,
     MediaStreamRequestResult result,
     const blink::WebString& result_name) {
   PostCrossThreadTask(
       *task_runner.get(), FROM_HERE,
       CrossThreadBindOnce(&UserMediaProcessor::OnAudioSourceStarted,
-                          std::move(weak_ptr), CrossThreadUnretained(source),
-                          result, String(result_name)));
+                          WrapCrossThreadWeakPersistent(weak_ptr),
+                          CrossThreadUnretained(source), result,
+                          String(result_name)));
 }
 
 void UserMediaProcessor::OnAudioSourceStarted(
@@ -1038,6 +1038,10 @@ void UserMediaProcessor::OnDeviceChanged(const MediaStreamDevice& old_device,
   source_impl->ChangeSource(new_device);
 }
 
+void UserMediaProcessor::Trace(Visitor* visitor) {
+  visitor->Trace(frame_);
+}
+
 blink::WebMediaStreamSource UserMediaProcessor::InitializeVideoSourceObject(
     const MediaStreamDevice& device) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
@@ -1047,7 +1051,7 @@ blink::WebMediaStreamSource UserMediaProcessor::InitializeVideoSourceObject(
   if (!source.GetPlatformSource()) {
     source.SetPlatformSource(CreateVideoSource(
         device, WTF::BindRepeating(&UserMediaProcessor::OnLocalSourceStopped,
-                                   weak_factory_.GetWeakPtr())));
+                                   WrapWeakPersistent(this))));
     String device_id(device.id.data());
     source.SetCapabilities(ComputeCapabilitiesForVideoSource(
         // TODO(crbug.com/704136): Change ComputeCapabilitiesForVideoSource to
@@ -1089,12 +1093,12 @@ blink::WebMediaStreamSource UserMediaProcessor::InitializeAudioSourceObject(
   blink::WebPlatformMediaStreamSource::ConstraintsRepeatingCallback
       source_ready = ConvertToBaseCallback(CrossThreadBindRepeating(
           &UserMediaProcessor::OnAudioSourceStartedOnAudioThread, task_runner_,
-          weak_factory_.GetWeakPtr()));
+          WrapCrossThreadWeakPersistent(this)));
 
   std::unique_ptr<blink::MediaStreamAudioSource> audio_source =
       CreateAudioSource(device, std::move(source_ready));
-  audio_source->SetStopCallback(BindRepeating(
-      &UserMediaProcessor::OnLocalSourceStopped, weak_factory_.GetWeakPtr()));
+  audio_source->SetStopCallback(WTF::BindRepeating(
+      &UserMediaProcessor::OnLocalSourceStopped, WrapWeakPersistent(this)));
 
 #if DCHECK_IS_ON()
   for (const auto& local_source : local_sources_) {
@@ -1219,9 +1223,9 @@ void UserMediaProcessor::StartTracks(const String& label) {
       ToStdVector(current_request_info_->audio_devices()),
       ToStdVector(current_request_info_->video_devices()),
       WTF::BindRepeating(&UserMediaProcessor::OnDeviceStopped,
-                         weak_factory_.GetWeakPtr()),
+                         WrapWeakPersistent(this)),
       WTF::BindRepeating(&UserMediaProcessor::OnDeviceChanged,
-                         weak_factory_.GetWeakPtr()));
+                         WrapWeakPersistent(this)));
 
   Vector<blink::WebMediaStreamTrack> audio_tracks(
       current_request_info_->audio_devices().size());
@@ -1240,7 +1244,7 @@ void UserMediaProcessor::StartTracks(const String& label) {
   // Wait for the tracks to be started successfully or to fail.
   current_request_info_->CallbackOnTracksStarted(
       WTF::Bind(&UserMediaProcessor::OnCreateNativeTracksCompleted,
-                weak_factory_.GetWeakPtr(), label));
+                WrapWeakPersistent(this), label));
 }
 
 void UserMediaProcessor::CreateVideoTracks(
@@ -1338,7 +1342,7 @@ void UserMediaProcessor::GetUserMediaRequestSucceeded(
   task_runner_->PostTask(
       FROM_HERE,
       WTF::Bind(&UserMediaProcessor::DelayedGetUserMediaRequestSucceeded,
-                weak_factory_.GetWeakPtr(), stream, web_request));
+                WrapWeakPersistent(this), stream, web_request));
 }
 
 void UserMediaProcessor::DelayedGetUserMediaRequestSucceeded(
@@ -1367,8 +1371,8 @@ void UserMediaProcessor::GetUserMediaRequestFailed(
   task_runner_->PostTask(
       FROM_HERE,
       WTF::Bind(&UserMediaProcessor::DelayedGetUserMediaRequestFailed,
-                weak_factory_.GetWeakPtr(),
-                current_request_info_->web_request(), result, constraint_name));
+                WrapWeakPersistent(this), current_request_info_->web_request(),
+                result, constraint_name));
 }
 
 void UserMediaProcessor::DelayedGetUserMediaRequestFailed(

third_party/blink/renderer/modules/mediastream/user_media_processor.h

@@ -11,7 +11,6 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "third_party/blink/public/common/mediastream/media_stream_request.h"
 #include "third_party/blink/public/mojom/mediastream/media_devices.mojom-blink.h"
@@ -56,7 +55,8 @@ struct UserMediaRequestInfo {
 // Only one MediaStream at a time can be in the process of being created.
 // UserMediaProcessor must be created, called and destroyed on the main
 // render thread. There should be only one UserMediaProcessor per frame.
-class MODULES_EXPORT UserMediaProcessor {
+class MODULES_EXPORT UserMediaProcessor
+    : public GarbageCollectedFinalized<UserMediaProcessor> {
  public:
   using MediaDevicesDispatcherCallback = base::RepeatingCallback<
       const blink::mojom::blink::MediaDevicesDispatcherHostPtr&()>;
@@ -103,6 +103,8 @@ class MODULES_EXPORT UserMediaProcessor {
     dispatcher_host_ = std::move(dispatcher_host);
   }
 
+  void Trace(Visitor*);
+
  protected:
   // These methods are virtual for test purposes. A test can override them to
   // test requesting local media streams. The function notifies WebKit that the
@@ -199,7 +201,7 @@ class MODULES_EXPORT UserMediaProcessor {
 
   static void OnAudioSourceStartedOnAudioThread(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-      base::WeakPtr<UserMediaProcessor> weak_ptr,
+      UserMediaProcessor* weak_ptr,
       blink::WebPlatformMediaStreamSource* source,
       blink::mojom::blink::MediaStreamRequestResult result,
       const blink::WebString& result_name);
@@ -292,17 +294,11 @@ class MODULES_EXPORT UserMediaProcessor {
   MediaDevicesDispatcherCallback media_devices_dispatcher_cb_;
   base::OnceClosure request_completed_cb_;
 
-  // TODO(crbug.com/704136): Consider moving UserMediaClient to
-  // Oilpan and use a Member.
-  WeakPersistent<LocalFrame> frame_;
+  WeakMember<LocalFrame> frame_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   THREAD_CHECKER(thread_checker_);
 
-  // Note: This member must be the last to ensure all outstanding weak pointers
-  // are invalidated first.
-  base::WeakPtrFactory<UserMediaProcessor> weak_factory_{this};
-
   DISALLOW_COPY_AND_ASSIGN(UserMediaProcessor);
 };