Reland "Extensions: Fix memory leak in ExtensionApiFrameIdMap."

This is a reland of 613ba331. This collided with
r541891.

Original change's description:
> Extensions: Fix memory leak in ExtensionApiFrameIdMap.
>
> Currently in ExtensionApiFrameIdMap::UpdateTabAndWindowId and
> ExtensionApiFrameIdMap::LookupFrameDataOnUI we don't check whether a
> RenderFrameHost is alive, before storing it's frame data. It's possible that
> such a RenderFrameHost never becomes alive, and hence we don't get any
> OnRenderFrameDeleted notification for it. Since ExtensionApiFrameIdMap is a
> singleton, this causes a memory leak. To fix, ensure that only live
> RenderFrameHosts are tracked.
>
> Also ensure in InProcessBrowserTest::TearDown that ExtensionApiFrameIdMap is not
> leaking memory for any browser test.
>
> BUG=817205
>
> Change-Id: I78bf06cee7e10465f9eb5973ee8e59f525a74d93
> Reviewed-on: https://chromium-review.googlesource.com/951975
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#541950}

TBR=sky@chromium.org,rdevlin.cronin@chromium.org

Bug: 817205
Change-Id: I7b3ac6806d0c747d439e74f80f623a7fd4412050
Reviewed-on: https://chromium-review.googlesource.com/956762
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542025}

chrome/test/BUILD.gn

@@ -4413,6 +4413,7 @@ if (!is_android) {
       "//components/nacl/common:buildflags",
       "//components/os_crypt:test_support",
       "//content/public/browser:browser",
+      "//extensions/buildflags",
       "//skia",
       "//testing/gtest",
       "//third_party/WebKit/public:blink_headers",
@@ -4424,6 +4425,10 @@ if (!is_android) {
         "ppapi/ppapi_test.h",
       ]
     }
+
+    if (enable_extensions) {
+      deps += [ "//extensions/browser" ]
+    }
   }
 
   import("//third_party/protobuf/proto_library.gni")

chrome/test/base/in_process_browser_test.cc

@@ -60,6 +60,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_launcher.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "extensions/buildflags/buildflags.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/display/display_switches.h"
 
@@ -91,6 +92,10 @@
 #include "ui/views/test/test_desktop_screen_x11.h"
 #endif
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/browser/extension_api_frame_id_map.h"
+#endif
+
 namespace {
 
 // Passed as value of kTestType.
@@ -286,6 +291,14 @@ void InProcessBrowserTest::TearDown() {
   BrowserTestBase::TearDown();
   OSCryptMocker::TearDown();
   ChromeContentBrowserClient::SetDefaultQuotaSettingsForTesting(nullptr);
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  // By now, all the WebContents should be destroyed, Ensure that we are not
+  // leaking memory in ExtensionAPIFrameIdMap. crbug.com/817205.
+  EXPECT_EQ(
+      0u,
+      extensions::ExtensionApiFrameIdMap::Get()->GetFrameDataCountForTesting());
+#endif
 }
 
 void InProcessBrowserTest::CloseBrowserSynchronously(Browser* browser) {

extensions/browser/extension_api_frame_id_map.cc

@@ -162,6 +162,10 @@ ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::KeyToValue(
     const RenderFrameIdKey& key) const {
   content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
       key.render_process_id, key.frame_routing_id);
+
+  if (!rfh || !rfh->IsRenderFrameLive())
+    return FrameData();
+
   int tab_id = extension_misc::kUnknownTabId;
   int window_id = extension_misc::kUnknownWindowId;
   if (helper_)
@@ -347,19 +351,18 @@ void ExtensionApiFrameIdMap::UpdateTabAndWindowId(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(rfh);
   const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
+
+  // Only track FrameData for live render frames.
+  if (!rfh->IsRenderFrameLive()) {
+    return;
+  }
+
   base::AutoLock lock(frame_data_map_lock_);
   FrameDataMap::iterator iter = frame_data_map_.find(key);
-  if (iter != frame_data_map_.end()) {
-    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.
-    frame_data_map_[key] =
-        FrameData(GetFrameId(rfh), GetParentFrameId(rfh), tab_id, window_id);
-  }
+  // The FrameData for |rfh| should have already been initialized.
+  DCHECK(iter != frame_data_map_.end());
+  iter->second.tab_id = tab_id;
+  iter->second.window_id = window_id;
 }
 
 bool ExtensionApiFrameIdMap::HasCachedFrameDataForTesting(

extensions/browser/extension_api_frame_id_map.h

@@ -131,7 +131,7 @@ class ExtensionApiFrameIdMap {
   // the given render frame.
   void OnRenderFrameDeleted(content::RenderFrameHost* rfh);
 
-  // Updates the tab and window id for the given RenderFrameHost, if any exists.
+  // Updates the tab and window id for the given RenderFrameHost if necessary.
   void UpdateTabAndWindowId(int tab_id,
                             int window_id,
                             content::RenderFrameHost* rfh);
@@ -179,8 +179,9 @@ class ExtensionApiFrameIdMap {
   virtual ~ExtensionApiFrameIdMap();
 
   // Determines the value to be stored in |frame_data_map_| for a given key.
-  // This method is only called when |key| is not in |frame_data_map_|.
-  // virtual for testing.
+  // Returns empty FrameData when the corresponding RenderFrameHost is not
+  // alive. This method is only called when |key| is not in |frame_data_map_|.
+  // Virtual for testing.
   virtual FrameData KeyToValue(const RenderFrameIdKey& key) const;
 
   // Looks up the data for the given |key| and adds it to the |frame_data_map_|.