Extensions: Ensure Devtools frames are correctly tracked by ExtensionWebContentsObserver.

Currently ExtensionWebContentsObserver(EWCO) does not get RenderFrameCreated
notification for the Devtools main frame. This happens because the frame is
created before the EWCO construction. Also, at the time of
ExtensionWebContentsObserver construction, this frame happens to be the
speculative RenderFrameHost of the root FrameTreeNode and hence is not
enumerated via WebContents::ForEachFrame. To fix this, also track the new hosts
seen through the WebContentsObserver::RenderFrameHostChanged method. Add a
browser test for the same.

Also, rename ExtensionAPIFrameIdMap::OnRenderFrameCreated to
InitializeRenderFrameHost to clarify the new usage.

BUG=817075

Change-Id: I81213f0441162a483fda9191991763a3fee65905
Reviewed-on: https://chromium-review.googlesource.com/942684
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540978}

chrome/browser/extensions/web_contents_browsertest.cc

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "base/strings/stringprintf.h"
+#include "chrome/browser/devtools/devtools_window.h"
+#include "chrome/browser/devtools/devtools_window_testing.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -49,6 +51,39 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WebContents) {
   EXPECT_TRUE(result);
 }
 
+// Test that the frame data for the Devtools main frame is cached. Regression
+// test for crbug.com/817075.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, DevToolsMainFrameIsCached) {
+  auto test_devtools_main_frame_cached = [](Browser* browser, bool is_docked) {
+    SCOPED_TRACE(base::StringPrintf("Testing a %s devtools window",
+                                    is_docked ? "docked" : "undocked"));
+    const auto* api_frame_id_map = extensions::ExtensionApiFrameIdMap::Get();
+    size_t prior_count = api_frame_id_map->GetFrameDataCountForTesting();
+
+    // Open a devtools window.
+    DevToolsWindow* devtools_window =
+        DevToolsWindowTesting::OpenDevToolsWindowSync(browser, is_docked);
+
+    // Ensure that frame data for its main frame is cached.
+    content::WebContents* devtools_web_contents =
+        DevToolsWindow::GetInTabWebContents(
+            devtools_window->GetInspectedWebContents(), nullptr);
+    ASSERT_TRUE(devtools_web_contents);
+    EXPECT_TRUE(api_frame_id_map->HasCachedFrameDataForTesting(
+        devtools_web_contents->GetMainFrame()));
+    EXPECT_GT(api_frame_id_map->GetFrameDataCountForTesting(), prior_count);
+
+    DevToolsWindowTesting::CloseDevToolsWindowSync(devtools_window);
+
+    // Ensure that the frame data for the devtools main frame, which might have
+    // been destroyed by now, is deleted.
+    EXPECT_EQ(prior_count, api_frame_id_map->GetFrameDataCountForTesting());
+  };
+
+  test_devtools_main_frame_cached(browser(), true /*is_docked*/);
+  test_devtools_main_frame_cached(browser(), false /*is_docked*/);
+}
+
 // Test that we cache frame data for all frames on creation. Regression test for
 // crbug.com/810614.
 IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, FrameDataCached) {

extensions/browser/extension_api_frame_id_map.cc

@@ -316,9 +316,11 @@ ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::GetFrameData(
   return LookupFrameDataOnUI(key, false /* is_from_io */);
 }
 
-void ExtensionApiFrameIdMap::OnRenderFrameCreated(
+void ExtensionApiFrameIdMap::InitializeRenderFrameData(
     content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(rfh);
+  DCHECK(rfh->IsRenderFrameLive());
 
   const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
   CacheFrameData(key);
@@ -351,6 +353,7 @@ void ExtensionApiFrameIdMap::UpdateTabAndWindowId(
     iter->second.tab_id = tab_id;
     iter->second.window_id = window_id;
   } else {
+    DCHECK(!rfh->IsRenderFrameLive());
     // TODO(crbug.com/817205): Remove this branch. We should only maintain frame
     // data for tracked live render frames. Most probably this is causing a
     // memory leak.
@@ -369,6 +372,11 @@ bool ExtensionApiFrameIdMap::HasCachedFrameDataForTesting(
   return frame_data_map_.find(key) != frame_data_map_.end();
 }
 
+size_t ExtensionApiFrameIdMap::GetFrameDataCountForTesting() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return frame_data_map_.size();
+}
+
 void ExtensionApiFrameIdMap::RemoveFrameData(const RenderFrameIdKey& key) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 

extensions/browser/extension_api_frame_id_map.h

@@ -124,9 +124,8 @@ class ExtensionApiFrameIdMap {
   // If |rfh| is nullptr, then the map is not modified.
   FrameData GetFrameData(content::RenderFrameHost* rfh) WARN_UNUSED_RESULT;
 
-  // Called when a render frame is created. Caches the FrameData for the given
-  // render frame.
-  void OnRenderFrameCreated(content::RenderFrameHost* rfh);
+  // Initializes the FrameData for the given |rfh|.
+  void InitializeRenderFrameData(content::RenderFrameHost* rfh);
 
   // Called when a render frame is deleted. Removes the FrameData mapping for
   // the given render frame.
@@ -140,6 +139,8 @@ class ExtensionApiFrameIdMap {
   // Returns whether frame data for |rfh| is cached.
   bool HasCachedFrameDataForTesting(content::RenderFrameHost* rfh) const;
 
+  size_t GetFrameDataCountForTesting() const;
+
  protected:
   friend struct base::LazyInstanceTraitsBase<ExtensionApiFrameIdMap>;
 

extensions/browser/extension_web_contents_observer.cc

@@ -91,7 +91,7 @@ void ExtensionWebContentsObserver::RenderFrameCreated(
   // Optimization: Look up the extension API frame ID to force the mapping to be
   // cached. This minimizes the number of IO->UI->IO thread hops when the ID is
   // looked up again on the IO thread for the webRequest API.
-  ExtensionApiFrameIdMap::Get()->OnRenderFrameCreated(render_frame_host);
+  ExtensionApiFrameIdMap::Get()->InitializeRenderFrameData(render_frame_host);
 
   const Extension* extension = GetExtensionFromFrame(render_frame_host, false);
   if (!extension)
@@ -136,6 +136,17 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
   ExtensionApiFrameIdMap::Get()->OnRenderFrameDeleted(render_frame_host);
 }
 
+void ExtensionWebContentsObserver::RenderFrameHostChanged(
+    content::RenderFrameHost* old_host,
+    content::RenderFrameHost* new_host) {
+  // TODO(karandeepb): The |new_host| here may correspond to a RenderFrameHost
+  // we haven't seen yet, which means it might also need some other
+  // initialization. See crbug.com/817205.
+  if (new_host->IsRenderFrameLive()) {
+    ExtensionApiFrameIdMap::Get()->InitializeRenderFrameData(new_host);
+  }
+}
+
 void ExtensionWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->HasCommitted())
@@ -253,7 +264,7 @@ void ExtensionWebContentsObserver::InitializeFrameHelper(
   if (render_frame_host->IsRenderFrameLive()) {
     // Initialize the FrameData for this frame here since we didn't receive the
     // RenderFrameCreated notification for it.
-    ExtensionApiFrameIdMap::Get()->OnRenderFrameCreated(render_frame_host);
+    ExtensionApiFrameIdMap::Get()->InitializeRenderFrameData(render_frame_host);
     InitializeRenderFrame(render_frame_host);
   }
 }

extensions/browser/extension_web_contents_observer.h

@@ -83,6 +83,8 @@ class ExtensionWebContentsObserver
   // content::WebContentsObserver overrides.
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameHostChanged(content::RenderFrameHost* old_host,
+                              content::RenderFrameHost* new_host) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;