Executed HTTPS upgrade before notifying the start of the provisional load.

content/browser/frame_host/navigation_handle_impl_browsertest.cc

 // Copyright 2016 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 "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "ui/base/page_transition_types.h"
 #include "url/url_constants.h"
 
 namespace content {
 
 namespace {
 
+// Gathers data from the NavigationHandle assigned to navigations that start
+// with the expected URL.
 class NavigationHandleObserver : public WebContentsObserver {
  public:
-  NavigationHandleObserver(WebContents* web_contents, const GURL& expected_url)
+  NavigationHandleObserver(WebContents* web_contents,
+                           const GURL& expected_start_url)
       : WebContentsObserver(web_contents),
         handle_(nullptr),
         has_committed_(false),
         is_error_(false),
         is_main_frame_(false),
         is_parent_main_frame_(false),
         is_renderer_initiated_(true),
         is_synchronous_(false),
         is_srcdoc_(false),
         was_redirected_(false),
         frame_tree_node_id_(-1),
         page_transition_(ui::PAGE_TRANSITION_LINK),
-        expected_url_(expected_url) {}
+        expected_start_url_(expected_start_url) {}
 
   void DidStartNavigation(NavigationHandle* navigation_handle) override {
-    if (handle_ || navigation_handle->GetURL() != expected_url_)
+    if (handle_ || navigation_handle->GetURL() != expected_start_url_)
       return;
 
     handle_ = navigation_handle;
     has_committed_ = false;
     is_error_ = false;
     page_transition_ = ui::PAGE_TRANSITION_LINK;
     last_committed_url_ = GURL();
 
     is_main_frame_ = navigation_handle->IsInMainFrame();
     is_parent_main_frame_ = navigation_handle->IsParentMainFrame();
@@ skipping 55 matching lines @@
   bool has_committed_;
   bool is_error_;
   bool is_main_frame_;
   bool is_parent_main_frame_;
   bool is_renderer_initiated_;
   bool is_synchronous_;
   bool is_srcdoc_;
   bool was_redirected_;
   int frame_tree_node_id_;
   ui::PageTransition page_transition_;
-  GURL expected_url_;
+  GURL expected_start_url_;
   GURL last_committed_url_;
 };
 
 // A test NavigationThrottle that will return pre-determined checks and run
-// callbacks when the various NavigationThrottle methods are called.
+// callbacks when the various NavigationThrottle methods are called. It is
+// not instantiated directly but through a TestNavigationThrottleInstaller.
 class TestNavigationThrottle : public NavigationThrottle {
  public:
   TestNavigationThrottle(
       NavigationHandle* handle,
       NavigationThrottle::ThrottleCheckResult will_start_result,
       NavigationThrottle::ThrottleCheckResult will_redirect_result,
       NavigationThrottle::ThrottleCheckResult will_process_result,
       base::Closure did_call_will_start,
       base::Closure did_call_will_redirect,
       base::Closure did_call_will_process)
@@ skipping 28 matching lines @@
   }
 
   NavigationThrottle::ThrottleCheckResult will_start_result_;
   NavigationThrottle::ThrottleCheckResult will_redirect_result_;
   NavigationThrottle::ThrottleCheckResult will_process_result_;
   base::Closure did_call_will_start_;
   base::Closure did_call_will_redirect_;
   base::Closure did_call_will_process_;
 };
 
-// Install a TestNavigationThrottle on all requests and allows waiting for
-// various NavigationThrottle related events.
+// Install a TestNavigationThrottle on all following requests and allows waiting
+// for various NavigationThrottle related events. Waiting works only for the
+// immediately next navigation. New instances are needed to wait for further
+// navigations.
 class TestNavigationThrottleInstaller : public WebContentsObserver {
  public:
   TestNavigationThrottleInstaller(
       WebContents* web_contents,
       NavigationThrottle::ThrottleCheckResult will_start_result,
       NavigationThrottle::ThrottleCheckResult will_redirect_result,
       NavigationThrottle::ThrottleCheckResult will_process_result)
       : WebContentsObserver(web_contents),
         will_start_result_(will_start_result),
         will_redirect_result_(will_redirect_result),
@@ skipping 79 matching lines @@
   NavigationThrottle::ThrottleCheckResult will_process_result_;
   int will_start_called_;
   int will_redirect_called_;
   int will_process_called_;
   TestNavigationThrottle* navigation_throttle_;
   scoped_refptr<MessageLoopRunner> will_start_loop_runner_;
   scoped_refptr<MessageLoopRunner> will_redirect_loop_runner_;
   scoped_refptr<MessageLoopRunner> will_process_loop_runner_;
 };
 
+// Records all navigation start URLs from the WebContents.
+class NavigationStartUrlRecorder : public WebContentsObserver {
+ public:
+  NavigationStartUrlRecorder(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  void DidStartNavigation(NavigationHandle* navigation_handle) override {
+    urls_.push_back(navigation_handle->GetURL());
+  }
+
+  const std::vector<GURL>& urls() const { return urls_; }
+
+ private:
+  std::vector<GURL> urls_;
+};
+
 }  // namespace
 
 class NavigationHandleImplBrowserTest : public ContentBrowserTest {
  protected:
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(embedded_test_server()->Start());
     SetupCrossSiteRedirector(embedded_test_server());
   }
 };
@@ skipping 342 matching lines @@
   // Wait for the end of the navigation.
   navigation_observer.Wait();
 
   EXPECT_TRUE(observer.has_committed());
   EXPECT_TRUE(observer.was_redirected());
   EXPECT_FALSE(observer.is_error());
   EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(),
             GURL(embedded_test_server()->GetURL("bar.com", "/title2.html")));
 }
 
+// Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
+// URL from the very beginning of the navigation.
+class NavigationHandleImplHttpsUpgradeBrowserTest
+    : public NavigationHandleImplBrowserTest {
+ public:
+  void CheckHttpsUpgradedIframeNavigation(const GURL& start_url,
+                                          const GURL& iframe_secure_url) {
+    ASSERT_TRUE(start_url.SchemeIs(url::kHttpScheme));
+    ASSERT_TRUE(iframe_secure_url.SchemeIs(url::kHttpsScheme));
+
+    NavigationStartUrlRecorder url_recorder(shell()->web_contents());
+    TestNavigationThrottleInstaller installer(
+        shell()->web_contents(), NavigationThrottle::PROCEED,
+        NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+    TestNavigationManager navigation_manager(shell()->web_contents(),
+                                             iframe_secure_url);
+
+    // Load the page and wait for the frame load with the expected URL.
+    // Note: if the test times out while waiting then a navigation to
+    // iframe_secure_url never happened and the expected upgrade may not be
+    // working.
+    shell()->LoadURL(start_url);
+    navigation_manager.WaitForWillStartRequest();
+
+    // The main frame should have finished navigating while the iframe should
+    // have just started.
+    EXPECT_EQ(2, installer.will_start_called());
+    EXPECT_EQ(0, installer.will_redirect_called());
+    EXPECT_EQ(1, installer.will_process_called());
+
+    // Check the correct start URLs have been registered.
+    EXPECT_EQ(iframe_secure_url, url_recorder.urls().back());
+    EXPECT_EQ(start_url, url_recorder.urls().front());
+    EXPECT_EQ(2ul, url_recorder.urls().size());
+  }
+};
+
+// Tests that the start URL is HTTPS upgraded for a same site navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplHttpsUpgradeBrowserTest,
+                       StartUrlIsHttpsUpgradedSameSite) {
+  GURL start_url(
+      embedded_test_server()->GetURL("/https_upgrade_same_site.html"));
+
+  // Builds the expected upgraded URL.
+  GURL::Replacements replace_scheme;
+  replace_scheme.SetSchemeStr("https");
+  GURL cross_site_iframe_secure_url = embedded_test_server()
+                                          ->GetURL("/title1.html")
+                                          .ReplaceComponents(replace_scheme);
+
+  CheckHttpsUpgradedIframeNavigation(start_url, cross_site_iframe_secure_url);
+}
+
+// Tests that the start URL is HTTPS upgraded for a cross site navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplHttpsUpgradeBrowserTest,
+                       StartUrlIsHttpsUpgradedCrossSite) {
+  GURL start_url(
+      embedded_test_server()->GetURL("/https_upgrade_cross_site.html"));
+
+  // Builds the expected upgraded URL.
+  GURL::Replacements replace_scheme;
+  replace_scheme.SetSchemeStr("https");
+  replace_scheme.SetPortStr("");
+  GURL cross_site_iframe_secure_url = embedded_test_server()
+                                          ->GetURL("other.com", "/title1.html")
+                                          .ReplaceComponents(replace_scheme);
+  ASSERT_FALSE(cross_site_iframe_secure_url.has_port());

[Mike West, 2016/07/06 06:55:35]

Nit: If this is necessary to make the test pass, please add a `// TODO(mkwst):
Upgrades shouldn't modify the port.` and file a bug. It surprises me that this
is the case, since we have blink tests that explicitly rely on the fact that we
don't modify the port (see
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
for example).

[carlosk, 2016/07/06 08:46:21]

I was wrong to place the blame in the upgrade code. The URL has no port from the
start being obtained directly and fully from the HTML code which is to be
expected.

For the same site case Chrome ads the server+port from the main document because
the HTML doesn't specify one hence the difference in behavior.

So I simplified the code and removed the ASSERT.

+
+  CheckHttpsUpgradedIframeNavigation(start_url, cross_site_iframe_secure_url);
+}
+
 }  // namespace content