Fix how captive portals track which page is loading.

chrome/browser/captive_portal/captive_portal_tab_helper.cc

Index: chrome/browser/captive_portal/captive_portal_tab_helper.cc
===================================================================
--- chrome/browser/captive_portal/captive_portal_tab_helper.cc	(revision 151564)
+++ chrome/browser/captive_portal/captive_portal_tab_helper.cc	(working copy)
@@ -14,9 +14,13 @@
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_request_details.h"
+#include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
 
 namespace captive_portal {
@@ -34,13 +38,16 @@
       login_detector_(new CaptivePortalLoginDetector(profile)),
       profile_(profile),
       pending_error_code_(net::OK),
-      provisional_main_frame_id_(-1) {
+      provisional_render_view_host_(NULL) {
   registrar_.Add(this,
                  chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
                  content::Source<Profile>(profile_));
   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());
 }
 
 CaptivePortalTabHelper::~CaptivePortalTabHelper() {
@@ -58,18 +65,17 @@
   if (!is_main_frame)
     return;
 
-  provisional_main_frame_id_ = frame_id;
+  if (provisional_render_view_host_) {
+    // If loading an error page for a previous failure, treat this as part of
+    // the previous load.  Link Doctor pages act like error page loads in a

[cbentzel, 2012/08/15 17:00:38]

"Link Doctor pages act like error page loads in a row" is unclear to me - is
this supposed to be "act like two error page loads in a row"?

[mmenke, 2012/08/16 14:30:03]

Correct.  "two" added

+    // row.  The second time, provisional_render_view_host_ will be NULL.

[cbentzel, 2012/08/15 17:00:38]

Why will provisional_render_view_host_ be NULL? Will it commit the first error
page?

Is the sequence:

  - provisional load on original page
  - provisional load of first error page
  - commit of first error page
  - provisional load of link doctor page
  - commit of link doctor page

[mmenke, 2012/08/16 14:30:03]

It's:

- provisional load
- provisional load fails  // I get the error code here
- provisional load of first error page (is_error_page == true)
- commit of first error page  // I use the error code here, and set
provisional_render_view_host_ to NULL (And clear the error code).
- provisional load of link doctor page (is_error_page == true)
- commit of link doctor page

+    if (is_error_page && provisional_render_view_host_ == render_view_host)
+      return;
+    // Otherwise, abort the old load.
+    tab_reloader_->OnAbort();
+  }
 
-  // If loading an error page for a previous failure, treat this as part of
-  // the previous load.  The second check is needed because Link Doctor pages
-  // result in two error page provisional loads in a row.  Currently, the
-  // second load is treated as a normal load, rather than reusing old error
-  // codes.
-  if (is_error_page && pending_error_code_ != net::OK)
-    return;
-
-  // Makes the second load for Link Doctor pages act as a normal load.
-  // TODO(mmenke):  Figure out if this affects any other cases.
+  provisional_render_view_host_ = render_view_host;
   pending_error_code_ = net::OK;
 
   tab_reloader_->OnLoadStart(validated_url.SchemeIsSecure());
@@ -87,9 +93,20 @@
   if (!is_main_frame)
     return;
 
-  provisional_main_frame_id_ = -1;
+  if (provisional_render_view_host_ == render_view_host) {
+    tab_reloader_->OnLoadCommitted(pending_error_code_);
+  } else {
+    // This may happen if the active RenderView commits a page before a cross
+    // process navigation cancels the old load.  In this case, the commit of the
+    // old navigation will cancel the newer one.
+    tab_reloader_->OnAbort();
 
-  tab_reloader_->OnLoadCommitted(pending_error_code_);
+    // Send information about the new load.

[cbentzel, 2012/08/15 17:00:38]

Why do you need to do this? Just for stats?

[mmenke, 2012/08/16 14:30:03]

To make TabReloader's interface a bit better defined:
TabReloader::OnLoadCommitted corresponds to real commits.  It has no other
purpose.  If you prefer, I can document the omission and remove this code.

[cbentzel, 2012/08/16 19:15:04]

I think this is fine.

+    tab_reloader_->OnLoadStart(url.SchemeIsSecure());
+    tab_reloader_->OnLoadCommitted(net::OK);
+  }
+
+  provisional_render_view_host_ = NULL;
   pending_error_code_ = net::OK;
 }
 
@@ -102,15 +119,15 @@
     content::RenderViewHost* render_view_host) {
   DCHECK(CalledOnValidThread());
 
-  // Ignore subframes.
-  if (!is_main_frame)
+  // Ignore subframes and unexpected RenderViewHosts.
+  if (!is_main_frame || render_view_host != provisional_render_view_host_)
     return;
 
-  provisional_main_frame_id_ = -1;
-
   // Aborts generally aren't followed by loading an error page, so go ahead and
   // reset the state now, to prevent any captive portal checks from triggering.
   if (error_code == net::ERR_ABORTED) {
+    // No further messages for the cancelled navigation will occur.
+    provisional_render_view_host_ = NULL;
     // May have been aborting the load of an error page.
     pending_error_code_ = net::OK;
 
@@ -133,33 +150,52 @@
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(CalledOnValidThread());
-  if (type == content::NOTIFICATION_RESOURCE_RECEIVED_REDIRECT) {
-    DCHECK_EQ(web_contents(),
-              content::Source<content::WebContents>(source).ptr());
+  switch (type) {
+    case content::NOTIFICATION_RESOURCE_RECEIVED_REDIRECT: {
+      DCHECK_EQ(web_contents(),
+                content::Source<content::WebContents>(source).ptr());
 
-    const content::ResourceRedirectDetails* redirect_details =
-        content::Details<content::ResourceRedirectDetails>(details).ptr();
+      const content::ResourceRedirectDetails* redirect_details =
+          content::Details<content::ResourceRedirectDetails>(details).ptr();
 
-    if (redirect_details->resource_type == ResourceType::MAIN_FRAME)
-      OnRedirect(redirect_details->frame_id, redirect_details->new_url);
-  } else if (type == chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT) {
-    DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
+      OnRedirect(redirect_details->origin_child_id,
+                 redirect_details->resource_type,
+                 redirect_details->new_url);
+      break;
+    }
+    case chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT: {
+      DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
 
-    const CaptivePortalService::Results* results =
-        content::Details<CaptivePortalService::Results>(details).ptr();
+      const CaptivePortalService::Results* results =
+          content::Details<CaptivePortalService::Results>(details).ptr();
 
-    OnCaptivePortalResults(results->previous_result, results->result);
-  } else {
-    NOTREACHED();
+      OnCaptivePortalResults(results->previous_result, results->result);
+      break;
+    }
+    case content::NOTIFICATION_RENDER_VIEW_HOST_DELETED: {
+      content::RenderViewHost* render_view_host =
+          content::Source<content::RenderViewHost>(source).ptr();
+      // This can happen when a cross-process navigation is aborted, either by
+      // pressing stop or by starting a new cross-process navigation that can't
+      // re-use |provisional_render_view_host_|.  May also happen on a crash.
+      if (render_view_host == provisional_render_view_host_) {
+        provisional_render_view_host_ = NULL;

[cbentzel, 2012/08/15 17:00:38]

Should you share this logic with the error_code == net::ERR_ABORTED case in
DidFailProvisionalLoad()?

[mmenke, 2012/08/16 14:30:03]

Done.  Went ahead and used the same function for the other two abort cases as
well, though they don't actually need the extra two lines, they don't hurt.

+        pending_error_code_ = net::OK;
+        tab_reloader_->OnAbort();
+      }
+      break;
+    }
+    default:
+      NOTREACHED();
   }
 }
 
-void CaptivePortalTabHelper::OnRedirect(int64 frame_id, const GURL& new_url) {
-  // If the main frame's not currently loading, or the redirect is for some
-  // other frame, ignore the redirect.  It's unclear if |frame_id| can ever be
-  // -1 ("invalid/unknown"), but best to be careful.
-  if (provisional_main_frame_id_ == -1 ||
-      provisional_main_frame_id_ != frame_id) {
+void CaptivePortalTabHelper::OnRedirect(int child_id,
+                                        ResourceType::Type resource_type,
+                                        const GURL& new_url) {
+  // Only main frame redirects for the provisional RenderViewHost matter.
+  if (resource_type != ResourceType::MAIN_FRAME ||

[cbentzel, 2012/08/15 17:00:38]

Would child_id ever be -1 here?

[mmenke, 2012/08/16 14:30:03]

Since the id is actually used to lookup the RenderViewHost and send us the
message, I don't see how it could be.  That having been said, I've inlined
GetProvisionalChildID, since it's no longer needed.

+      child_id != GetProvisionalChildID()) {
     return;
   }
 
@@ -214,4 +250,10 @@
   tab_contents->captive_portal_tab_helper()->SetIsLoginTab();
 }
 
+int CaptivePortalTabHelper::GetProvisionalChildID() const {
+  if (!provisional_render_view_host_)
+    return -1;
+  return provisional_render_view_host_->GetProcess()->GetID();
+}
+
 }  // namespace captive_portal