Fix flaky ExtensionWebRequestApiTest.WebRequestTypes

This started flaking after WebRequest was moved to UI in
http://crrev.com/c/1682434. Previously, we got lucky that the beacon
requests from unloaded frames came in before the FrameData was removed
from the frame data map, since WebRequest could grab the FrameData on IO
before the beacon request started.

This CL stores recently deleted frames, and posts a task for the removal
of the deleted frame's FrameData so we have access to it when the beacon
request comes in.

Bug: 522129, 980774
Change-Id: I1fa0d16d52c52cf61d1095588882170b48ab3e32
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1713864
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680134}

chrome/browser/extensions/web_contents_browsertest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
@@ -148,7 +149,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, FrameDataCached) {
 
   // Returns the cached frame data for |rfh|.
   auto get_frame_data = [](content::RenderFrameHost* rfh) {
-    return ExtensionApiFrameIdMap::Get()->GetFrameData(rfh);
+    return ExtensionApiFrameIdMap::Get()->GetFrameData(
+        rfh->GetProcess()->GetID(), rfh->GetRoutingID());
   };
 
   // Adds an iframe with the given |name| and |src| to the given |web_contents|

chrome/test/base/in_process_browser_test.cc

@@ -106,10 +106,6 @@
 #include "ui/views/test/test_desktop_screen_x11.h"
 #endif
 
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-#include "extensions/browser/extension_api_frame_id_map.h"
-#endif
-
 #if defined(TOOLKIT_VIEWS)
 #include "chrome/test/views/accessibility_checker.h"
 #include "ui/views/views_delegate.h"
@@ -316,14 +312,6 @@ void InProcessBrowserTest::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
-
 #if defined(OS_CHROMEOS)
   chromeos::device_sync::DeviceSyncImpl::Factory::SetInstanceForTesting(
       nullptr);

extensions/browser/api/web_request/web_request_api.cc

@@ -1462,9 +1462,8 @@ bool ExtensionWebRequestEventRouter::DispatchEvent(
 
   // TODO(http://crbug.com/980774): Investigate if this is necessary.
   if (!request->frame_data) {
-    content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
+    request->frame_data = ExtensionApiFrameIdMap::Get()->GetFrameData(
         request->render_process_id, request->frame_id);
-    request->frame_data = ExtensionApiFrameIdMap::Get()->GetFrameData(rfh);
   }
   event_details->SetFrameData(request->frame_data.value());
   DispatchEventToListeners(browser_context, std::move(listeners_to_dispatch),

extensions/browser/api/web_request/web_request_event_details.cc

@@ -141,10 +141,9 @@ void WebRequestEventDetails::SetFrameData(
 
 void WebRequestEventDetails::DetermineFrameDataOnUI() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  content::RenderFrameHost* rfh =
-      content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
   ExtensionApiFrameIdMap::FrameData frame_data =
-      ExtensionApiFrameIdMap::Get()->GetFrameData(rfh);
+      ExtensionApiFrameIdMap::Get()->GetFrameData(render_process_id_,
+                                                  render_frame_id_);
   SetFrameData(frame_data);
 }
 

extensions/browser/api/web_request/web_request_info.cc

@@ -213,8 +213,8 @@ void WebRequestInfoInitParams::InitializeWebViewAndFrameData(
     }
 
     // For subresource loads we attempt to resolve the FrameData immediately.
-    frame_data = ExtensionApiFrameIdMap::Get()->GetFrameData(
-        content::RenderFrameHost::FromID(render_process_id, frame_id));
+    frame_data = ExtensionApiFrameIdMap::Get()->GetFrameData(render_process_id,
+                                                             frame_id);
   }
 }
 

extensions/browser/extension_api_frame_id_map.cc

@@ -188,36 +188,35 @@ ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::KeyToValue(
 }
 
 ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::LookupFrameDataOnUI(
-    const RenderFrameIdKey& key) {
+    const RenderFrameIdKey& key,
+    bool check_deleted_frames) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  bool lookup_successful = false;
-  FrameData data;
   FrameDataMap::const_iterator frame_id_iter = frame_data_map_.find(key);
-  if (frame_id_iter != frame_data_map_.end()) {
-    lookup_successful = true;
-    data = frame_id_iter->second;
-  } else {
-    data = KeyToValue(key);
-    // Don't save invalid values in the map.
-    if (data.frame_id != kInvalidFrameId) {
-      lookup_successful = true;
-      auto kvpair = FrameDataMap::value_type(key, data);
-      frame_data_map_.insert(kvpair);
-    }
+
+  if (frame_id_iter != frame_data_map_.end())
+    return frame_id_iter->second;
+
+  if (check_deleted_frames) {
+    frame_id_iter = deleted_frame_data_map_.find(key);
+    if (frame_id_iter != deleted_frame_data_map_.end())
+      return frame_id_iter->second;
   }
+
+  FrameData data = KeyToValue(key);
+  // Don't save invalid values in the map.
+  if (data.frame_id != kInvalidFrameId)
+    frame_data_map_.insert({key, data});
+
   return data;
 }
 
 ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::GetFrameData(
-    content::RenderFrameHost* rfh) {
+    int render_process_id,
+    int render_frame_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (!rfh)
-    return FrameData();
-
-  const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
-  return LookupFrameDataOnUI(key);
+  const RenderFrameIdKey key(render_process_id, render_frame_id);
+  return LookupFrameDataOnUI(key, true /* check_deleted_frames */);
 }
 
 void ExtensionApiFrameIdMap::InitializeRenderFrameData(
@@ -227,21 +226,33 @@ void ExtensionApiFrameIdMap::InitializeRenderFrameData(
   DCHECK(rfh->IsRenderFrameLive());
 
   const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
-  CacheFrameData(key);
+  LookupFrameDataOnUI(key, false /* check_deleted_frames */);
   DCHECK(frame_data_map_.find(key) != frame_data_map_.end());
 }
 
-void ExtensionApiFrameIdMap::CacheFrameData(const RenderFrameIdKey& key) {
-  LookupFrameDataOnUI(key);
-}
-
 void ExtensionApiFrameIdMap::OnRenderFrameDeleted(
     content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(rfh);
 
   const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
-  RemoveFrameData(key);
+  // TODO(http://crbug.com/522129): This is necessary right now because beacon
+  // requests made in window.onunload may start after this has been called.
+  // Delay the RemoveFrameData() call, so we will still have the frame data
+  // cached when the beacon request comes in.
+  auto iter = frame_data_map_.find(key);
+  if (iter == frame_data_map_.end())
+    return;
+
+  deleted_frame_data_map_.insert({key, iter->second});
+  frame_data_map_.erase(key);
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](ExtensionApiFrameIdMap* self, const RenderFrameIdKey& key) {
+            self->deleted_frame_data_map_.erase(key);
+          },
+          base::Unretained(this), key));
 }
 
 void ExtensionApiFrameIdMap::UpdateTabAndWindowId(
@@ -350,10 +361,4 @@ size_t ExtensionApiFrameIdMap::GetFrameDataCountForTesting() const {
   return frame_data_map_.size();
 }
 
-void ExtensionApiFrameIdMap::RemoveFrameData(const RenderFrameIdKey& key) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  frame_data_map_.erase(key);
-}
-
 }  // namespace extensions

