Another attempt at fixing dead frames being tracked by webNavigation

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 2e0b3c628a37ee585eb2561fe61c5c4d76396df6..3b7d348eba50e9ca354ef7289553c1c503673764 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
@@ -377,22 +377,31 @@ void WebNavigationTabObserver::DidCommitProvisionalLoadForFrame(
       render_view_host != pending_render_view_host_)
     return;
   FrameNavigationState::FrameID frame_id(frame_num, render_view_host);
-  FrameNavigationState::FrameID id_to_skip;
-  if (render_view_host == render_view_host_)
-    id_to_skip = frame_id;
-  if (is_main_frame)
-    SendErrorEvents(web_contents(), render_view_host_, id_to_skip);
-  render_view_host_ = render_view_host;
-  pending_render_view_host_ = NULL;
-
-  if (!navigation_state_.CanSendEvents(frame_id))
-    return;
 
   bool is_reference_fragment_navigation =
       IsReferenceFragmentNavigation(frame_id, url);
   bool is_history_navigation =
       navigation_state_.GetNavigationCommitted(frame_id);
 
+  if (is_main_frame && render_view_host_ == render_view_host) {

[Matt Perry, 2012/08/06 20:00:51]

this branch confuses to me. how could the current rvh have a main frame
navigation? shouldn't it have been pending first?

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

pending means that it's in a different process. This mirrors the naming of the
RenderViewHostManager

[Matt Perry, 2012/08/06 21:16:26]

Actually, the RVHM uses "pending" to refer to a navigation that has begun but
not been committed yet. Navigations are always pending before they commit.

Your logic in AboutToNavigateRenderView seems wrong in this regard. It should
always assign to pending_render_view_host_.

Maybe this confusion is why you and creis seem to disagree on when navigations
are cancelled.

[Charlie Reis, 2012/08/06 21:30:26]

Pending is ambiguous here.  NavigationController has "pending entries" for all
browser-initiated navigations (cross-process or same-process), but RVHM only has
"pending RVHs" for cross-process navigations.  In that sense, Jochen's name
agrees with RVHM.

[jochen (gone - plz use gerrit), 2012/08/06 21:42:32]

I added a comment to the defintion in the header.

+    if (!is_reference_fragment_navigation && !is_history_navigation)

[Charlie Reis, 2012/08/06 20:04:13]

Please add comments here, since this code is difficult to understand without
them.  Are you saying that we don't need to notify others about subframes in
these two cases because those loads will proceed?  That makes sense to me for
reference fragments but not history navigations.  And how does pushState fit
into this?

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

I added a comment and clarified the variable name: if you add a history entry
via history.pushState or replaceState, no navigation is started (there's just a
commit signal, but no load start signal), and iframe navigations aren't
cancelled either.

+      SendErrorEvents(web_contents(), render_view_host_, frame_id);
+    if (pending_render_view_host_) {

[Matt Perry, 2012/08/06 20:00:51]

is it ever possible for pending_rvh_ to be the same as rvh_ here?

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

No (see above)

+      SendErrorEvents(web_contents(),
+                      pending_render_view_host_,
+                      FrameNavigationState::FrameID());
+      pending_render_view_host_ = NULL;
+    }
+  } else if (pending_render_view_host_ == render_view_host) {
+    SendErrorEvents(
+        web_contents(), render_view_host_, FrameNavigationState::FrameID());
+    render_view_host_ = pending_render_view_host_;
+    pending_render_view_host_ = NULL;
+  }
+
+  if (!navigation_state_.CanSendEvents(frame_id))
+    return;
+
   // Update the URL as it might have changed.
   navigation_state_.UpdateFrame(frame_id, url);
   navigation_state_.SetNavigationCommitted(frame_id);