Revert "Add pre- and post-commit modes to LoginHandler"

This reverts commit a6f001ae.

Reason for revert: Speculative. These 3 tests started failing
on all main waterfall mac tester bots in browser_tests:
ServiceProcessControlBrowserTest.HistogramsNoService
ServiceProcessControlBrowserTest.LaunchAndIPC
ServiceProcessControlBrowserTest.LaunchAndReconnect

All builds where this started happening had this CL in common.
The build that best shows this is:
https://ci.chromium.org/p/chromium/builders/ci/Mac10.11%20Tests/37949
The other 3 CLs in that build look like they're likely no-ops on macOS.

This CL also doesn't look super related to the failure message, but
relanding is easy if this turns out to not help.
Original change's description:
> Add pre- and post-commit modes to LoginHandler
> 
> LoginHandler currently handles two tasks when showing an interstitial:
> running the request through extensions and then showing a login
> prompt. When converted to committed interstitials, we need to run the
> request through extensions before the navigation commits, and then
> show a login prompt on top of a committed error page. This CL adds a
> "mode" argument to LoginHandler. When committed interstitials are
> enabled, the "mode" argument determines whether we should just run the
> request through extensions and then cancel it (pre-commit mode), or
> show a login prompt (post-commit). The post-commit mode is currently
> not reachable; we'll use it once we add the WebContentsObserver that
> observes the committed error page (https://crbug.com/963313).
> 
> Note: this "mode" design is kind of messy and we'll probably want to
> come back and clean it up later. For now, it's the simplest way to
> leave LoginHandler intact for when committed interstitials are
> disabled, and use the parts of it that we need before+after commit
> when committed interstitials are enabled. Once committed interstitials
> land and we clean up the old code, we will probably want to split
> LoginHandler into two classes rather than having one class that can
> operate in two different modes.
> 
> Bug: 963311
> Change-Id: I1efddb3eca3a6b822135bd721db05f2f47561a86
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1616943
> Reviewed-by: Carlos IL <carlosil@chromium.org>
> Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
> Commit-Queue: Emily Stark <estark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#661141}

TBR=estark@chromium.org,carlosil@chromium.org,jdeblasio@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 963311
Change-Id: Id22a831ac81cd1056da31efbf5f045edb5533726
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1619069Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661206}

chrome/browser/chrome_content_browser_client.cc

