Using WeakPtr for requests to MediaStreamDispatcher.

This is to ensure no dangling pointers to MediaStreamImpl (and other requesters). It's one of several fixes for the bug. No actual crash or issue has been reported that this CL fixes; it's for removing potential issues.

BUG=112408

Review URL: https://chromiumcodereview.appspot.com/9903014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131330 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 84 insertions, 63 deletions

diff --git a/content/renderer/media/media_stream_dispatcher.cc b/content/renderer/media/media_stream_dispatcher.cc
index 254740a..cdf1d02 100644
--- a/content/renderer/media/media_stream_dispatcher.cc
+++ b/content/renderer/media/media_stream_dispatcher.cc
@@ -10,22 +10,22 @@
 #include "content/renderer/render_view_impl.h"
 
 struct MediaStreamDispatcher::Request {
-  Request(MediaStreamDispatcherEventHandler* handler,
+  Request(const base::WeakPtr<MediaStreamDispatcherEventHandler>& handler,
           int request_id,
           int ipc_request)
       : handler(handler),
         request_id(request_id),
         ipc_request(ipc_request) {
   }
-  MediaStreamDispatcherEventHandler* handler;
+  base::WeakPtr<MediaStreamDispatcherEventHandler> handler;
   int request_id;
   int ipc_request;
 };
 
 struct MediaStreamDispatcher::Stream {
-  Stream() : handler(NULL) {}
+  Stream() {}
   ~Stream() {}
-  MediaStreamDispatcherEventHandler* handler;
+  base::WeakPtr<MediaStreamDispatcherEventHandler> handler;
   media_stream::StreamDeviceInfoArray audio_array;
   media_stream::StreamDeviceInfoArray video_array;
 };
@@ -39,7 +39,7 @@ MediaStreamDispatcher::~MediaStreamDispatcher() {}
 
 void MediaStreamDispatcher::GenerateStream(
     int request_id,
-    MediaStreamDispatcherEventHandler* event_handler,
+    const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
     media_stream::StreamOptions components,
     const std::string& security_origin) {
   DVLOG(1) << "MediaStreamDispatcher::GenerateStream(" << request_id << ")";
@@ -65,7 +65,7 @@ void MediaStreamDispatcher::StopStream(const std::string& label) {
 
 void MediaStreamDispatcher::EnumerateDevices(
     int request_id,
-    MediaStreamDispatcherEventHandler* event_handler,
+    const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
     media_stream::MediaStreamType type,
     const std::string& security_origin) {
   DVLOG(1) << "MediaStreamDispatcher::EnumerateDevices("
@@ -80,7 +80,7 @@ void MediaStreamDispatcher::EnumerateDevices(
 
 void MediaStreamDispatcher::OpenDevice(
     int request_id,
-    MediaStreamDispatcherEventHandler* event_handler,
+    const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
     const std::string& device_id,
     media_stream::MediaStreamType type,
     const std::string& security_origin) {
@@ -140,10 +140,12 @@ void MediaStreamDispatcher::OnStreamGenerated(
       new_stream.audio_array = audio_array;
       new_stream.video_array = video_array;
       label_stream_map_[label] = new_stream;
-      request.handler->OnStreamGenerated(request.request_id, label,
-                                         audio_array, video_array);
-      DVLOG(1) << "MediaStreamDispatcher::OnStreamGenerated("
-               << request.request_id << ", " << label << ")";
+      if (request.handler) {
+        request.handler->OnStreamGenerated(request.request_id, label,
+                                           audio_array, video_array);
+        DVLOG(1) << "MediaStreamDispatcher::OnStreamGenerated("
+                 << request.request_id << ", " << label << ")";
+      }
       requests_.erase(it);
       break;
     }
@@ -155,9 +157,11 @@ void MediaStreamDispatcher::OnStreamGenerationFailed(int request_id) {
        it != requests_.end(); ++it) {
     Request& request = *it;
     if (request.ipc_request == request_id) {
-      request.handler->OnStreamGenerationFailed(request.request_id);
-      DVLOG(1) << "MediaStreamDispatcher::OnStreamGenerationFailed("
-               << request.request_id << ")\n";
+      if (request.handler) {
+        request.handler->OnStreamGenerationFailed(request.request_id);
+        DVLOG(1) << "MediaStreamDispatcher::OnStreamGenerationFailed("
+                 << request.request_id << ")\n";
+      }
       requests_.erase(it);
       break;
     }
@@ -175,7 +179,8 @@ void MediaStreamDispatcher::OnVideoDeviceFailed(const std::string& label,
   media_stream::StreamDeviceInfoArray::iterator device_it =
       it->second.video_array.begin();
   it->second.video_array.erase(device_it + index);
-  it->second.handler->OnVideoDeviceFailed(label, index);
+  if (it->second.handler)
+    it->second.handler->OnVideoDeviceFailed(label, index);
 }
 
 void MediaStreamDispatcher::OnAudioDeviceFailed(const std::string& label,
@@ -189,7 +194,8 @@ void MediaStreamDispatcher::OnAudioDeviceFailed(const std::string& label,
   media_stream::StreamDeviceInfoArray::iterator device_it =
       it->second.audio_array.begin();
   it->second.audio_array.erase(device_it + index);
-  it->second.handler->OnAudioDeviceFailed(label, index);
+  if (it->second.handler)
+    it->second.handler->OnAudioDeviceFailed(label, index);
 }
 
 void MediaStreamDispatcher::OnDevicesEnumerated(
@@ -200,9 +206,11 @@ void MediaStreamDispatcher::OnDevicesEnumerated(
        it != requests_.end(); ++it) {
     Request& request = *it;
     if (request.ipc_request == request_id) {
-      request.handler->OnDevicesEnumerated(request.request_id, device_array);
-      DVLOG(1) << "MediaStreamDispatcher::OnDevicesEnumerated("
-               << request.request_id << ")";
+      if (request.handler) {
+        request.handler->OnDevicesEnumerated(request.request_id, device_array);
+        DVLOG(1) << "MediaStreamDispatcher::OnDevicesEnumerated("
+                 << request.request_id << ")";
+      }
       requests_.erase(it);
       break;
     }
@@ -214,9 +222,11 @@ void MediaStreamDispatcher::OnDevicesEnumerationFailed(int request_id) {
        it != requests_.end(); ++it) {
     Request& request = *it;
     if (request.ipc_request == request_id) {
-      request.handler->OnStreamGenerationFailed(request.request_id);
-      DVLOG(1) << "MediaStreamDispatcher::OnDevicesEnumerationFailed("
-               << request.request_id << ")\n";
+      if (request.handler) {
+        request.handler->OnStreamGenerationFailed(request.request_id);
+        DVLOG(1) << "MediaStreamDispatcher::OnDevicesEnumerationFailed("
+                 << request.request_id << ")\n";
+      }
       requests_.erase(it);
       break;
     }
@@ -240,10 +250,12 @@ void MediaStreamDispatcher::OnDeviceOpened(
         new_stream.audio_array.push_back(device_info);
       }
       label_stream_map_[label] = new_stream;
-      request.handler->OnDeviceOpened(request.request_id, label,
-                                      device_info);
-      DVLOG(1) << "MediaStreamDispatcher::OnDeviceOpened("
-               << request.request_id << ", " << label << ")";
+      if (request.handler) {
+        request.handler->OnDeviceOpened(request.request_id, label,
+                                        device_info);
+        DVLOG(1) << "MediaStreamDispatcher::OnDeviceOpened("
+                 << request.request_id << ", " << label << ")";
+      }
       requests_.erase(it);
       break;
     }
@@ -255,9 +267,11 @@ void MediaStreamDispatcher::OnDeviceOpenFailed(int request_id) {
        it != requests_.end(); ++it) {
     Request& request = *it;
     if (request.ipc_request == request_id) {
-      request.handler->OnDeviceOpenFailed(request.request_id);
-      DVLOG(1) << "MediaStreamDispatcher::OnDeviceOpenFailed("
-               << request.request_id << ")\n";
+      if (request.handler) {
+        request.handler->OnDeviceOpenFailed(request.request_id);
+        DVLOG(1) << "MediaStreamDispatcher::OnDeviceOpenFailed("
+                 << request.request_id << ")\n";
+      }
       requests_.erase(it);
       break;
     }
diff --git a/content/renderer/media/media_stream_dispatcher.h b/content/renderer/media/media_stream_dispatcher.h
index 72dd02b..33bc954 100644
--- a/content/renderer/media/media_stream_dispatcher.h
+++ b/content/renderer/media/media_stream_dispatcher.h
@@ -34,7 +34,7 @@ class CONTENT_EXPORT MediaStreamDispatcher
   // Note: The event_handler must be valid for as long as the stream exists.
   virtual void GenerateStream(
       int request_id,
-      MediaStreamDispatcherEventHandler* event_handler,
+      const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
       media_stream::StreamOptions components,
       const std::string& security_origin);
 
@@ -42,17 +42,19 @@ class CONTENT_EXPORT MediaStreamDispatcher
   virtual void StopStream(const std::string& label);
 
   // Request to enumerate devices.
-  void EnumerateDevices(int request_id,
-                        MediaStreamDispatcherEventHandler* event_handler,
-                        media_stream::MediaStreamType type,
-                        const std::string& security_origin);
+  void EnumerateDevices(
+      int request_id,
+      const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
+      media_stream::MediaStreamType type,
+      const std::string& security_origin);
 
   // Request to open a device.
-  void OpenDevice(int request_id,
-                  MediaStreamDispatcherEventHandler* event_handler,
-                  const std::string& device_id,
-                  media_stream::MediaStreamType type,
-                  const std::string& security_origin);
+  void OpenDevice(
+      int request_id,
+      const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
+      const std::string& device_id,
+      media_stream::MediaStreamType type,
+      const std::string& security_origin);
 
   // Close a started device. |label| is provided in OnDeviceOpened.
   void CloseDevice(const std::string& label);
diff --git a/content/renderer/media/media_stream_dispatcher_unittest.cc b/content/renderer/media/media_stream_dispatcher_unittest.cc
index 6bf3454..bc14972 100644
--- a/content/renderer/media/media_stream_dispatcher_unittest.cc
+++ b/content/renderer/media/media_stream_dispatcher_unittest.cc
@@ -23,7 +23,8 @@ const int kRequestId3 = 30;
 const int kRequestId4 = 40;
 
 class MockMediaStreamDispatcherEventHandler
-    : public MediaStreamDispatcherEventHandler {
+    : public MediaStreamDispatcherEventHandler,
+      public base::SupportsWeakPtr<MockMediaStreamDispatcherEventHandler> {
  public:
   MockMediaStreamDispatcherEventHandler()
       : request_id_(-1),
@@ -94,12 +95,12 @@ TEST(MediaStreamDispatcherTest, BasicStream) {
   std::string security_origin;
 
   int ipc_request_id1 = dispatcher->next_ipc_id_;
-  dispatcher->GenerateStream(kRequestId1, handler.get(), components,
-                             security_origin);
+  dispatcher->GenerateStream(kRequestId1, handler.get()->AsWeakPtr(),
+                             components, security_origin);
   int ipc_request_id2 = dispatcher->next_ipc_id_;
   EXPECT_NE(ipc_request_id1, ipc_request_id2);
-  dispatcher->GenerateStream(kRequestId2, handler.get(), components,
-                             security_origin);
+  dispatcher->GenerateStream(kRequestId2, handler.get()->AsWeakPtr(),
+                             components, security_origin);
   EXPECT_EQ(dispatcher->requests_.size(), size_t(2));
 
   media_stream::StreamDeviceInfoArray audio_device_array(1);
@@ -168,12 +169,12 @@ TEST(MediaStreamDispatcherTest, BasicVideoDevice) {
   std::string security_origin;
 
   int ipc_request_id1 = dispatcher->next_ipc_id_;
-  dispatcher->EnumerateDevices(kRequestId1, handler.get(),
+  dispatcher->EnumerateDevices(kRequestId1, handler.get()->AsWeakPtr(),
                                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE,
                                security_origin);
   int ipc_request_id2 = dispatcher->next_ipc_id_;
   EXPECT_NE(ipc_request_id1, ipc_request_id2);
-  dispatcher->EnumerateDevices(kRequestId2, handler.get(),
+  dispatcher->EnumerateDevices(kRequestId2, handler.get()->AsWeakPtr(),
                                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE,
                                security_origin);
   EXPECT_EQ(dispatcher->requests_.size(), size_t(2));
@@ -201,13 +202,13 @@ TEST(MediaStreamDispatcherTest, BasicVideoDevice) {
   EXPECT_EQ(dispatcher->label_stream_map_.size(), size_t(0));
 
   int ipc_request_id3 = dispatcher->next_ipc_id_;
-  dispatcher->OpenDevice(kRequestId3, handler.get(),
+  dispatcher->OpenDevice(kRequestId3, handler.get()->AsWeakPtr(),
                          video_device_info.device_id,
                          content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE,
                          security_origin);
   int ipc_request_id4 = dispatcher->next_ipc_id_;
   EXPECT_NE(ipc_request_id3, ipc_request_id4);
-  dispatcher->OpenDevice(kRequestId4, handler.get(),
+  dispatcher->OpenDevice(kRequestId4, handler.get()->AsWeakPtr(),
                          video_device_info.device_id,
                          content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE,
                          security_origin);
@@ -258,8 +259,8 @@ TEST(MediaStreamDispatcherTest, TestFailure) {
 
   // Test failure when creating a stream.
   int ipc_request_id1 = dispatcher->next_ipc_id_;
-  dispatcher->GenerateStream(kRequestId1, handler.get(), components,
-                             security_origin);
+  dispatcher->GenerateStream(kRequestId1, handler.get()->AsWeakPtr(),
+                             components, security_origin);
   dispatcher->OnMessageReceived(MediaStreamMsg_StreamGenerationFailed(
       kRouteId, ipc_request_id1));
 
@@ -269,8 +270,8 @@ TEST(MediaStreamDispatcherTest, TestFailure) {
 
   // Create a new stream.
   ipc_request_id1 = dispatcher->next_ipc_id_;
-  dispatcher->GenerateStream(kRequestId1, handler.get(), components,
-                             security_origin);
+  dispatcher->GenerateStream(kRequestId1, handler.get()->AsWeakPtr(),
+                             components, security_origin);
 
   media_stream::StreamDeviceInfoArray audio_device_array(1);
   media_stream::StreamDeviceInfo audio_device_info;
diff --git a/content/renderer/media/media_stream_impl.cc b/content/renderer/media/media_stream_impl.cc
index 7d8c14a..072def1 100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -177,7 +177,7 @@ void MediaStreamImpl::requestUserMedia(
 
   media_stream_dispatcher_->GenerateStream(
       request_id,
-      this,
+      AsWeakPtr(),
       media_stream::StreamOptions(audio, video_option),
       security_origin);
 }
diff --git a/content/renderer/media/media_stream_impl.h b/content/renderer/media/media_stream_impl.h
index 250fb0b..7a59582 100644
--- a/content/renderer/media/media_stream_impl.h
+++ b/content/renderer/media/media_stream_impl.h
@@ -14,6 +14,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop_proxy.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/thread.h"
@@ -61,7 +62,8 @@ class CONTENT_EXPORT MediaStreamImpl
       NON_EXPORTED_BASE(public webkit_media::MediaStreamClient),
       public MediaStreamDispatcherEventHandler,
       NON_EXPORTED_BASE(public base::NonThreadSafe),
-      public base::RefCountedThreadSafe<MediaStreamImpl> {
+      public base::RefCountedThreadSafe<MediaStreamImpl>,
+      public base::SupportsWeakPtr<MediaStreamImpl> {
  public:
   MediaStreamImpl(
       MediaStreamDispatcher* media_stream_dispatcher,
diff --git a/content/renderer/media/mock_media_stream_dispatcher.cc b/content/renderer/media/mock_media_stream_dispatcher.cc
index 16894bf..4a85e39d 100644
--- a/content/renderer/media/mock_media_stream_dispatcher.cc
+++ b/content/renderer/media/mock_media_stream_dispatcher.cc
@@ -7,7 +7,6 @@
 MockMediaStreamDispatcher::MockMediaStreamDispatcher()
     : MediaStreamDispatcher(NULL),
       request_id_(-1),
-      event_handler_(NULL),
       components_(NULL),
       stop_stream_counter_(0) {
 }
@@ -16,7 +15,7 @@ MockMediaStreamDispatcher::~MockMediaStreamDispatcher() {}
 
 void MockMediaStreamDispatcher::GenerateStream(
     int request_id,
-    MediaStreamDispatcherEventHandler* event_handler,
+    const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
     media_stream::StreamOptions components,
     const std::string& security_origin) {
   request_id_ = request_id;
diff --git a/content/renderer/media/mock_media_stream_dispatcher.h b/content/renderer/media/mock_media_stream_dispatcher.h
index e712664..dcad0ee 100644
--- a/content/renderer/media/mock_media_stream_dispatcher.h
+++ b/content/renderer/media/mock_media_stream_dispatcher.h
@@ -15,10 +15,11 @@ class MockMediaStreamDispatcher : public MediaStreamDispatcher {
   MockMediaStreamDispatcher();
   virtual ~MockMediaStreamDispatcher();
 
-  virtual void GenerateStream(int request_id,
-                              MediaStreamDispatcherEventHandler* event_handler,
-                              media_stream::StreamOptions components,
-                              const std::string& security_origin) OVERRIDE;
+  virtual void GenerateStream(
+      int request_id,
+      const base::WeakPtr<MediaStreamDispatcherEventHandler>& event_handler,
+      media_stream::StreamOptions components,
+      const std::string& security_origin) OVERRIDE;
   virtual void StopStream(const std::string& label) OVERRIDE;
   virtual bool IsStream(const std::string& label) OVERRIDE;
   virtual int video_session_id(const std::string& label, int index) OVERRIDE;
@@ -34,7 +35,7 @@ class MockMediaStreamDispatcher : public MediaStreamDispatcher {
 
  private:
   int request_id_;
-  MediaStreamDispatcherEventHandler* event_handler_;
+  base::WeakPtr<MediaStreamDispatcherEventHandler> event_handler_;
   media_stream::StreamOptions* components_;
   std::string security_origin_;
   int stop_stream_counter_;
diff --git a/content/renderer/pepper/pepper_plugin_delegate_impl.cc b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
index cfea3d1..301add4 100644
--- a/content/renderer/pepper/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
@@ -1350,7 +1350,7 @@ int PepperPluginDelegateImpl::EnumerateDevices(
 
 #if defined(ENABLE_WEBRTC)
   render_view_->media_stream_dispatcher()->EnumerateDevices(
-      request_id, device_enumeration_event_handler_.get(),
+      request_id, device_enumeration_event_handler_.get()->AsWeakPtr(),
       PepperDeviceEnumerationEventHandler::FromPepperDeviceType(type), "");
 #else
   MessageLoop::current()->PostTask(
@@ -1538,7 +1538,9 @@ int PepperPluginDelegateImpl::OpenDevice(PP_DeviceType_Dev type,
 
 #if defined(ENABLE_WEBRTC)
   render_view_->media_stream_dispatcher()->OpenDevice(
-      request_id, device_enumeration_event_handler_.get(), device_id,
+      request_id,
+      device_enumeration_event_handler_.get()->AsWeakPtr(),
+      device_id,
       PepperDeviceEnumerationEventHandler::FromPepperDeviceType(type), "");
 #else
   MessageLoop::current()->PostTask(