Fix how captive portals track which page is loading.

The old code incorrectly assumed RenderView frame IDs
were globally unique, and used them to track main frame
reloads.

The corrected version tracks the provisional RenderViewHost
instead.

The new code also will now never send two LoadStart events
to the CaptivePortalReloader without an intervening load
committed/aborted event.

R=cbentzel@chromium.org
BUG=87100,115487
Review URL: https://chromiumcodereview.appspot.com/10837146

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

chrome/browser/captive_portal/captive_portal_browsertest.cc

@@ -88,6 +88,11 @@ const char* const kMockCaptivePortalTestUrl =
 const char* const kMockHttpsUrl =
     "https://mock.captive.portal.long.timeout/title2.html";
 
+// Same as above, but different domain, so can be used to trigger cross-site
+// navigations.
+const char* const kMockHttpsUrl2 =
+    "https://mock.captive.portal.long.timeout2/title2.html";
+
 // Same as kMockHttpsUrl, except the timeout happens instantly.
 const char* const kMockHttpsQuickTimeoutUrl =
     "https://mock.captive.portal.quick.timeout/title2.html";
@@ -377,6 +382,8 @@ void URLRequestMockCaptivePortalJobFactory::AddUrlHandlersOnIOThread() {
                         URLRequestMockCaptivePortalJobFactory::Factory);
   filter->AddUrlHandler(GURL(kMockHttpsUrl),
                         URLRequestMockCaptivePortalJobFactory::Factory);
+  filter->AddUrlHandler(GURL(kMockHttpsUrl2),
+                        URLRequestMockCaptivePortalJobFactory::Factory);
   filter->AddUrlHandler(GURL(kMockHttpsQuickTimeoutUrl),
                         URLRequestMockCaptivePortalJobFactory::Factory);
 }