@@ -5223,8 +5223,7 @@ ChromeContentBrowserClient::CreateLoginDelegate(
     LoginAuthRequiredCallback auth_required_callback) {
   return CreateLoginPrompt(
       auth_info, web_contents, request_id, is_request_for_main_frame, url,
-      std::move(response_headers), LoginHandler::PRE_COMMIT,
-      std::move(auth_required_callback));
+      std::move(response_headers), std::move(auth_required_callback));
 }
 
 bool ChromeContentBrowserClient::HandleExternalProtocol(

chrome/browser/ui/login/login_handler.cc

@@ -10,7 +10,6 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
-#include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -22,7 +21,6 @@
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/login/login_interstitial_delegate.h"
-#include "chrome/common/chrome_features.h"
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
 #include "components/password_manager/core/browser/log_manager.h"
 #include "components/password_manager/core/browser/password_manager.h"
@@ -124,8 +122,7 @@ void LoginHandler::Start(
     const content::GlobalRequestID& request_id,
     bool is_main_frame,
     const GURL& request_url,
-    scoped_refptr<net::HttpResponseHeaders> response_headers,
-    HandlerMode mode) {
+    scoped_refptr<net::HttpResponseHeaders> response_headers) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(web_contents());
   DCHECK(!WasAuthHandled());
@@ -137,9 +134,9 @@ void LoginHandler::Start(
   auto* api =
       extensions::BrowserContextKeyedAPIFactory<extensions::WebRequestAPI>::Get(
           web_contents()->GetBrowserContext());
-  auto continuation = base::BindOnce(&LoginHandler::MaybeSetUpLoginPrompt,
-                                     weak_factory_.GetWeakPtr(), request_url,
-                                     is_main_frame, mode);
+  auto continuation =
+      base::BindOnce(&LoginHandler::MaybeSetUpLoginPrompt,
+                     weak_factory_.GetWeakPtr(), request_url, is_main_frame);
   if (api->MaybeProxyAuthRequest(web_contents()->GetBrowserContext(),
                                  auth_info_, std::move(response_headers),
                                  request_id, is_main_frame,
@@ -155,7 +152,7 @@ void LoginHandler::Start(
       FROM_HERE, {BrowserThread::UI},
       base::BindOnce(&LoginHandler::MaybeSetUpLoginPrompt,
                      weak_factory_.GetWeakPtr(), request_url, is_main_frame,
-                     mode, base::nullopt, false /* should_cancel */));
+                     base::nullopt, false /* should_cancel */));
 }
 
 void LoginHandler::SetAuth(const base::string16& username,
@@ -416,7 +413,6 @@ void LoginHandler::GetDialogStrings(const GURL& request_url,
 void LoginHandler::MaybeSetUpLoginPrompt(
     const GURL& request_url,
     bool is_request_for_main_frame,
-    HandlerMode mode,
     const base::Optional<net::AuthCredentials>& credentials,
     bool should_cancel) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -480,25 +476,6 @@ void LoginHandler::MaybeSetUpLoginPrompt(
           blink::kWebDisplayModeStandalone) {
     RecordHttpAuthPromptType(AUTH_PROMPT_TYPE_WITH_INTERSTITIAL);
 
-    if (base::FeatureList::IsEnabled(
-            features::kHTTPAuthCommittedInterstitials)) {
-      switch (mode) {
-        case PRE_COMMIT:
-          prompt_started_ = false;
-          CancelAuth();
-          return;
-        case POST_COMMIT:
-          // TODO(https://crbug.com/963313): add a WebContentsObserver which
-          // observes when LoginHandler does the PRE_COMMIT cancel and triggers
-          // the login prompt in POST_COMMIT mode on top of the committed error
-          // page.
-          NOTREACHED();
-          ShowLoginPrompt(request_url);
-          return;
-      }
-      NOTREACHED();
-    }
-
     // Show a blank interstitial for main-frame, cross origin requests
     // so that the correct URL is shown in the omnibox.
     base::OnceClosure callback =
@@ -598,11 +575,10 @@ std::unique_ptr<content::LoginDelegate> CreateLoginPrompt(
     bool is_request_for_main_frame,
     const GURL& url,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
-    LoginHandler::HandlerMode mode,
     LoginAuthRequiredCallback auth_required_callback) {
   std::unique_ptr<LoginHandler> handler = LoginHandler::Create(
       auth_info, web_contents, std::move(auth_required_callback));
   handler->Start(request_id, is_request_for_main_frame, url,
-                 std::move(response_headers), mode);
+                 std::move(response_headers));
   return handler;
 }

chrome/browser/ui/login/login_handler.h

@@ -38,17 +38,6 @@ class LoginHandler : public content::LoginDelegate,
                      public content::NotificationObserver,
                      public content::WebContentsObserver {
  public:
-  // When committed interstitials are enabled, LoginHandler has pre-commit and
-  // post-commit modes that determines how main-frame cross-origin navigations
-  // are handled. Pre-commit, login challenges on such navigations are
-  // optionally handled by extensions and then cancelled so that an error page
-  // can commit. Post-commit, LoginHandler shows a login prompt on top of the
-  // committed error page.
-  enum HandlerMode {
-    PRE_COMMIT,
-    POST_COMMIT,
-  };
-
   // The purpose of this struct is to enforce that BuildViewImpl receives either
   // both the login model and the observed form, or none. That is a bit spoiled
   // by the fact that the model is a pointer to LoginModel, as opposed to a
@@ -79,14 +68,10 @@ class LoginHandler : public content::LoginDelegate,
       content::WebContents* web_contents,
       LoginAuthRequiredCallback auth_required_callback);
 
-  // |mode| is ignored when committed interstitials are disabled. See the
-  // comment on HandlerMode for a description of how it affects LoginHandler's
-  // behavior when committed interstitials are enabled.
   void Start(const content::GlobalRequestID& request_id,
              bool is_main_frame,
              const GURL& request_url,
-             scoped_refptr<net::HttpResponseHeaders> response_headers,
-             HandlerMode mode);
+             scoped_refptr<net::HttpResponseHeaders> response_headers);
 
   // Resend the request with authentication credentials.
   // This function can be called from either thread.
@@ -166,14 +151,11 @@ class LoginHandler : public content::LoginDelegate,
   // Continuation from |Start| after any potential interception from the
   // extensions WebRequest API. If |should_cancel| is |true| the request is
   // cancelled. Otherwise |credentials| are used if supplied. Finally if the
-  // request is NOT cancelled AND |credentials| is empty, then we'll take the
-  // necessary steps to show a login prompt, depending on |mode| and whether
-  // committed interstitials are enabled (see comment on
-  // LoginHandler::HandlerMode).
+  // request is NOT cancelled AND |credentials| is empty, then we'll actually
+  // show a login prompt.
   void MaybeSetUpLoginPrompt(
       const GURL& request_url,
       bool is_main_frame,
-      HandlerMode mode,
       const base::Optional<net::AuthCredentials>& credentials,
       bool should_cancel);
 
@@ -253,7 +235,6 @@ std::unique_ptr<content::LoginDelegate> CreateLoginPrompt(
     bool is_main_frame,
     const GURL& url,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
-    LoginHandler::HandlerMode mode,
     LoginAuthRequiredCallback auth_required_callback);
 
 #endif  // CHROME_BROWSER_UI_LOGIN_LOGIN_HANDLER_H_

chrome/browser/ui/login/login_handler_browsertest.cc

@@ -52,13 +52,13 @@ using content::Referrer;
 
 namespace {
 
-bool AreSSLCommittedInterstitialsEnabled() {
+bool AreCommittedInterstitialsEnabled() {
   return base::FeatureList::IsEnabled(features::kSSLCommittedInterstitials);
 }
 
 content::InterstitialPageDelegate* GetInterstitialDelegate(
     content::WebContents* tab) {
-  if (AreSSLCommittedInterstitialsEnabled()) {
+  if (AreCommittedInterstitialsEnabled()) {
     security_interstitials::SecurityInterstitialTabHelper* helper =
         security_interstitials::SecurityInterstitialTabHelper::FromWebContents(
             tab);
@@ -1507,7 +1507,7 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest,
     ui_test_utils::NavigateToURL(browser(), broken_ssl_page);
     ASSERT_EQ("127.0.0.1", contents->GetURL().host());
     ASSERT_TRUE(contents->GetURL().SchemeIs("https"));
-    if (AreSSLCommittedInterstitialsEnabled()) {
+    if (AreCommittedInterstitialsEnabled()) {
       ASSERT_TRUE(WaitForRenderFrameReady(contents->GetMainFrame()));
     } else {
       content::WaitForInterstitialAttach(contents);
@@ -1580,7 +1580,7 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest,
 
   // Redirect to a broken SSL page. This redirect should not accidentally
   // proceed through the SSL interstitial.
-  if (AreSSLCommittedInterstitialsEnabled()) {
+  if (AreCommittedInterstitialsEnabled()) {
     content::TestNavigationObserver observer(contents);
     EXPECT_TRUE(content::ExecuteScript(
         browser()->tab_strip_model()->GetActiveWebContents(),
@@ -1646,30 +1646,4 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestBasicAuthDisabled) {
   }
 }
 
-// Tests that when HTTP Auth committed interstitials are enabled, a cross-origin
-// main-frame auth challenge cancels the auth request.
-IN_PROC_BROWSER_TEST_F(
-    LoginPromptBrowserTest,
-    TestAuthChallengeCancelsNavigationWithCommittedInterstitials) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      features::kHTTPAuthCommittedInterstitials);
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  NavigationController* controller = &contents->GetController();
-  LoginPromptBrowserTestObserver observer;
-  observer.Register(content::Source<NavigationController>(controller));
-
-  GURL test_page = embedded_test_server()->GetURL(kAuthBasicPage);
-  ui_test_utils::NavigateToURL(browser(), test_page);
-
-  const base::string16 kExpectedTitle =
-      base::ASCIIToUTF16("Denied: Missing Authorization Header");
-  content::TitleWatcher title_watcher(contents, kExpectedTitle);
-  EXPECT_EQ(kExpectedTitle, title_watcher.WaitAndGetTitle());
-
-  EXPECT_EQ(0, observer.auth_cancelled_count());
-}
 }  // namespace