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. Overlapping navigations are ignored; create new
+// instances as needed.
 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_) {
+      last_unmatched_start_url_ = navigation_handle->GetURL();
       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();
     is_renderer_initiated_ = navigation_handle->IsRendererInitiated();
@@ skipping 35 matching lines @@
   bool has_committed() { return has_committed_; }
   bool is_error() { return is_error_; }
   bool is_main_frame() { return is_main_frame_; }
   bool is_parent_main_frame() { return is_parent_main_frame_; }
   bool is_renderer_initiated() { return is_renderer_initiated_; }
   bool is_synchronous() { return is_synchronous_; }
   bool is_srcdoc() { return is_srcdoc_; }
   bool was_redirected() { return was_redirected_; }
   int frame_tree_node_id() { return frame_tree_node_id_; }
 
+  const GURL& last_unmatched_start_url() { return last_unmatched_start_url_; }
   const GURL& last_committed_url() { return last_committed_url_; }
 
   ui::PageTransition page_transition() { return page_transition_; }
 
  private:
   // A reference to the NavigationHandle so this class will track only
   // one navigation at a time. It is set at DidStartNavigation and cleared
   // at DidFinishNavigation before the NavigationHandle is destroyed.
   NavigationHandle* handle_;
   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_unmatched_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. This is
+// generally 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 29 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.
+// various NavigationThrottle related events. Waiting is only allows for the
+// immediately next navigation; create new instances as needed.
 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),
         will_process_result_(will_process_result),
         will_start_called_(0),
         will_redirect_called_(0),
         will_process_called_(0),
         navigation_throttle_(nullptr) {}
   ~TestNavigationThrottleInstaller() override{};
 
   TestNavigationThrottle* navigation_throttle() { return navigation_throttle_; }
 
   void WaitForThrottleWillStart() {
-    if (will_start_called_)
-      return;
+    CHECK(!will_start_called_);
     will_start_loop_runner_ = new MessageLoopRunner();
     will_start_loop_runner_->Run();
     will_start_loop_runner_ = nullptr;
   }
 
   void WaitForThrottleWillRedirect() {
-    if (will_redirect_called_)
-      return;
+    CHECK(!will_redirect_called_);
     will_redirect_loop_runner_ = new MessageLoopRunner();
     will_redirect_loop_runner_->Run();
     will_redirect_loop_runner_ = nullptr;
   }
 
   void WaitForThrottleWillProcess() {
-    if (will_process_called_)
-      return;
+    CHECK(!will_process_called_);
     will_process_loop_runner_ = new MessageLoopRunner();
     will_process_loop_runner_->Run();
     will_process_loop_runner_ = nullptr;
   }
 
   int will_start_called() { return will_start_called_; }
   int will_redirect_called() { return will_redirect_called_; }
   int will_process_called() { return will_process_called_; }
 
  private:
@@ skipping 403 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")));
 }
 
+// Ensure that the URL received with the provisional load start notification is
+// already upgraded to HTTPS.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
+                       ProvisionalLoadStartUrlIsHttpsUpgraded) {
+  GURL start_url(embedded_test_server()->GetURL(
+      "/navigation_handle_https_upgrade_test.html"));
+  ASSERT_TRUE(start_url.SchemeIs(url::kHttpScheme));
+
+  // Builds the expected upgraded URL.
+  GURL::Replacements replace_scheme;
+  replace_scheme.SetSchemeStr("https");
+  replace_scheme.SetPortStr("");
+  GURL iframe_secure_url = embedded_test_server()
+                               ->GetURL("other.com", "/title1.html")
+                               .ReplaceComponents(replace_scheme);
+  ASSERT_TRUE(iframe_secure_url.SchemeIs(url::kHttpsScheme));
+  ASSERT_FALSE(iframe_secure_url.has_port());

[Mike West, 2016/07/04 08:33:25]

I'm missing something; how does this work? Don't you need to keep the random
port that `embedded_test_server()` comes up with, and do some sort of
port-replacement in the document in order for it to not timeout?

[carlosk, 2016/07/04 09:22:59]

I'm testing only if the URL received by the NavigationHandle upon navigation
start is the expected HTTPS-upgraded one. The upgrade process seems to remove
the port hence my cleaning it up here. This test works well enough for asserting
that.

I'm not testing that the frame document is actually loaded. In fact the frame
navigation is abandoned at the end of the test before it commits. So it doesn't
matter that the port information was removed.

[Mike West, 2016/07/04 09:31:43]

The upgrade should not remove the port. That is, `http://example.test:123/`
should become `https://example.test:123/` after the upgrade (similar to what
we're doing for HSTS: if it's not a default port, we leave it alone). See
`FrameFetchContextUpgradeTest::UpgradeInsecureResourceRequests` for instance...

If you needed to do this in order for things to work, then something's wrong. :(

[carlosk, 2016/07/05 09:57:23]

I updated the test so that it will check the same and cross site cases where
there's different behavior in regards to the the port. If this behavior is
incorrect it should be fix in another CL.

+
+  NavigationHandleObserver observer_main(shell()->web_contents(), start_url);
+  NavigationHandleObserver observer_iframe(shell()->web_contents(),
+                                           iframe_secure_url);
+  TestNavigationThrottleInstaller installer_main(
+      shell()->web_contents(), NavigationThrottle::PROCEED,
+      NavigationThrottle::PROCEED, NavigationThrottle::DEFER);
+
+  // Start loading the main frame and stop at WillProcess.
+  shell()->LoadURL(start_url);
+  installer_main.WaitForThrottleWillProcess();
+  EXPECT_EQ(1, installer_main.will_start_called());
+  EXPECT_EQ(0, installer_main.will_redirect_called());
+  EXPECT_EQ(1, installer_main.will_process_called());
+
+  // Create a new throttle installer and start the iframe navigation until
+  // WillStart.
+  TestNavigationThrottleInstaller installer_iframe(
+      shell()->web_contents(), NavigationThrottle::DEFER,
+      NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+  installer_main.navigation_throttle()->Resume();
+  installer_iframe.WaitForThrottleWillStart();
+  EXPECT_EQ(1, installer_iframe.will_start_called());
+  EXPECT_EQ(0, installer_iframe.will_redirect_called());
+  EXPECT_EQ(0, installer_iframe.will_process_called());
+
+  // Confirm the navigation of the main frame succeeded.
+  EXPECT_NE(-1, observer_main.frame_tree_node_id());
+  EXPECT_EQ(start_url, observer_main.last_committed_url());
+  EXPECT_TRUE(observer_main.has_committed());
+  EXPECT_TRUE(observer_main.is_main_frame());
+  EXPECT_FALSE(observer_main.is_renderer_initiated());
+
+  // Check that the handle observer did receive the start call with the correct
+  // URL.
+  ASSERT_NE(-1, observer_iframe.frame_tree_node_id())
+      << "The start URL \"" << observer_iframe.last_unmatched_start_url()
+      << "\" didn't match the expected \"" << iframe_secure_url << "\"";
+  EXPECT_FALSE(observer_iframe.has_committed());
+  EXPECT_FALSE(observer_iframe.is_main_frame());
+  EXPECT_TRUE(observer_iframe.is_parent_main_frame());
+  EXPECT_TRUE(observer_iframe.is_renderer_initiated());
+}
+
 }  // namespace content