@@ -398,7 +405,8 @@ net::URLRequestJob* URLRequestMockCaptivePortalJobFactory::Factory(
   FilePath root_http;
   PathService::Get(chrome::DIR_TEST_DATA, &root_http);
 
-  if (request->url() == GURL(kMockHttpsUrl)) {
+  if (request->url() == GURL(kMockHttpsUrl) ||
+      request->url() == GURL(kMockHttpsUrl2)) {
     if (behind_captive_portal_)
       return new URLRequestTimeoutOnDemandJob(request);
     // Once logged in to the portal, HTTPS requests return the page that was
@@ -876,6 +884,24 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   // active tab.  There should be no timed out tabs when this is called.
   void FailLoadsWithoutLogin(Browser* browser, int num_loading_tabs);
 
+  // Navigates |browser|'s active tab to |starting_url| while not behind a
+  // captive portal.  Then navigates to |interrupted_url|, which should create
+  // a URLRequestTimeoutOnDemandJob, which is then abandoned.  The load should
+  // trigger a captive portal check, which finds a captive portal and opens a
+  // tab.
+  //
+  // Then the navigation is interrupted by a navigation to |timeout_url|, which
+  // should trigger a captive portal check, and finally the test simulates
+  // logging in.
+  //
+  // The purpose of this test is to make sure the TabHelper triggers a captive
+  // portal check when a load is interrupted by another load, particularly in
+  // the case of cross-process navigations.
+  void RunNavigateLoadingTabToTimeoutTest(Browser* browser,
+                                          const GURL& starting_url,
+                                          const GURL& interrupted_url,
+                                          const GURL& timeout_url);
+
   // Sets the timeout used by a CaptivePortalTabReloader on slow SSL loads
   // before a captive portal check.
   void SetSlowSSLLoadTime(CaptivePortalTabReloader* tab_reloader,
@@ -1405,6 +1431,68 @@ void CaptivePortalBrowserTest::FailLoadsWithoutLogin(Browser* browser,
                    chrome::GetWebContentsAt(browser, login_tab)));
 }
 
+void CaptivePortalBrowserTest::RunNavigateLoadingTabToTimeoutTest(
+    Browser* browser,
+    const GURL& starting_url,
+    const GURL& hanging_url,
+    const GURL& timeout_url) {
+  // Temporarily disable the captive portal and navigate to the starting
+  // URL, which may be a URL that will hang when behind a captive portal.
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(false);
+  NavigateToPageExpectNoTest(browser, starting_url, 1);
+  URLRequestMockCaptivePortalJobFactory::SetBehindCaptivePortal(true);
+
+  // Go to the first hanging url.
+  SlowLoadBehindCaptivePortal(browser, true, hanging_url);
+
+  // Abandon the request.
+  URLRequestTimeoutOnDemandJob::WaitForJobs(1);
+  URLRequestTimeoutOnDemandJob::AbandonJobs(1);
+
+  CaptivePortalTabReloader* tab_reloader =
+      GetTabReloader(chrome::GetTabContentsAt(browser, 0));
+  ASSERT_TRUE(tab_reloader);
+
+  // A non-zero delay makes it more likely that CaptivePortalTabHelper will
+  // be confused by events relating to canceling the old navigation.
+  SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromSeconds(2));
+  CaptivePortalObserver portal_observer(browser->profile());
+
+  // Navigate the error tab to another slow loading page.  Can't have
+  // ui_test_utils do the navigation because it will wait for loading tabs to
+  // stop loading before navigating.
+  //
+  // This may result in either 0 or 1 DidStopLoading events.  If there is one,
+  // it must happen before the CaptivePortalService sends out its test request,
+  // so waiting for PortalObserver to see that request prevents it from
+  // confusing the MultiNavigationObservers used later.
+  chrome::ActivateTabAt(browser, 0, true);
+  browser->OpenURL(content::OpenURLParams(timeout_url,
+                                          content::Referrer(),
+                                          CURRENT_TAB,
+                                          content::PAGE_TRANSITION_TYPED,
+                                          false));
+  portal_observer.WaitForResults(1);
+  EXPECT_FALSE(CheckPending(browser));
+  EXPECT_EQ(1, NumLoadingTabs());
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_BROKEN_BY_PORTAL,
+            GetStateOfTabReloaderAt(browser, 0));
+  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
+            GetStateOfTabReloader(chrome::GetTabContentsAt(browser, 1)));
+  ASSERT_TRUE(IsLoginTab(chrome::GetTabContentsAt(browser, 1)));
+
+  // Need to make sure the request has been issued before logging in.
+  URLRequestTimeoutOnDemandJob::WaitForJobs(1);
+
+  // Simulate logging in.
+  chrome::ActivateTabAt(browser, 1, true);
+  SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromDays(1));
+  Login(browser, 1, 0);
+
+  // Timeout occurs, and page is automatically reloaded.
+  FailLoadsAfterLogin(browser, 1);
+}
+
 void CaptivePortalBrowserTest::SetSlowSSLLoadTime(
     CaptivePortalTabReloader* tab_reloader,
     base::TimeDelta slow_ssl_load_time) {
@@ -1685,54 +1773,38 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest, NavigateBrokenTab) {
   Login(browser(), 0, 0);
 }
 
-// Navigates a broken, but still loading, tab to another such page before
-// logging in.
+// Checks that captive portal detection triggers correctly when a same-site
+// navigation is cancelled by a navigation to the same site.
 IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
