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

chrome/browser/captive_portal/captive_portal_tab_helper.h

+// 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.
+
+#ifndef CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_TAB_HELPER_H_
+#define CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_TAB_HELPER_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time.h"
+#include "base/timer.h"
+#include "chrome/browser/captive_portal/captive_portal_tab_observer.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
+
+class Profile;
+class TabContentsWrapper;
+
+namespace captive_portal {
+
+// Keeps track of whether a tab is a login page or has encountered a navigation
+// error caused by a captive portal.  It triggers captive portal checks and
+// opens tabs at the portal's login page as necessary.  All methods may only be
+// called on the UI thread.
+//
+// Only supports SSL main frames which time out in response to captive portals,
+// since these make for a particularly bad user experience.  Non-SSL requests
+// are intercepted by captive portals, which take users to the login page.  SSL
+// requests, however, are generally silently blackholed.  They then take two
+// minutes to timeout, and will timeout again when refreshed.
+//
+// For the design doc, see:
+// https://docs.google.com/document/d/1k-gP2sswzYNvryu9NcgN7q5XrsMlUdlUdoW9WRaEmfM/edit
+class CaptivePortalTabHelper : public content::NotificationObserver,

[cbentzel, 2012/05/22 17:54:27]

Would it make sense to have different TabHelper's for the login page versus
every other page?

[mmenke, 2012/05/22 18:22:36]

I've gone back and forth on this more than once.

We need to be able to tell if a tab is a captive portal login page to avoid
opening multiple login tabs, so there needs to be some publicly accessible
entity that tracks this.  Two obvious ways to do this are to make them part of
the TabContentsWrapper interface, or have the CaptivePortalService track them
(In the latter case, we could only create them as needed).  I didn't go with the
second option because of the Browser-not-supported-on-Android issue means the
CaptivePortalService can't open the login tab, so it seems weird to have it
track them, though we could just make that one function non-android, if so
desired.

Similarly, I prefer that BrowserTests be able to get the CaptivePortalTabHelper
state of a tab, though it's not strictly necessary, and to do that, they need to
be part of the TabContentsWrapper interface, unless something else tracks them.

I decided to go with just one class to avoid interface bloat, but having two
always-existent observers is certainly doable.  Since they share pretty much no
behavior with each other, it would simplify some things a little.

Another option is to replace the STATE_CAPTIVE_PORTAL_LOGIN_PAGE with a boolean
flag, tracked independently of everything else.  Cleared when a captive portal
goes away, and just use it to check state again when a load stops.  This would
remove a couple of the "if STATE_CAPTIVE_PORTAL_LOGIN_PAGE, do nothing" checks,
and isolate the two types of state, while also introducing only a single new
TabHelper.

I think all 3 approaches are reasonable, though have a weak preference for the
last option (Get rid of state, use a bool).  Didn't go with it because it
occurred to me late, and wanted to see if you had any opinion on the three
options.  If you have a preference for two classes, I'll go ahead and go that
route.

[cbentzel, 2012/05/24 12:41:37]

I would not have two classes which are directly referenced by TabContentsWrapper
since that adds complexity for users of this class. You could have
CaptivePortalTabHelper hold scoped_ptr's to objects of the two different
classes, and just forward requests along to them. I do think that the
implementation may be simpler in that case, since the state machines are pretty
much completely separate for the login tab page versus every other tab.

[mmenke, 2012/05/24 14:19:04]

SGTM, will do.

+                               public base::NonThreadSafe {
+ public:
+  enum State {
+    STATE_NONE,
+    // The slow load timer is running.  Only started on SSL provisional loads.
+    // If the timer triggers before the page has been committed, a captive
+    // portal test will be requested.
+    STATE_TIMER_RUNNING,
+    // The tab may have been broken by a captive portal.  A tab switches to
+    // this state either on an ERR_CONNECTION_TIMEOUT of an SSL page or when
+    // an SSL request takes too long to commit.  The tab will remain in this
+    // state until the current load succeeds, a new provisional load starts,
+    // or it gets a captive portal result.
+    STATE_MAYBE_BROKEN_BY_PORTAL,
+    // The TabHelper switches to this state from STATE_MAYBE_BROKEN_BY_PORTAL in
+    // response to a RESULT_BEHIND_CAPTIVE_PORTAL.  The tab will remain in this
+    // state until a new provisional load starts, the original load successfully
+    // commits, the current load is aborted, or the tab reloads the page in
+    // response to receiving a captive portal result other than
+    // RESULT_BEHIND_CAPTIVE_PORTAL.
+    STATE_BROKEN_BY_PORTAL,
+    // The page may need to be reloaded.  The tab will be reloaded if the page
+    // fails the next load with a timeout, or immediately upon switching to this
+    // state, if the page already timed out.  If anything else happens
+    // when in this state (Another error, successful navigation, or the original
+    // navigation was aborted), the TabHelper transitions to STATE_NONE without
+    // reloading.
+    STATE_NEEDS_RELOAD,
+    // This tab is, or was, at the captive portal login page.  This state is
+    // only cleared once the CaptivePortalService returns something other than
+    // RESULT_BEHIND_CAPTIVE_PORTAL.
+    STATE_CAPTIVE_PORTAL_LOGIN_PAGE,
+  };
+
+  // |profile| and |web_contents| will only be dereferenced in ReloadTab,
+  // MaybeOpenCaptivePortalLoginTab, and CheckForCaptivePortal, so they can
+  // both be NULL in the unit tests, though |profile| will still be used as a
+  // notification source.
+  CaptivePortalTabHelper(Profile* profile, content::WebContents* web_contents);
+
+  virtual ~CaptivePortalTabHelper();
+
+ protected:
+  // The following 4 functions are all invoked by |observer_|.
+
+  // Called when a non-error main frame load starts.  Resets current state,
+  // unless this is a login tab.  Each load will eventually result in a call to
+  // OnLoadCommitted or OnAbort.  The former will be called both on successful
+  // loads and for error pages.
+  virtual void OnLoadStart(bool is_ssl);
+
+  // Called when a page is committed.  |net_error| will be net::OK in the case
+  // of a successful load.  For an errror page, the entire 3-step process of
+  // getting the error, starting a new provisional load for the error page, and
+  // committing the error page is treated as a single commit.
+  //
+  // The Link Doctor page will typically be one OnLoadCommitted with an error
+  // code, followed by another OnLoadCommitted with net::OK for the Link Doctor
+  // page.
+  virtual void OnLoadCommitted(int net_error);

[cbentzel, 2012/05/22 17:54:27]

Could this use net::Error instead of int? 

[mmenke, 2012/05/22 18:22:36]

The WebContentsObserver only declares it as an int, and the network stack almost
always passes net::Errors around as ints as well, so I think using an int is
more in keeping with existing style.  I admit to also not being a big fan of
casts unless necessary.  In this case, it makes it through a couple levels of
WebKit in the renderer process before we finally get it.

+
+  // This is called when the current provisional load is canceled.
+  // Sets state to STATE_NONE, unless this is a login tab.
+  virtual void OnAbort();
+
+  // Called when the WebContents stops loading.  Starts a captive portal check
+  // if this is the login tab.
+  virtual void OnStopLoading();
+
+  // Sets |state_| and takes any action associated with the new state.
+  void SetState(State new_state);
+
+  State state() const { return state_; }
+
+  // Used by unit tests.
+  void set_slow_ssl_load_time(base::TimeDelta slow_ssl_load_time) {
+    slow_ssl_load_time_ = slow_ssl_load_time;
+  }
+
+  // Started whenever an SSL tab starts loading, when the state is switched to
+  // STATE_TIMER_RUNNING.  Stopped on any state change, including when a page
+  // commits or there's an error.  If the timer triggers, the state switches to
+  // STATE_MAYBE_BROKEN_BY_PORTAL and |this| kicks off a captive portal check.
+  // TODO(mmenke):  On redirects, update this timer.
+  base::OneShotTimer<CaptivePortalTabHelper> slow_ssl_load_timer_;
+
+  // Handles messages from the WebContents.  Separate class to simplify logic
+  // and testing.
+  CaptivePortalTabObserver observer_;
+
+ private:
+  friend class CaptivePortalBrowserTest;
+  // friend class CaptivePortalTabHelperTest;
+  friend class CaptivePortalTabObserver;
+
+  // content::NotificationObserver:
+  virtual void Observe(
+      int type,
+      const content::NotificationSource& source,
+      const content::NotificationDetails& details) OVERRIDE;
+
+  // Called by a timer when an SSL main frame provisional load is taking a
+  // while to commit.
+  void OnSlowSSLConnect();
+
+  // Reloads the tab if there's no provisional load going on and the current
+  // state is STATE_NEEDS_RELOAD.  Not safe to call synchronously when called
+  // by |observer_|, since the WebContents is already taking some action.
+  void ReloadTabIfNeeded();
+
+  // Reloads the tab.
+  virtual void ReloadTab();
+
+  // Opens a login tab in the topmost browser window for the |profile_|, if the
+  // profile has a tabbed browser window and the window doesn't already have a
+  // login tab.  Otherwise, does nothing.
+  virtual void MaybeOpenCaptivePortalLoginTab();
+
+  // Tries to get |profile_|'s CaptivePortalService and have it start a captive
+  // portal check.
+  virtual void CheckForCaptivePortal();
+
+  Profile* profile_;
+
+  State state_;
+
+  // Tracks if there's a load going on that can't safely be interrupted.  This
+  // is true between the time when a provisional load fails and when an error
+  // page's provisional load starts, so does not perfectly align with the
+  // notion of a provisional load used by the WebContents.
+  bool provisional_main_frame_load_;
+
+  // Time to wait after a provisional HTTPS load before triggering a captive
+  // portal check.
+  base::TimeDelta slow_ssl_load_time_;
+
+  base::WeakPtrFactory<CaptivePortalTabHelper> weak_factory_;
+
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(CaptivePortalTabHelper);
+};
+
+}  // namespace captive_portal
+
+#endif  // CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_TAB_HELPER_H_