Only delete old frames when a new main frame navigation commits

Also, use RVH* to identify frames, not the process id. when we run out of
processes, several unrelated RVHs might share a process.

BUG=136090,139117
TEST=browser_tests:*WebNavigation*, unit_tests:FrameNavigation*

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

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

chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc

@@ -26,30 +26,31 @@ const char* kValidSchemes[] = {
 
 FrameNavigationState::FrameID::FrameID()
     : frame_num(-1),
-      render_process_id(-1) {
+      render_view_host(NULL) {
 }
 
-FrameNavigationState::FrameID::FrameID(int64 frame_num,
-                                       int render_process_id)
+FrameNavigationState::FrameID::FrameID(
+    int64 frame_num,
+    content::RenderViewHost* render_view_host)
     : frame_num(frame_num),
-      render_process_id(render_process_id) {
+      render_view_host(render_view_host) {
 }
 
 bool FrameNavigationState::FrameID::IsValid() const {
-  return frame_num >= 0 && render_process_id >= 0;
+  return frame_num >= 0 && render_view_host;
 }
 
 bool FrameNavigationState::FrameID::operator<(
     const FrameNavigationState::FrameID& other) const {
   return frame_num < other.frame_num ||
       (frame_num == other.frame_num &&
-       render_process_id < other.render_process_id);
+       render_view_host < other.render_view_host);
 }
 
 bool FrameNavigationState::FrameID::operator==(
     const FrameNavigationState::FrameID& other) const {
   return frame_num == other.frame_num &&
-      render_process_id == other.render_process_id;
+      render_view_host == other.render_view_host;
 }
 
 bool FrameNavigationState::FrameID::operator!=(
@@ -91,10 +92,6 @@ void FrameNavigationState::TrackFrame(FrameID frame_id,
                                       const GURL& url,
                                       bool is_main_frame,
                                       bool is_error_page) {
-  if (is_main_frame) {
-    frame_state_map_.clear();
-    frame_ids_.clear();
-  }
   FrameState& frame_state = frame_state_map_[frame_id];
   frame_state.error_occurred = is_error_page;
   frame_state.url = url;
@@ -102,12 +99,26 @@ void FrameNavigationState::TrackFrame(FrameID frame_id,
   frame_state.is_navigating = true;
   frame_state.is_committed = false;
   frame_state.is_server_redirected = false;
-  if (is_main_frame) {
-    main_frame_id_ = frame_id;
-  }
   frame_ids_.insert(frame_id);
 }
 
+void FrameNavigationState::StopTrackingFramesInRVH(
+    content::RenderViewHost* render_view_host) {
+  for (std::set<FrameID>::iterator frame = frame_ids_.begin();
+       frame != frame_ids_.end();) {
+    if (frame->render_view_host != render_view_host) {
+      ++frame;
+      continue;
+    }
+    FrameID frame_id = *frame;
+    ++frame;
+    if (frame_id == main_frame_id_)
+      main_frame_id_ = FrameID();
+    frame_state_map_.erase(frame_id);
+    frame_ids_.erase(frame_id);
+  }
+}
+
 void FrameNavigationState::UpdateFrame(FrameID frame_id, const GURL& url) {
   FrameIdToStateMap::iterator frame_state = frame_state_map_.find(frame_id);
   if (frame_state == frame_state_map_.end()) {
@@ -134,7 +145,10 @@ GURL FrameNavigationState::GetUrl(FrameID frame_id) const {
 }
 
 bool FrameNavigationState::IsMainFrame(FrameID frame_id) const {
-  return main_frame_id_.IsValid() && main_frame_id_ == frame_id;
+  FrameIdToStateMap::const_iterator frame_state =
+      frame_state_map_.find(frame_id);
+  return (frame_state != frame_state_map_.end() &&
+          frame_state->second.is_main_frame);
 }
 
 FrameNavigationState::FrameID FrameNavigationState::GetMainFrameID() const {
@@ -168,6 +182,8 @@ bool FrameNavigationState::GetNavigationCompleted(FrameID frame_id) const {
 void FrameNavigationState::SetNavigationCommitted(FrameID frame_id) {
   DCHECK(frame_state_map_.find(frame_id) != frame_state_map_.end());
   frame_state_map_[frame_id].is_committed = true;
+  if (frame_state_map_[frame_id].is_main_frame)
+    main_frame_id_ = frame_id;
 }
 
 bool FrameNavigationState::GetNavigationCommitted(FrameID frame_id) const {

chrome/browser/extensions/api/web_navigation/frame_navigation_state.h

@@ -11,6 +11,10 @@
 #include "base/compiler_specific.h"
 #include "googleurl/src/gurl.h"
 
+namespace content {
+class RenderViewHost;
+}
+
 namespace extensions {
 
 // Tracks the navigation state of all frames in a given tab currently known to
@@ -18,12 +22,10 @@ namespace extensions {
 // occurred so no further events for this frame are being sent.
 class FrameNavigationState {
  public:
-  // A frame is uniquely identified by its frame ID and the render process ID.
-  // We use the render process ID instead of e.g. a pointer to the RVH so we
-  // don't depend on the lifetime of the RVH.
+  // A frame is uniquely identified by its frame ID and the RVH it's in.
   struct FrameID {
     FrameID();
-    FrameID(int64 frame_num, int render_process_id);
+    FrameID(int64 frame_num, content::RenderViewHost* render_view_host);
 
     bool IsValid() const;
 
@@ -32,7 +34,7 @@ class FrameNavigationState {
     bool operator!=(const FrameID& other) const;
 
     int64 frame_num;
-    int render_process_id;
+    content::RenderViewHost* render_view_host;
   };
   typedef std::set<FrameID>::const_iterator const_iterator;
 
@@ -55,6 +57,9 @@ class FrameNavigationState {
                   bool is_main_frame,
                   bool is_error_page);
 
+  // Stops tracking all frames for a given RenderViewHost.
+  void StopTrackingFramesInRVH(content::RenderViewHost* render_view_host);
+
   // Update the URL associated with a given frame.
   void UpdateFrame(FrameID frame_id, const GURL& url);
 
@@ -64,11 +69,12 @@ class FrameNavigationState {
   // Returns the URL corresponding to a tracked frame given by its |frame_id|.
   GURL GetUrl(FrameID frame_id) const;
 
-  // True if the frame given by its |frame_id| is the main frame of its tab.
+  // True if the frame given by its |frame_id| is a main frame of its tab.
+  // There might be multiple uncomitted main frames.
   bool IsMainFrame(FrameID frame_id) const;
 
-  // Returns the frame ID of the main frame, or -1 if the frame ID is not
-  // known.
+  // Returns the frame ID of the last comitted main frame, or -1 if the frame
+  // ID is not known.
   FrameID GetMainFrameID() const;
 
   // Marks a frame as in an error state, i.e. the onErrorOccurred event was
@@ -121,7 +127,7 @@ class FrameNavigationState {
   // Set of all known frames.
   std::set<FrameID> frame_ids_;
 
-  // The current main frame.
+  // The id of the last comitted main frame.
   FrameID main_frame_id_;
 
   // If true, also allow events from chrome-extension:// URLs.

chrome/browser/extensions/api/web_navigation/frame_navigation_state_unittest.cc

@@ -4,21 +4,20 @@
 
 #include "base/values.h"
 #include "chrome/browser/extensions/api/web_navigation/frame_navigation_state.h"
-#include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
-class FrameNavigationStateTest : public ChromeRenderViewHostTestHarness {
-};
+content::RenderViewHost* fake_rvh =
+    reinterpret_cast<content::RenderViewHost*>(31337);
 
 // Test that a frame is correctly tracked, and removed once the tab contents
 // goes away.
-TEST_F(FrameNavigationStateTest, TrackFrame) {
+TEST(FrameNavigationStateTest, TrackFrame) {
   FrameNavigationState navigation_state;
-  const FrameNavigationState::FrameID frame_id1(23, 1);
-  const FrameNavigationState::FrameID frame_id2(42, 1);
+  const FrameNavigationState::FrameID frame_id1(23, fake_rvh);
+  const FrameNavigationState::FrameID frame_id2(42, fake_rvh);
   const GURL url1("http://www.google.com/");
   const GURL url2("http://mail.google.com/");
 
@@ -26,6 +25,7 @@ TEST_F(FrameNavigationStateTest, TrackFrame) {
   EXPECT_FALSE(navigation_state.CanSendEvents(frame_id1));
   EXPECT_FALSE(navigation_state.IsValidFrame(frame_id1));
   navigation_state.TrackFrame(frame_id1, url1, true, false);
+  navigation_state.SetNavigationCommitted(frame_id1);
   EXPECT_TRUE(navigation_state.CanSendEvents(frame_id1));
   EXPECT_TRUE(navigation_state.IsValidFrame(frame_id1));
 
@@ -33,6 +33,7 @@ TEST_F(FrameNavigationStateTest, TrackFrame) {
   EXPECT_FALSE(navigation_state.CanSendEvents(frame_id2));
   EXPECT_FALSE(navigation_state.IsValidFrame(frame_id2));
   navigation_state.TrackFrame(frame_id2, url2, false, false);
+  navigation_state.SetNavigationCommitted(frame_id2);
   EXPECT_TRUE(navigation_state.CanSendEvents(frame_id2));
   EXPECT_TRUE(navigation_state.IsValidFrame(frame_id2));
 
@@ -42,13 +43,20 @@ TEST_F(FrameNavigationStateTest, TrackFrame) {
   EXPECT_FALSE(navigation_state.IsMainFrame(frame_id2));
   EXPECT_EQ(url2, navigation_state.GetUrl(frame_id2));
   EXPECT_EQ(frame_id1, navigation_state.GetMainFrameID());
+
+  // Drop the frames.
+  navigation_state.StopTrackingFramesInRVH(fake_rvh);
+  EXPECT_FALSE(navigation_state.CanSendEvents(frame_id1));
+  EXPECT_FALSE(navigation_state.IsValidFrame(frame_id1));
+  EXPECT_FALSE(navigation_state.CanSendEvents(frame_id2));
+  EXPECT_FALSE(navigation_state.IsValidFrame(frame_id2));
 }
 
 // Test that no events can be sent for a frame after an error occurred, but
 // before a new navigation happened in this frame.
-TEST_F(FrameNavigationStateTest, ErrorState) {
+TEST(FrameNavigationStateTest, ErrorState) {
   FrameNavigationState navigation_state;
-  const FrameNavigationState::FrameID frame_id(42, 1);
+  const FrameNavigationState::FrameID frame_id(42, fake_rvh);
   const GURL url("http://www.google.com/");
 
   navigation_state.TrackFrame(frame_id, url, true, false);
@@ -73,10 +81,10 @@ TEST_F(FrameNavigationStateTest, ErrorState) {
 
 // Tests that for a sub frame, no events are send after an error occurred, but
 // before a new navigation happened in this frame.
-TEST_F(FrameNavigationStateTest, ErrorStateFrame) {
+TEST(FrameNavigationStateTest, ErrorStateFrame) {
   FrameNavigationState navigation_state;
-  const FrameNavigationState::FrameID frame_id1(23, 1);
-  const FrameNavigationState::FrameID frame_id2(42, 1);
+  const FrameNavigationState::FrameID frame_id1(23, fake_rvh);
+  const FrameNavigationState::FrameID frame_id2(42, fake_rvh);
   const GURL url("http://www.google.com/");
 
   navigation_state.TrackFrame(frame_id1, url, true, false);
@@ -101,9 +109,9 @@ TEST_F(FrameNavigationStateTest, ErrorStateFrame) {
 }
 
 // Tests that no events are send for a not web-safe scheme.
-TEST_F(FrameNavigationStateTest, WebSafeScheme) {
+TEST(FrameNavigationStateTest, WebSafeScheme) {
   FrameNavigationState navigation_state;
-  const FrameNavigationState::FrameID frame_id(23, 1);
+  const FrameNavigationState::FrameID frame_id(23, fake_rvh);
   const GURL url("unsafe://www.google.com/");
 
   navigation_state.TrackFrame(frame_id, url, true, false);

chrome/browser/extensions/api/web_navigation/web_navigation_api.cc

@@ -182,7 +182,7 @@ void WebNavigationEventRouter::Retargeting(const RetargetingDetails* details) {
 
   FrameNavigationState::FrameID frame_id(
       details->source_frame_id,
-      details->source_web_contents->GetRenderViewHost()->GetProcess()->GetID());
+      details->source_web_contents->GetRenderViewHost());
   if (!frame_navigation_state.CanSendEvents(frame_id))
     return;
 
@@ -248,6 +248,9 @@ WebNavigationTabObserver::WebNavigationTabObserver(
   registrar_.Add(this,
                  content::NOTIFICATION_RESOURCE_RECEIVED_REDIRECT,
                  content::Source<content::WebContents>(web_contents));
+  registrar_.Add(this,
+                 content::NOTIFICATION_RENDER_VIEW_HOST_DELETED,
+                 content::NotificationService::AllSources());
 }
 
 WebNavigationTabObserver::~WebNavigationTabObserver() {}
@@ -259,6 +262,19 @@ WebNavigationTabObserver* WebNavigationTabObserver::Get(
   return i == g_tab_observer.Get().end() ? NULL : i->second;
 }
 
+content::RenderViewHost* WebNavigationTabObserver::GetRenderViewHostInProcess(
+    int process_id) const {
+  if (render_view_host_ &&
+      render_view_host_->GetProcess()->GetID() == process_id) {
+    return render_view_host_;
+  }
+  if (pending_render_view_host_ &&
+      pending_render_view_host_->GetProcess()->GetID() == process_id) {
+    return pending_render_view_host_;
+  }
+  return NULL;
+}
+
 void WebNavigationTabObserver::Observe(
     int type,
     const content::NotificationSource& source,
@@ -271,9 +287,20 @@ void WebNavigationTabObserver::Observe(
           resource_redirect_details->resource_type;
       if (resource_type == ResourceType::MAIN_FRAME ||
           resource_type == ResourceType::SUB_FRAME) {
+        content::RenderViewHost* render_view_host = NULL;
+        if (render_view_host_ &&
+            resource_redirect_details->origin_child_id ==
+                render_view_host_->GetProcess()->GetID()) {
+          render_view_host = render_view_host_;
+        } else if (pending_render_view_host_ &&
+                   resource_redirect_details->origin_child_id ==
+                       pending_render_view_host_->GetProcess()->GetID()) {
+          render_view_host = pending_render_view_host_;
+        }
+        if (!render_view_host)
+          return;
         FrameNavigationState::FrameID frame_id(
-            resource_redirect_details->frame_id,
-            resource_redirect_details->origin_child_id);
+            resource_redirect_details->frame_id, render_view_host);
         if (!navigation_state_.CanSendEvents(frame_id))
           return;
         navigation_state_.SetIsServerRedirected(frame_id);
@@ -281,6 +308,19 @@ void WebNavigationTabObserver::Observe(
       break;
     }
 
+    case content::NOTIFICATION_RENDER_VIEW_HOST_DELETED: {
+      content::RenderViewHost* render_view_host =
+          content::Source<content::RenderViewHost>(source).ptr();
+      if (render_view_host == render_view_host_)
+        render_view_host_ = NULL;
+      else if (render_view_host == pending_render_view_host_)
+        pending_render_view_host_ = NULL;
+      else
+        return;
+      SendErrorEvents(web_contents(), render_view_host);
+      break;
+    }
+
     default:
       NOTREACHED();
   }
@@ -291,8 +331,8 @@ void WebNavigationTabObserver::AboutToNavigateRenderView(
   if (!render_view_host_) {
     render_view_host_ = render_view_host;
   } else if (render_view_host != render_view_host_) {
-    // TODO(jochen): If pending_render_view_host_ is non-NULL, send error events
-    // for all ongoing navigations in that RVH.
+    if (pending_render_view_host_)
+      SendErrorEvents(web_contents(), pending_render_view_host_);
     pending_render_view_host_ = render_view_host;
   }
 }
@@ -309,8 +349,7 @@ void WebNavigationTabObserver::DidStartProvisionalLoadForFrame(
       render_view_host != pending_render_view_host_)
     return;
 
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
 
   navigation_state_.TrackFrame(frame_id,
                                validated_url,
@@ -333,13 +372,12 @@ void WebNavigationTabObserver::DidCommitProvisionalLoadForFrame(
   if (render_view_host != render_view_host_ &&
       render_view_host != pending_render_view_host_)
     return;
-  // TODO(jochen): If we switched the RVH, send error events for all ongoing
-  // navigations in the old RVH.
+  if (render_view_host != render_view_host_)
+    SendErrorEvents(web_contents(), render_view_host_);
   render_view_host_ = render_view_host;
   pending_render_view_host_ = NULL;
 
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
 
@@ -398,18 +436,21 @@ void WebNavigationTabObserver::DidFailProvisionalLoad(
   if (render_view_host != render_view_host_ &&
       render_view_host != pending_render_view_host_)
     return;
-  if (render_view_host == pending_render_view_host_)
+  bool stop_tracking_frames = false;
+  if (render_view_host == pending_render_view_host_) {
     pending_render_view_host_ = NULL;
+    stop_tracking_frames = true;
+  }
 
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
-  if (!navigation_state_.CanSendEvents(frame_id))
-    return;
-
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
+  if (navigation_state_.CanSendEvents(frame_id)) {
     navigation_state_.SetErrorOccurredInFrame(frame_id);
     helpers::DispatchOnErrorOccurred(
         web_contents(), render_view_host->GetProcess()->GetID(), validated_url,
         frame_num, is_main_frame, error_code);
+  }
+  if (stop_tracking_frames)
+    navigation_state_.StopTrackingFramesInRVH(render_view_host);
 }
 
 void WebNavigationTabObserver::DocumentLoadedInFrame(
@@ -417,8 +458,7 @@ void WebNavigationTabObserver::DocumentLoadedInFrame(
     content::RenderViewHost* render_view_host) {
   if (render_view_host != render_view_host_)
     return;
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   helpers::DispatchOnDOMContentLoaded(web_contents(),
@@ -434,8 +474,7 @@ void WebNavigationTabObserver::DidFinishLoad(
     content::RenderViewHost* render_view_host) {
   if (render_view_host != render_view_host_)
     return;
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   navigation_state_.SetNavigationCompleted(frame_id);
@@ -456,8 +495,7 @@ void WebNavigationTabObserver::DidFailLoad(
     content::RenderViewHost* render_view_host) {
   if (render_view_host != render_view_host_)
     return;
-  FrameNavigationState::FrameID frame_id(
-      frame_num, render_view_host->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   navigation_state_.SetErrorOccurredInFrame(frame_id);
@@ -473,8 +511,7 @@ void WebNavigationTabObserver::DidOpenRequestedURL(
     WindowOpenDisposition disposition,
     content::PageTransition transition,
     int64 source_frame_num) {
-  FrameNavigationState::FrameID frame_id(
-      source_frame_num, render_view_host_->GetProcess()->GetID());
+  FrameNavigationState::FrameID frame_id(source_frame_num, render_view_host_);
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
 
@@ -499,19 +536,29 @@ void WebNavigationTabObserver::DidOpenRequestedURL(
 
 void WebNavigationTabObserver::WebContentsDestroyed(content::WebContents* tab) {
   g_tab_observer.Get().erase(tab);
+  registrar_.RemoveAll();
+  SendErrorEvents(tab, NULL);
+}
+
+void WebNavigationTabObserver::SendErrorEvents(
+    content::WebContents* web_contents,
+    content::RenderViewHost* render_view_host) {
   for (FrameNavigationState::const_iterator frame = navigation_state_.begin();
        frame != navigation_state_.end(); ++frame) {
     if (!navigation_state_.GetNavigationCompleted(*frame) &&
-        navigation_state_.CanSendEvents(*frame)) {
+        navigation_state_.CanSendEvents(*frame) &&
+        (!render_view_host || frame->render_view_host == render_view_host)) {
       helpers::DispatchOnErrorOccurred(
-        tab,
-        frame->render_process_id,
+        web_contents,
+        frame->render_view_host->GetProcess()->GetID(),
         navigation_state_.GetUrl(*frame),
         frame->frame_num,
         navigation_state_.IsMainFrame(*frame),
         net::ERR_ABORTED);
     }
   }
+  if (render_view_host)
+    navigation_state_.StopTrackingFramesInRVH(render_view_host);
 }
 
 // See also NavigationController::IsURLInPageNavigation.
@@ -559,7 +606,12 @@ bool GetFrameFunction::RunImpl() {
   if (frame_id == 0)
     frame_id = frame_navigation_state.GetMainFrameID().frame_num;
 
-  FrameNavigationState::FrameID internal_frame_id(frame_id, process_id);
+  content::RenderViewHost* render_view_host =
+      observer->GetRenderViewHostInProcess(process_id);
+  if (!render_view_host)
+    return true;
+
+  FrameNavigationState::FrameID internal_frame_id(frame_id, render_view_host);
   if (!frame_navigation_state.IsValidFrame(internal_frame_id))
     return true;
 
@@ -613,7 +665,7 @@ bool GetAllFramesFunction::RunImpl() {
     frame->url = frame_url.spec();
     frame->frame_id = helpers::GetFrameId(
         navigation_state.IsMainFrame(frame_id), frame_id.frame_num);
-    frame->process_id = frame_id.render_process_id;
+    frame->process_id = frame_id.render_view_host->GetProcess()->GetID();
     frame->error_occurred = navigation_state.GetErrorOccurredInFrame(frame_id);
     result_list.push_back(frame);
   }

chrome/browser/extensions/api/web_navigation/web_navigation_api.h

@@ -40,6 +40,8 @@ class WebNavigationTabObserver : public content::NotificationObserver,
     return navigation_state_;
   }
 
+  content::RenderViewHost* GetRenderViewHostInProcess(int process_id) const;
+
   // content::NotificationObserver implementation.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
@@ -97,6 +99,12 @@ class WebNavigationTabObserver : public content::NotificationObserver,
   bool IsReferenceFragmentNavigation(FrameNavigationState::FrameID frame_id,
                                      const GURL& url);
 
+  // Creates and sends onErrorOccurred events for all on-going navigations. If
+  // |render_view_host| is non-NULL, only generates events for frames in this
+  // render view host.
+  void SendErrorEvents(content::WebContents* web_contents,
+                       content::RenderViewHost* render_view_host);
+
   // Tracks the state of the frames we are sending events for.
   FrameNavigationState navigation_state_;
 

chrome/test/data/extensions/api_test/webnavigation/test_getFrame.html

@@ -8,7 +8,7 @@ chrome.test.runTests([
     chrome.tabs.create({"url": "about:blank"}, function(tab) {
       tabId = tab.id;
       var done = chrome.test.listenForever(
-        chrome.webNavigation.onBeforeNavigate,
+        chrome.webNavigation.onCommitted,
         function (details) {
           if (details.tabId != tabId)
             return;