-                       NavigateBrokenToTimeoutTabWhileLoading) {
-  // Go to the error page.
-  SlowLoadBehindCaptivePortal(browser(), true);
-
-  // Abandon the request.
-  URLRequestTimeoutOnDemandJob::WaitForJobs(1);
-  URLRequestTimeoutOnDemandJob::AbandonJobs(1);
-
-  CaptivePortalTabReloader* tab_reloader =
-      GetTabReloader(chrome::GetTabContentsAt(browser(), 0));
-  ASSERT_TRUE(tab_reloader);
-  SetSlowSSLLoadTime(tab_reloader, base::TimeDelta());
-
-  CaptivePortalObserver portal_observer(browser()->profile());
-  MultiNavigationObserver navigation_observer;
-
-  // Navigate the error tab to another slow loading page.  Can't have
-  // ui_test_utils do the navigation because it will wait for loading tabs to
-  // stop loading before navigating.
-  chrome::ActivateTabAt(browser(), 0, true);
-  browser()->OpenURL(content::OpenURLParams(GURL(kMockHttpsUrl),
-                                            content::Referrer(),
-                                            CURRENT_TAB,
-                                            content::PAGE_TRANSITION_TYPED,
-                                            false));
-  navigation_observer.WaitForNavigations(1);
-  portal_observer.WaitForResults(1);
-  EXPECT_FALSE(CheckPending(browser()));
-  EXPECT_EQ(1, NumLoadingTabs());
-  EXPECT_EQ(CaptivePortalTabReloader::STATE_BROKEN_BY_PORTAL,
-            GetStateOfTabReloaderAt(browser(), 0));
-  EXPECT_EQ(CaptivePortalTabReloader::STATE_NONE,
-            GetStateOfTabReloader(chrome::GetTabContentsAt(browser(), 1)));
-  ASSERT_TRUE(IsLoginTab(chrome::GetTabContentsAt(browser(), 1)));
-
-  // Need to make sure the request has been issued before logging in.
-  URLRequestTimeoutOnDemandJob::WaitForJobs(1);
+                       NavigateLoadingTabToTimeoutSingleSite) {
+  RunNavigateLoadingTabToTimeoutTest(
+      browser(),
+      GURL(kMockHttpsUrl),
+      GURL(kMockHttpsUrl),
+      GURL(kMockHttpsUrl));
+}
 
-  // Simulate logging in.
-  chrome::ActivateTabAt(browser(), 1, true);
-  SetSlowSSLLoadTime(tab_reloader, base::TimeDelta::FromDays(1));
-  Login(browser(), 1, 0);
+// Checks that captive portal detection triggers correctly when a same-site
+// navigation is cancelled by a navigation to another site.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       NavigateLoadingTabToTimeoutTwoSites) {
+  RunNavigateLoadingTabToTimeoutTest(
+      browser(),
+      GURL(kMockHttpsUrl),
+      GURL(kMockHttpsUrl),
+      GURL(kMockHttpsUrl2));
+}
 
-  // Timeout occurs, and page is automatically reloaded.
-  FailLoadsAfterLogin(browser(), 1);
+// Checks that captive portal detection triggers correctly when a cross-site
+// navigation is cancelled by a navigation to yet another site.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBrowserTest,
+                       NavigateLoadingTabToTimeoutThreeSites) {
+  RunNavigateLoadingTabToTimeoutTest(
+      browser(),
+      URLRequestMockHTTPJob::GetMockUrl(
+          FilePath(FILE_PATH_LITERAL("title.html"))),
+      GURL(kMockHttpsUrl),
+      GURL(kMockHttpsUrl2));
 }
 
 // Checks that navigating a timed out tab back clears its state.

chrome/browser/captive_portal/captive_portal_tab_helper.cc

@@ -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 @@ CaptivePortalTabHelper::CaptivePortalTabHelper(
       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 @@ void CaptivePortalTabHelper::DidStartProvisionalLoadForFrame(
   if (!is_main_frame)
     return;
 
-  provisional_main_frame_id_ = frame_id;
-
-  // 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;
+  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 two error page loads in a
+    // row.  The second time, provisional_render_view_host_ will be NULL.
+    if (is_error_page && provisional_render_view_host_ == render_view_host)
+      return;
+    // Otherwise, abort the old load.
+    OnLoadAborted();
+  }
 
-  // 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 @@ void CaptivePortalTabHelper::DidCommitProvisionalLoadForFrame(
   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.
+    OnLoadAborted();
+
+    // Send information about the new load.
+    tab_reloader_->OnLoadStart(url.SchemeIsSecure());
+    tab_reloader_->OnLoadCommitted(net::OK);
+  }
 
-  tab_reloader_->OnLoadCommitted(pending_error_code_);
+  provisional_render_view_host_ = NULL;
   pending_error_code_ = net::OK;
 }
 
@@ -102,19 +119,14 @@ void CaptivePortalTabHelper::DidFailProvisionalLoad(
     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) {
-    // May have been aborting the load of an error page.
-    pending_error_code_ = net::OK;
-
-    tab_reloader_->OnAbort();
+    OnLoadAborted();
     return;
   }
 
@@ -133,33 +145,54 @@ void CaptivePortalTabHelper::Observe(
     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());
