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

content/browser/frame_host/navigation_handle_impl_browsertest.cc

Index: content/browser/frame_host/navigation_handle_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_handle_impl_browsertest.cc b/content/browser/frame_host/navigation_handle_impl_browsertest.cc
index d9ab400ec2e47a47b33fb7fb5f526066d0595c13..8aeb305453bb6c21608fa5f5888aaa7f3240ceb5 100644
--- a/content/browser/frame_host/navigation_handle_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_handle_impl_browsertest.cc
@@ -21,6 +21,9 @@ namespace content {
 
 namespace {
 
+// Gathers data from the NavigationHandle assigned to the next navigation that
+// happens to the expected URL. All subsequent navigations are ignored; create
+// new instances as needed.
 class NavigationHandleObserver : public WebContentsObserver {
  public:
   NavigationHandleObserver(WebContents* web_contents, const GURL& expected_url)
@@ -39,14 +42,12 @@ class NavigationHandleObserver : public WebContentsObserver {
         expected_url_(expected_url) {}
 
   void DidStartNavigation(NavigationHandle* navigation_handle) override {
-    if (handle_ || navigation_handle->GetURL() != expected_url_)
+    if (handle_ || navigation_handle->GetURL() != expected_url_) {
+      last_unmatched_url_ = navigation_handle->GetURL();

[carlosk, 2016/06/28 14:44:32]

I added this new member to allow for more meaningful error messages for
failures. See usage in new test.

       return;
+    }
 
     handle_ = navigation_handle;
-    has_committed_ = false;
-    is_error_ = false;
-    page_transition_ = ui::PAGE_TRANSITION_LINK;
-    last_committed_url_ = GURL();

[carlosk, 2016/06/28 14:44:32]

These were in fact never updated given the if-check for handle_ in the beginning
of this method. The comment added to the class now reflects that.

[nasko, 2016/06/28 22:38:20]

I'm not sure I follow. DidStartNavigation will be called multiple times for
WebContents. The handle_ check ensures that it doesn't track more than one
handle at a time, but it can very well track multiple. At the
DidFinishNavigation end, the handle_ is reset, so a subsequent navigation to the
expected URL will still go through here.

[carlosk, 2016/07/01 20:22:51]

Yes, you are right, I missed that. Reverted and made a few more changes to make
it all clearer.

 
     is_main_frame_ = navigation_handle->IsInMainFrame();
     is_parent_main_frame_ = navigation_handle->IsParentMainFrame();
@@ -74,7 +75,7 @@ class NavigationHandleObserver : public WebContentsObserver {
       has_committed_ = true;
       if (!navigation_handle->IsErrorPage()) {
         page_transition_ = navigation_handle->GetPageTransition();
-        last_committed_url_ = navigation_handle->GetURL();
+        committed_url_ = navigation_handle->GetURL();
       } else {
         is_error_ = true;
       }
@@ -96,7 +97,8 @@ class NavigationHandleObserver : public WebContentsObserver {
   bool was_redirected() { return was_redirected_; }
   int frame_tree_node_id() { return frame_tree_node_id_; }
 
-  const GURL& last_committed_url() { return last_committed_url_; }
+  const GURL& last_unmatched_url() { return last_unmatched_url_; }
+  const GURL& committed_url() { return committed_url_; }

[carlosk, 2016/06/28 14:44:32]

Renamed this because it was confusing: last_... gave me the impression it would
be updated with further navigations which is not the case (it is only set once).

[nasko, 2016/06/28 22:38:20]

It actually can be updated multiple times : ).

[carlosk, 2016/07/01 20:22:52]

Reverted.

 
   ui::PageTransition page_transition() { return page_transition_; }
 
@@ -116,11 +118,14 @@ class NavigationHandleObserver : public WebContentsObserver {
   int frame_tree_node_id_;
   ui::PageTransition page_transition_;
   GURL expected_url_;
-  GURL last_committed_url_;
+  GURL last_unmatched_url_;
+  GURL 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(
@@ -170,7 +175,8 @@ class TestNavigationThrottle : public NavigationThrottle {
 };
 
 // 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(
@@ -191,24 +197,21 @@ class TestNavigationThrottleInstaller : public WebContentsObserver {
   TestNavigationThrottle* navigation_throttle() { return navigation_throttle_; }
 
   void WaitForThrottleWillStart() {
-    if (will_start_called_)
-      return;
+    CHECK(!will_start_called_);

[carlosk, 2016/06/28 14:44:32]

Switched this to a CHECK because calls are useless after the 1st navigation,
what was confusing to me at first. Also updated the class comment to explain it.

     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;
@@ -295,7 +298,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyPageTransition) {
 
     EXPECT_TRUE(observer.has_committed());
     EXPECT_FALSE(observer.is_error());
-    EXPECT_EQ(url, observer.last_committed_url());
+    EXPECT_EQ(url, observer.committed_url());
     EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs(
         observer.page_transition(), expected_transition));
   }
@@ -316,7 +319,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyPageTransition) {
     EXPECT_TRUE(observer.has_committed());
     EXPECT_FALSE(observer.is_error());
     EXPECT_EQ(embedded_test_server()->GetURL("baz.com", "/title1.html"),
-              observer.last_committed_url());
+              observer.committed_url());
     EXPECT_TRUE(ui::PageTransitionTypeIncludingQualifiersIs(
         observer.page_transition(), expected_transition));
     EXPECT_FALSE(observer.is_main_frame());
@@ -347,14 +350,14 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyFrameTree) {
   // Verify the main frame.
   EXPECT_TRUE(main_observer.has_committed());
   EXPECT_FALSE(main_observer.is_error());
-  EXPECT_EQ(main_url, main_observer.last_committed_url());
+  EXPECT_EQ(main_url, main_observer.committed_url());
   EXPECT_TRUE(main_observer.is_main_frame());
   EXPECT_EQ(root->frame_tree_node_id(), main_observer.frame_tree_node_id());
 
   // Verify the b.com frame.
   EXPECT_TRUE(b_observer.has_committed());
   EXPECT_FALSE(b_observer.is_error());
-  EXPECT_EQ(b_url, b_observer.last_committed_url());
+  EXPECT_EQ(b_url, b_observer.committed_url());
   EXPECT_FALSE(b_observer.is_main_frame());
   EXPECT_TRUE(b_observer.is_parent_main_frame());
   EXPECT_EQ(root->child_at(0)->frame_tree_node_id(),
@@ -363,7 +366,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyFrameTree) {
   // Verify the c.com frame.
   EXPECT_TRUE(c_observer.has_committed());
   EXPECT_FALSE(c_observer.is_error());
-  EXPECT_EQ(c_url, c_observer.last_committed_url());
+  EXPECT_EQ(c_url, c_observer.committed_url());
   EXPECT_FALSE(c_observer.is_main_frame());
   EXPECT_FALSE(c_observer.is_parent_main_frame());
   EXPECT_EQ(root->child_at(0)->child_at(0)->frame_tree_node_id(),
@@ -632,4 +635,65 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, ThrottleDefer) {
             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());
+
+  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.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_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