[Cross-Site Isolation] Migrate entire MediaStream verticals to be per-RenderFrame.

chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc

Index: chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc
diff --git a/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc b/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc
index 1526b7d066201a084d9cfb1442b5235fb79253c4..55519e750dfc4ce73d20c455dfdf4a2f82a45624 100644
--- a/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc
+++ b/chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc
@@ -5,14 +5,9 @@
 #include "chrome/browser/extensions/api/tab_capture/tab_capture_registry.h"
 
 #include "base/lazy_instance.h"
-#include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/ui/fullscreen/fullscreen_controller.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/event_router.h"
@@ -28,6 +23,8 @@ namespace tab_capture = api::tab_capture;
 
 class FullscreenObserver : public content::WebContentsObserver {
  public:
+  // Caller of this constructor must guarantee that |request->target_contents|
+  // is a valid pointer (i.e., it will be dereferenced).

[ncarter (slow), 2014/07/10 01:17:51]

Just say: "... is not NULL". The rest is implicit.

[miu, 2014/07/10 22:16:11]

I think this was left over from earlier prototyping on my desktop.  Instead of
the comment, I've made the intention clear/documented in the form of a
DCHECK(web_contents()) in the ctor body.

   FullscreenObserver(TabCaptureRequest* request,
                      const TabCaptureRegistry* registry);
   virtual ~FullscreenObserver() {}
@@ -36,6 +33,7 @@ class FullscreenObserver : public content::WebContentsObserver {
   // content::WebContentsObserver implementation.
   virtual void DidShowFullscreenWidget(int routing_id) OVERRIDE;
   virtual void DidDestroyFullscreenWidget(int routing_id) OVERRIDE;
+  virtual void DidToggleFullscreenModeForTab(bool entered_fullscreen) OVERRIDE;
 
   TabCaptureRequest* request_;
   const TabCaptureRegistry* registry_;
@@ -45,15 +43,13 @@ class FullscreenObserver : public content::WebContentsObserver {
 
 // Holds all the state related to a tab capture stream.
 struct TabCaptureRequest {
-  TabCaptureRequest(int render_process_id,
-                    int render_view_id,
+  TabCaptureRequest(content::WebContents* target_contents,
                     const std::string& extension_id,
                     int tab_id,
                     TabCaptureState status);
   ~TabCaptureRequest();
 
-  const int render_process_id;
-  const int render_view_id;
+  content::WebContents* const target_contents;
   const std::string extension_id;
   const int tab_id;
   TabCaptureState status;
@@ -65,12 +61,9 @@ struct TabCaptureRequest {
 FullscreenObserver::FullscreenObserver(
     TabCaptureRequest* request,
     const TabCaptureRegistry* registry)
-    : request_(request),
+    : content::WebContentsObserver(request->target_contents),
+      request_(request),
       registry_(registry) {
-  content::RenderViewHost* const rvh =
-      content::RenderViewHost::FromID(request->render_process_id,
-                                      request->render_view_id);
-  Observe(rvh ? content::WebContents::FromRenderViewHost(rvh) : NULL);
 }
 
 void FullscreenObserver::DidShowFullscreenWidget(
@@ -85,18 +78,24 @@ void FullscreenObserver::DidDestroyFullscreenWidget(
   registry_->DispatchStatusChangeEvent(request_);
 }
 
+void FullscreenObserver::DidToggleFullscreenModeForTab(
+    bool entered_fullscreen) {
+  request_->fullscreen = entered_fullscreen;
+  registry_->DispatchStatusChangeEvent(request_);
+}
+
 TabCaptureRequest::TabCaptureRequest(
-    int render_process_id,
-    int render_view_id,
+    content::WebContents* target_contents,
     const std::string& extension_id,
     const int tab_id,
     TabCaptureState status)
-    : render_process_id(render_process_id),
-      render_view_id(render_view_id),
+    : target_contents(target_contents),
       extension_id(extension_id),
       tab_id(tab_id),
       status(status),
       last_status(status),
+      // TODO(miu): This initial value for |fullscreen| is a faulty assumption.
+      // http://crbug.com/350491
       fullscreen(false) {
 }
 
@@ -106,9 +105,6 @@ TabCaptureRequest::~TabCaptureRequest() {
 TabCaptureRegistry::TabCaptureRegistry(content::BrowserContext* context)
     : browser_context_(context), extension_registry_observer_(this) {
   MediaCaptureDevicesDispatcher::GetInstance()->AddObserver(this);
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_FULLSCREEN_CHANGED,
-                 content::NotificationService::AllSources());
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
@@ -143,44 +139,6 @@ const TabCaptureRegistry::RegistryCaptureInfo
   return list;
 }
 
-void TabCaptureRegistry::Observe(int type,
-                                 const content::NotificationSource& source,
-                                 const content::NotificationDetails& details) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(chrome::NOTIFICATION_FULLSCREEN_CHANGED, type);
-  FullscreenController* fullscreen_controller =
-      content::Source<FullscreenController>(source).ptr();
-  const bool is_fullscreen = *content::Details<bool>(details).ptr();
-  for (ScopedVector<TabCaptureRequest>::iterator it = requests_.begin();
-       it != requests_.end();
-       ++it) {
-    // If we are exiting fullscreen mode, we only need to check if any of
-    // the requests had the fullscreen flag toggled previously. The
-    // fullscreen controller no longer has the reference to the fullscreen
-    // web_contents here.
-    if (!is_fullscreen) {
-      if ((*it)->fullscreen) {
-        (*it)->fullscreen = false;
-        DispatchStatusChangeEvent(*it);
-        break;
-      }
-      continue;
-    }
-
-    // If we are entering fullscreen mode, find whether the web_contents we
-    // are capturing entered fullscreen mode.
-    content::RenderViewHost* const rvh = content::RenderViewHost::FromID(
-        (*it)->render_process_id, (*it)->render_view_id);
-    if (rvh &&
-        fullscreen_controller->IsFullscreenForTabOrPending(
-            content::WebContents::FromRenderViewHost(rvh))) {
-      (*it)->fullscreen = true;
-      DispatchStatusChangeEvent(*it);
-      break;
-    }
-  }
-}
-
 void TabCaptureRegistry::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
@@ -196,53 +154,64 @@ void TabCaptureRegistry::OnExtensionUnloaded(
   }
 }
 
-bool TabCaptureRegistry::AddRequest(int render_process_id,
-                                    int render_view_id,
+bool TabCaptureRegistry::AddRequest(content::WebContents* target_contents,
                                     const std::string& extension_id,
                                     int tab_id,
                                     TabCaptureState status) {
-  TabCaptureRequest* request = FindCaptureRequest(render_process_id,
-                                                  render_view_id);
+  TabCaptureRequest* const request = FindCaptureRequest(target_contents);
+
   // Currently, we do not allow multiple active captures for same tab.
   if (request != NULL) {
     if (request->status != tab_capture::TAB_CAPTURE_STATE_STOPPED &&
         request->status != tab_capture::TAB_CAPTURE_STATE_ERROR) {
       return false;
     } else {
-      DeleteCaptureRequest(render_process_id, render_view_id);
+      // Delete the request.
+      // TODO(miu): It's not clear that this code path will ever get executed
+      // and, in fact, it seems impossible that TabCaptureRequests will ever get
+      // deleted.  Probably the root cause of http://crbug.com/338445.

[ncarter (slow), 2014/07/10 01:17:51]

If the entries of |requests_| are not properly cleaned up, then the resulting
behavior is more of a problem after this CL. This is because an invalid
WebContents* pointer value in |request_| is unsafe in some situations where a
routing pair would be safe. Since they're never reused, routing pairs can be
reasoned about even after their referand is destroyed, but pointers carry no
such guarantee.

One way to possibly fix this, would be to have the FullscreenObserver override
WebContentsDestroyed, and pass the event on to the TabCaptureRegistry. This
might require installing the observer earlier, from the moment the request is
created, since there doesn't seem to be anything preventing a WebContents
disappearing between TabCaptureRegistry::AddRequest and
content::MEDIA_REQUEST_STATE_DONE . And if you're then always observing the
WebContents, there's nothing stopping you from merging TabCaptureRequest and
FullscreenObserver into a single class.

An alternative approach for handling this, would be to look up the WebContents
as needed, based on the tab_id.

[miu, 2014/07/10 22:16:11]

Done.

I used exactly your suggested approach of merging TabCaptureRequest with
FullscreenObserver into a new class called LiveRequest.  Then, there was a ton
of obvious clean-up exposed, which I took care of.  Also, I accidentally fixed a
number of bugs.  ;-)  Sorry about the huge patch delta, but I think the
improvement seems to warrant it.

Also, updated chrome/test/data/extensions/api_tests/tab_capture/api_test.js to
test that getCapturedTabs() works correctly since I fixed the "stale infos" bug
(crbug.com/338445).

[ncarter (slow), 2014/07/11 22:32:24]

Wonderful, glad it worked out.

+      for (ScopedVector<TabCaptureRequest>::iterator it = requests_.begin();
+           it != requests_.end(); ++it) {
+        if ((*it)->target_contents == target_contents) {
+          requests_.erase(it);
+          break;
+        }
+      }
     }
   }
 
-  requests_.push_back(new TabCaptureRequest(render_process_id,
-                                            render_view_id,
+  requests_.push_back(new TabCaptureRequest(target_contents,
                                             extension_id,
                                             tab_id,
                                             status));
   return true;
 }
 
-bool TabCaptureRegistry::VerifyRequest(int render_process_id,
-                                       int render_view_id) {
+bool TabCaptureRegistry::VerifyRequest(
+    const content::WebContents* target_contents,
+    const std::string& extension_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DVLOG(1) << "Verifying tabCapture request for "
-           << render_process_id << ":" << render_view_id;
-  // TODO(justinlin): Verify extension too.
-  return (FindCaptureRequest(render_process_id, render_view_id) != NULL);
+  TabCaptureRequest* const request = FindCaptureRequest(target_contents);
+  // TODO(miu): We should probably also verify the origin, like the desktop
+  // capture API.  http://crbug.com/163100
+  return request && request->extension_id == extension_id;
 }
 
 void TabCaptureRegistry::OnRequestUpdate(
     int render_process_id,
-    int render_view_id,
-    const content::MediaStreamDevice& device,
+    int render_frame_id,
+    content::MediaStreamType stream_type,
     const content::MediaRequestState new_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (device.type != content::MEDIA_TAB_VIDEO_CAPTURE &&
-      device.type != content::MEDIA_TAB_AUDIO_CAPTURE) {
+  if (stream_type != content::MEDIA_TAB_VIDEO_CAPTURE &&
+      stream_type != content::MEDIA_TAB_AUDIO_CAPTURE) {
     return;
   }
 
-  TabCaptureRequest* request = FindCaptureRequest(render_process_id,
-                                                  render_view_id);
+  TabCaptureRequest* const request = FindCaptureRequest(
+      content::WebContents::FromRenderFrameHost(
+          content::RenderFrameHost::FromID(
+              render_process_id, render_frame_id)));
   if (request == NULL) {
     // TODO(justinlin): This can happen because the extension's renderer does
     // not seem to always cleanup streams correctly.
@@ -328,27 +297,13 @@ void TabCaptureRegistry::DispatchStatusChangeEvent(
 }
 
 TabCaptureRequest* TabCaptureRegistry::FindCaptureRequest(
-    int render_process_id, int render_view_id) const {
+    const content::WebContents* target_contents) const {
   for (ScopedVector<TabCaptureRequest>::const_iterator it = requests_.begin();
        it != requests_.end(); ++it) {
-    if ((*it)->render_process_id == render_process_id &&
-        (*it)->render_view_id == render_view_id) {
+    if ((*it)->target_contents == target_contents)
       return *it;
-    }
   }
   return NULL;
 }
 
-void TabCaptureRegistry::DeleteCaptureRequest(int render_process_id,
-                                              int render_view_id) {
-  for (ScopedVector<TabCaptureRequest>::iterator it = requests_.begin();
-       it != requests_.end(); ++it) {
-    if ((*it)->render_process_id == render_process_id &&
-        (*it)->render_view_id == render_view_id) {
-      requests_.erase(it);
-      return;
-    }
-  }
-}
-
 }  // namespace extensions