-
-    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());
-
-    const CaptivePortalService::Results* results =
-        content::Details<CaptivePortalService::Results>(details).ptr();
-
-    OnCaptivePortalResults(results->previous_result, results->result);
-  } else {
-    NOTREACHED();
+  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();
+
+      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();
+
+      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_)
+        OnLoadAborted();
+      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) {
+bool CaptivePortalTabHelper::IsLoginTab() const {
+  return login_detector_->is_login_tab();
+}
+
+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 ||
+      !provisional_render_view_host_ ||
+      provisional_render_view_host_->GetProcess()->GetID() != child_id) {
     return;
   }
 
@@ -172,8 +205,13 @@ void CaptivePortalTabHelper::OnCaptivePortalResults(Result previous_result,
   login_detector_->OnCaptivePortalResults(previous_result, result);
 }
 
-bool CaptivePortalTabHelper::IsLoginTab() const {
-  return login_detector_->is_login_tab();
+void CaptivePortalTabHelper::OnLoadAborted() {
+  // 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;
+
+  tab_reloader_->OnAbort();
 }
 
 void CaptivePortalTabHelper::SetIsLoginTab() {

chrome/browser/captive_portal/captive_portal_tab_helper.h

@@ -13,6 +13,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "webkit/glue/resource_type.h"
 
 class GURL;
 class Profile;
@@ -35,9 +36,14 @@ class CaptivePortalTabReloader;
 // to inform the tab's CaptivePortalLoginDetector when the tab is at a captive
 // portal's login page.
 //
-// TODO(mmenke): Support redirects.  Needed for HSTS, which simulates redirects
-//               at the network layer.  Also may reduce the number of
-//               unnecessary captive portal checks on high latency connections.
+// The TabHelper assumes that a WebContents can only have one RenderViewHost
+// with a provisional load at a time, and tracks only that navigation.  This
+// assumption can be violated in rare cases, for example, a same-site
+// navigation interrupted by a cross-process navigation started from the
+// omnibox, may commit before it can be cancelled.  In these cases, this class
+// may pass incorrect messages to the TabReloader, which will, at worst, result
+// in not opening up a login tab until a second load fails or not automatically
+// reloading a tab after logging in.
 //
 // For the design doc, see:
 // https://docs.google.com/document/d/1k-gP2sswzYNvryu9NcgN7q5XrsMlUdlUdoW9WRaEmfM/edit
@@ -90,11 +96,15 @@ class CaptivePortalTabHelper : public content::WebContentsObserver,
   friend class CaptivePortalTabHelperTest;
 
   // Called by Observe in response to the corresponding event.
-  void OnRedirect(int64 frame_id, const GURL& new_url);
+  void OnRedirect(int child_id,
+                  ResourceType::Type resource_type,
+                  const GURL& new_url);
 
   // Called by Observe in response to the corresponding event.
   void OnCaptivePortalResults(Result previous_result, Result result);
 
+  void OnLoadAborted();
+
   // Called to indicate a tab is at, or is navigating to, the captive portal
   // login page.
   void SetIsLoginTab();
@@ -102,6 +112,10 @@ class CaptivePortalTabHelper : public content::WebContentsObserver,
   // |this| takes ownership of |tab_reloader|.
   void SetTabReloaderForTest(CaptivePortalTabReloader* tab_reloader);
 
+  const content::RenderViewHost* provisional_render_view_host() const {
+    return provisional_render_view_host_;
+  }
+
   CaptivePortalTabReloader* GetTabReloaderForTest();
 
   // Opens a login tab if the profile's active window doesn't have one already.
@@ -118,10 +132,11 @@ class CaptivePortalTabHelper : public content::WebContentsObserver,
   // net::OK, otherwise.
   int pending_error_code_;
 
-  // The ID of the main frame that's currently provisionally loaded, if there is
-  // one.  -1 (unknown/invalid) when there is no such frame, or when an id of
-  // -1 is passed to DidStartProvisionalLoadForFrame.
-  int64 provisional_main_frame_id_;
+  // The RenderViewHost with a provisional load, if any.  Can either be
+  // the currently displayed RenderViewHost or a pending RenderViewHost for
+  // cross-process navitations.  NULL when there's currently no provisional
+  // load.
+  content::RenderViewHost* provisional_render_view_host_;
 
   content::NotificationRegistrar registrar_;