Another attempt at fixing dead frames being tracked by webNavigation

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Implements the Chrome Extensions WebNavigation API.
 
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
 
 #include "base/lazy_instance.h"
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api_constants.h"
@@ skipping 359 matching lines @@
 void WebNavigationTabObserver::DidCommitProvisionalLoadForFrame(
     int64 frame_num,
     bool is_main_frame,
     const GURL& url,
     content::PageTransition transition_type,
     content::RenderViewHost* render_view_host) {
   if (render_view_host != render_view_host_ &&
       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);
 
   if (is_reference_fragment_navigation) {
     helpers::DispatchOnCommitted(
         keys::kOnReferenceFragmentUpdated,
         web_contents(),
         frame_num,
         is_main_frame,
@@ skipping 273 matching lines @@
         navigation_state.IsMainFrame(frame_id), frame_id.frame_num);
     frame->process_id = frame_id.render_view_host->GetProcess()->GetID();
     frame->error_occurred = navigation_state.GetErrorOccurredInFrame(frame_id);
     result_list.push_back(frame);
   }
   results_ = GetAllFrames::Results::Create(result_list);
   return true;
 }
 
 }  // namespace extensions