When simulating a navigation via TestRenderViewHost, first start a provisional load before committin

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

Index: chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
index d26b7f996de8b022360ace6a88252e59cf8fca0c..c09dc03ba13a6d3279f26eb3aeff6ee7d58bac6c 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
@@ -236,7 +236,9 @@ void WebNavigationEventRouter::TabDestroyed(content::WebContents* tab) {
 
 WebNavigationTabObserver::WebNavigationTabObserver(
     content::WebContents* web_contents)
-    : WebContentsObserver(web_contents) {
+    : WebContentsObserver(web_contents),
+      render_view_host_(NULL),
+      pending_render_view_host_(NULL) {
   g_tab_observer.Get().insert(TabObserverMap::value_type(web_contents, this));
   registrar_.Add(this,
                  content::NOTIFICATION_RESOURCE_RECEIVED_REDIRECT,
@@ -283,6 +285,21 @@ void WebNavigationTabObserver::DidStartProvisionalLoadForFrame(
     const GURL& validated_url,
     bool is_error_page,
     content::RenderViewHost* render_view_host) {
+  if (render_view_host == pending_render_view_host_) {
+    DCHECK(is_main_frame);
+    // Suppress incorrect duplicate navigation signals triggered by the
+    // TransfernavigationResourceThrottle. http://crbug.com/137219
+    return;
+  }
+
+  if (render_view_host_ == NULL) {

[Charlie Reis, 2012/07/18 17:21:18]

nit: if (!render_view_host_) {

+    render_view_host_ = render_view_host;
+  } else if (render_view_host != render_view_host_) {
+    DCHECK(is_main_frame);
+    DCHECK(!pending_render_view_host_);

[Charlie Reis, 2012/07/18 17:21:18]

Are you sure DidCommit or DidFail is always going to be called before DidStart,
when rapid back/forward cross-process navigations are in progress?  I'd love for
this to be true, but I'm skeptical because it relies on messages from the
renderers, allowing for races.

For example, I wouldn't be surprised to see this check fail if you tried the
following:
1) Navigate to several 5 or so different sites in the same tab via the omnibox,
causing process swaps.
2) Keep at least one of the renderer processes alive by opening another tab in
the same process.  (Might need a targeted link or window.open.)
3) Hold the Back keyboard shortcut down to race past entries, then the Forward
keyboard shortcut.  Or generally try to go back/forward fast in a random manner.

I've found this tends to expose lots of races we might not have expected.

[jochen (gone - plz use gerrit), 2012/07/18 18:03:40]

No, I'm not sure.

However, wouldn't that mean that a web contents can have almost arbitrarily many
non-swapped out RVH attached?

[Charlie Reis, 2012/07/18 18:51:34]

Yes, I don't see why messages couldn't come in from any RVH in the history of
the tab.  Remember, messages from the renderer can be arbitrarily delayed based
on how heavily loaded the client is.  Is there anything that would prevent this?

+    pending_render_view_host_ = render_view_host;
+  }
+
   // Ignore navigations of sub frames, if the main frame isn't committed yet.
   // This might happen if a sub frame triggers a navigation for both the main
   // frame and itself. Since the sub frame is about to be deleted, and there's
@@ -310,6 +327,11 @@ void WebNavigationTabObserver::DidCommitProvisionalLoadForFrame(
     const GURL& url,
     content::PageTransition transition_type,
     content::RenderViewHost* render_view_host) {
+  DCHECK(render_view_host == render_view_host_ ||
+         (render_view_host == pending_render_view_host_ && is_main_frame));
+  render_view_host_ = render_view_host;
+  pending_render_view_host_ = NULL;
+
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
 
@@ -365,6 +387,11 @@ void WebNavigationTabObserver::DidFailProvisionalLoad(
     int error_code,
     const string16& error_description,
     content::RenderViewHost* render_view_host) {
+  DCHECK(render_view_host == render_view_host_ ||
+         (render_view_host == pending_render_view_host_ && is_main_frame));
+  if (render_view_host == pending_render_view_host_)
+    pending_render_view_host_ = NULL;
+
   if (!navigation_state_.CanSendEvents(frame_id))
     return;
   navigation_state_.SetErrorOccurredInFrame(frame_id);