extensions/browser/extension_api_frame_id_map.h

@@ -123,10 +123,12 @@ class ExtensionApiFrameIdMap {
       content::WebContents* web_contents,
       int frame_id);
 
-  // Retrieves the FrameData for a given |rfh|. The map may be updated with the
-  // result if the map did not contain the FrameData before the lookup.
-  // If |rfh| is nullptr, then the map is not modified.
-  FrameData GetFrameData(content::RenderFrameHost* rfh) WARN_UNUSED_RESULT;
+  // Retrieves the FrameData for a given |render_process_id| and
+  // |render_frame_id|. The map may be updated with the result if the map did
+  // not contain the FrameData before the lookup. If a RenderFrameHost is not
+  // found, then the map is not modified.
+  FrameData GetFrameData(int render_process_id,
+                         int render_frame_id) WARN_UNUSED_RESULT;
 
   // Initializes the FrameData for the given |rfh|.
   void InitializeRenderFrameData(content::RenderFrameHost* rfh);
@@ -199,13 +201,10 @@ class ExtensionApiFrameIdMap {
   virtual FrameData KeyToValue(const RenderFrameIdKey& key) const;
 
   // Looks up the data for the given |key| and adds it to the |frame_data_map_|.
-  FrameData LookupFrameDataOnUI(const RenderFrameIdKey& key);
-
-  // Implementation of CacheFrameData(RenderFrameHost), separated for testing.
-  void CacheFrameData(const RenderFrameIdKey& key);
-
-  // Implementation of RemoveFrameData(RenderFrameHost), separated for testing.
-  void RemoveFrameData(const RenderFrameIdKey& key);
+  // If |check_deleted_frames| is true, |deleted_frame_data_map_| will also be
+  // used.
+  FrameData LookupFrameDataOnUI(const RenderFrameIdKey& key,
+                                bool check_deleted_frames);
 
   std::unique_ptr<ExtensionApiFrameIdMapHelper> helper_;
 
@@ -213,6 +212,11 @@ class ExtensionApiFrameIdMap {
   // TODO(http://crbug.com/980774): Investigate if this is still needed.
   FrameDataMap frame_data_map_;
 
+  // Holds mappings of render frame key to FrameData from frames that have been
+  // recently deleted. These are kept for a short time so beacon requests that
+  // continue after a frame is unloaded can access the FrameData.
+  FrameDataMap deleted_frame_data_map_;
+
   // The set of pending main frame navigations for which ReadyToCommitNavigation
   // has been fired. Only used on the UI thread. This is needed to clear state
   // set up in OnMainFrameReadyToCommitNavigation for navigations which