Open a login tab on captive portal detection on SSL loads.

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 0)
+++ chrome/browser/captive_portal/captive_portal_tab_helper.cc	(revision 0)
@@ -0,0 +1,274 @@
+// 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.
+
+#include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
+
+#include "base/bind.h"
+#include "base/message_loop.h"
+#include "chrome/browser/captive_portal/captive_portal_service.h"
+#include "chrome/browser/captive_portal/captive_portal_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/web_contents.h"
+#include "net/base/net_errors.h"
+
+namespace captive_portal {
+
+namespace {
+
+// The time we wait for a slow loading SSL page before trigginer a captive

[cbentzel, 2012/04/18 15:12:48]

Nit: trigginer

[mmenke, 2012/04/18 19:15:46]

Fixed.

+// portal check.
+static int kDefaultSlowSSLTime = 30;

[cbentzel, 2012/04/18 15:57:23]

Nit: kDefaultSlowSSLTimeSeconds since this does not have explicit units.

[mmenke, 2012/04/18 19:15:46]

Done.

+
+}  // namespace
+
+CaptivePortalTabHelper::~CaptivePortalTabHelper() {
+}
+
+// static
+CaptivePortalTabHelper* CaptivePortalTabHelper::Create(
+    TabContentsWrapper* tab_contents) {
+  return new CaptivePortalTabHelper(
+                 tab_contents->profile(),

[cbentzel, 2012/04/18 15:57:23]

Nit: is this correct indent? Should just be 4 from the "return". Alternately,
looks like you might be able to fit on one line.

[mmenke, 2012/04/18 19:15:46]

Removed the function.

+                 tab_contents->web_contents());
+}
+
+CaptivePortalTabHelper::CaptivePortalTabHelper(
+    Profile* profile,
+    content::WebContents* web_contents)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(observer_(this, web_contents)),
+      profile_(profile),
+      state_(STATE_NONE),
+      provisional_main_frame_load_(false),
+      slow_ssl_load_time_(base::TimeDelta::FromSeconds(kDefaultSlowSSLTime)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
+                 content::Source<Profile>(profile_));
+}
+
+void CaptivePortalTabHelper::OnLoadStart(bool is_ssl) {
+  DCHECK(CalledOnValidThread());
+
+  provisional_main_frame_load_ = true;
+
+  // Don't change state when error pages start loading.  If we're going to
+  // reload, we want to reload only after the error page has been committed.
+  // If this is a login tab, also do nothing.
+  if (state_ == STATE_CAPTIVE_PORTAL_LOGIN_PAGE)
+    return;
+
+  // Otherwise, reset the state, as we're at a new page.
+  SetState(STATE_NONE);
+
+  // Start the slow load timer for SSL pages.
+  // TODO(mmenke):  Should we be looking at the port instead?  The reason the
+  //                request never connects is because of the port, not the
+  //                protocol.
+  if (is_ssl)
+    SetState(STATE_TIMER_RUNNING);
+}
+
+void CaptivePortalTabHelper::OnLoadCommitted(int net_error) {
+  DCHECK(CalledOnValidThread());
+
+  provisional_main_frame_load_ = false;
+
+  if (state_ == STATE_CAPTIVE_PORTAL_LOGIN_PAGE || state_ == STATE_NONE)
+    return;
+
+  // If we don't have a timeout error, reset the state.
+  if (net_error != net::ERR_CONNECTION_TIMED_OUT) {
+    // TODO(mmenke):  If the new URL is the same as the old broken URL, and the
+    //                request succeeds, we should probably trigger another
+    //                captive portal check.
+    SetState(STATE_NONE);
+    return;
+  }
+
+  // We timed out before the timer expired.  This is not terribly likely, but
+  // if it does happen, we may have been broken by a captive portal.  Go ahead
+  // and start the check now, rather than waiting for the timer.
+  if (state_ == STATE_TIMER_RUNNING) {
+    OnSlowSSLConnect();
+    return;
+  }
+
+  // If we need to reload, do so asynchronously, since we're in a callback from
+  // the WebContents.
+  if (state_ == STATE_NEEDS_RELOAD) {
+    // If we need to reload the page, do so asynchronously.
+    MessageLoop::current()->PostTask(

[cbentzel, 2012/04/18 15:12:48]

I'm a little concerned here about doing unnecessary reloads - for example, we
started the https load, logged in correctly elsewhere, then it lets bytes
through, and then we decide to reload even though it wasn't needed. 

[mmenke, 2012/04/18 19:15:46]

We only get here when we commit with a connection timed out error page (See line
87 in this CL/80 in the most recent CL), so I don't think there's an issue.

+        FROM_HERE,
+        base::Bind(&CaptivePortalTabHelper::ReloadTabIfNeeded,
+                   weak_factory_.GetWeakPtr()));
+  }
+}
+
+void CaptivePortalTabHelper::OnAbort() {
+  DCHECK(CalledOnValidThread());
+
+  provisional_main_frame_load_ = false;
+  if (state_ == STATE_CAPTIVE_PORTAL_LOGIN_PAGE)
+    return;
+  SetState(STATE_NONE);
+}
+
+void CaptivePortalTabHelper::OnStopLoading() {
+  DCHECK(CalledOnValidThread());
+
+  // Check for captive portal when loading stops.  This can't be done on
+  // DidFinishLoad in the case of captive portals that return the user to
+  // the originally requested page, as getting a 204 response does not result
+  // in a DidFinishLoad call.
+  if (state_ == STATE_CAPTIVE_PORTAL_LOGIN_PAGE)
+    CheckForCaptivePortal();
+}
+
+void CaptivePortalTabHelper::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT, type);
+  DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
+
+  CaptivePortalService::Results* results =
+      content::Details<CaptivePortalService::Results>(details).ptr();
+
+  DCHECK(CalledOnValidThread());
+
+  if (results->result == RESULT_BEHIND_CAPTIVE_PORTAL) {
+    if (state_ == STATE_MAYBE_BROKEN_BY_PORTAL) {
+      SetState(STATE_BROKEN_BY_PORTAL);
+      MaybeOpenCaptivePortalLoginTab();
+    }
+    return;
+  }
+
+  switch (state_) {
+    case STATE_MAYBE_BROKEN_BY_PORTAL:
+    case STATE_TIMER_RUNNING:
+      // If the previous result was BEHIND_CAPTIVE_PORTAL, and we're either in
+      // STATE_MAYBE_BROKEN_BY_PORTAL or STATE_TIMER_RUNNING, reload the tab.
+      // In the latter case, the tab has yet to commit, but is an SSL page, so
+      // if the page ends up at a timeout error, we'll reload it.  If not,
+      // just reset the state.  The helps in the case that a user tries to
+      // reload a tab, and then quickly logs in.
+      if (results->previous_result == RESULT_BEHIND_CAPTIVE_PORTAL) {
+        SetState(STATE_NEEDS_RELOAD);
+        return;
+      }
+      SetState(STATE_NONE);
+      return;
+
+    case STATE_BROKEN_BY_PORTAL:
+      // If we don't have a provisional load going, and we were broken by a
+      // captive portal, reload the page.  If we don't reload the tab, we keep
+      // the current state and reload when the current load finally times out,
+      // assuming it does so.
+      SetState(STATE_NEEDS_RELOAD);
+      return;
+
+    case STATE_CAPTIVE_PORTAL_LOGIN_PAGE:
+      // There is no captive portal any more, so we're no longer a login tab.
+      SetState(STATE_NONE);

[cbentzel, 2012/04/18 15:12:48]

Should this politely close itself, or are there concerns about the user browsing
further in the same tab after the login page?

[mmenke, 2012/04/18 19:15:46]

The user could have logged in from another tab, so I'm a bit paranoid about
automatically closing tabs.  Definitely worth considering in the future, but I'd
like to keep it as-is, and make sure things are working, before adding behaviors
that increase the likelihood of causing problems.

There's also the multi-profile case, which I'm a bit concerned about, since if
we somehow end up openning login tabs for multiple profiles, we only end up
closing the login tabs for one of them here, and the user could be trying to
navigate to a bookmark or some other site or something with the login tab.  How
likely this is, I have no idea.

Clearing the LOGIN_PAGE on browser-process initiated navigations may circumvent
these issues, though if the user navigated to an SSL page again that then times
out, we'd open and focus another tab, which could be a little annoying.

+      return;
+
+    case STATE_NEEDS_RELOAD:
+    case STATE_NONE:
+      // If the tab needs to reload or is in STATE_NONE, do nothing.  The reload
+      // case shouldn't be very common, since it only lasts until a tab times
+      // out, but it's still possible.
+      return;
+
+    default:
+      NOTREACHED();
+  }
+}
+
+void CaptivePortalTabHelper::OnSlowSSLConnect() {
+  SetState(STATE_MAYBE_BROKEN_BY_PORTAL);
+}
+
+void CaptivePortalTabHelper::SetState(State new_state) {
+  // Stop the timer even when old and new states are the same.
+  slow_ssl_load_timer_.Stop();

[cbentzel, 2012/04/18 15:12:48]

Do we only need to do this if state_ == STATE_TIMER_RUNNING at this point?

[mmenke, 2012/04/18 19:15:46]

Done.

+

[cbentzel, 2012/04/18 15:12:48]

Would it make sense to add DCHECKs here about valid state transitions?

[mmenke, 2012/04/18 19:15:46]

Done.  Valid transitions are:

From anything to NONE (This is also the only self-to-self transition that can
occur)
From NONE to TIMER_RUNNING or LOGIN_PAGE.

For the states:
TIMER_RUNNING,
MAYBE_BROKEN,
BROKEN,
NEEDS_RELOAD,

Each state can transition to any of the later states, except TIMER_RUNNING can't
transition to BROKEN.  Adding a transition there could actually save us an extra
captive portal check in some cases, but I kept it out since we may not even have
a slow tab).

+  state_ = new_state;
+
+  switch (state_) {
+    case STATE_TIMER_RUNNING:
+      slow_ssl_load_timer_.Start(
+          FROM_HERE,
+          slow_ssl_load_time_,
+          this,
+          &CaptivePortalTabHelper::OnSlowSSLConnect);
+      break;
+
+    case STATE_MAYBE_BROKEN_BY_PORTAL:
+      CheckForCaptivePortal();
+      break;
+
+    case STATE_NEEDS_RELOAD:
+      // Try to reload the tab now.
+      ReloadTabIfNeeded();
+      break;
+
+    default:
+      break;
+  }
+}
+
+void CaptivePortalTabHelper::ReloadTabIfNeeded() {
+  // If there's still a provisional load going, or we're no longer in
+  // STATE_NEEDS_RELOAD, do nothing.
+  if (state_ != STATE_NEEDS_RELOAD || provisional_main_frame_load_)
+    return;
+  SetState(STATE_NONE);
+  ReloadTab();
+}
+
+void CaptivePortalTabHelper::ReloadTab() {
+  observer_.web_contents()->GetController().Reload(true);

[cbentzel, 2012/04/18 15:12:48]

Do you want to have the user manually reload if there is post data? What you
have will bring up the repost warning, but I don't know if that would be
considered annoying. Also, is the repost warning tab-constrained or
browser-constrained?

[mmenke, 2012/04/18 19:15:46]

Since we failed to commit, we'll always reload the page with a GET.  Hmm...At
least if it's a new navigation.  Need to double-check what happens if the
original page was in the history.  I believe the same thing will be true.

That having been said, I've added a check so we don't reload posts.

+}
+
+void CaptivePortalTabHelper::MaybeOpenCaptivePortalLoginTab() {

[cbentzel, 2012/04/18 15:12:48]

I'm wondering if this should move to the service in case there are multiple
signals to trigger detection of captive portals. Or do you anticipate that this
will be the only one to open tabs? 

[mmenke, 2012/04/18 19:15:46]

I've gone back and forth on this.  Currently, the captive portal service does
not depend on the TabHelpers, or even the browser process.  As a result, the
CaptivePortalService should work on Android, if we chose to compile it there,
but this function would not.  It's here primarily because of that.

That having been said, we aren't even compiling the service on Android, so I
think it's reasonable to either put this in the service, or give the service a
function to call this directly (Since we need to call SetState).

[cbentzel, 2012/04/18 21:12:37]

That's a good argument for why this is here.

+  Browser* browser = Browser::GetTabbedBrowser(profile_, true);
+  // If the Profile doesn't have a tabbed browser window open, do nothing.
+  if (!browser)
+    return;
+
+  // Check if the Profile's topmost browser window already has a login tab.
+  // If so, do nothing.
+  // TODO(mmenke):  Consider focusing that tab, at least if we're already
+  //                the active tab and in the same Browser window.
+  for (int i = 0; i < browser->tab_count(); ++i) {
+    TabContentsWrapper* tab_contents_wrapper =
+        browser->GetTabContentsWrapperAt(i);
+    if (tab_contents_wrapper->captive_portal_tab_helper()->state() ==
+            CaptivePortalTabHelper::STATE_CAPTIVE_PORTAL_LOGIN_PAGE) {
+      return;
+    }
+  }
+
+  // Otherwise, open a login tab.
+  TabContentsWrapper* tab_contents_wrapper =
+      browser->AddSelectedTabWithURL(
+          CaptivePortalServiceFactory::GetForProfile(profile_)->test_url(),
+          content::PAGE_TRANSITION_TYPED);

[cbentzel, 2012/04/18 15:57:23]

Should we add a new page transition type here? Do we want this to show up in
browsing history?

[mmenke, 2012/04/18 19:15:46]

It would be great if the page didn't show up in the history.  I want to get a
better understanding of just how these things work first, however.

I suggest saving that for another CL, since it will involve touching some other
parts of the code.

[cbentzel, 2012/04/18 21:12:37]

OK.

+  tab_contents_wrapper->captive_portal_tab_helper()->SetState(
+      STATE_CAPTIVE_PORTAL_LOGIN_PAGE);
+}
+
+void CaptivePortalTabHelper::CheckForCaptivePortal() {
+  CaptivePortalServiceFactory::GetForProfile(profile_)->CheckForCaptivePortal();

[cbentzel, 2012/04/18 14:36:31]

Is this guaranteed to always exist? Probably safer to check for non-NULL in case
we decide not to do this for incognito profiles, for example.

[mmenke, 2012/04/18 14:40:41]

Yes, it's guaranteed to exist.  I'll go ahead and add a NULL check, though I do
currently have an incognito profile browser test.

+}
+
+}  // namespace captive_portal