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 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)
       : 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) {}
 
   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();
     is_renderer_initiated_ = navigation_handle->IsRendererInitiated();
     is_synchronous_ = navigation_handle->IsSynchronousNavigation();
     is_srcdoc_ = navigation_handle->IsSrcdoc();
     was_redirected_ = navigation_handle->WasServerRedirect();
     frame_tree_node_id_ = navigation_handle->GetFrameTreeNodeId();
   }
 
   void DidFinishNavigation(NavigationHandle* navigation_handle) override {
     if (navigation_handle != handle_)
       return;
 
     DCHECK_EQ(is_main_frame_, navigation_handle->IsInMainFrame());
     DCHECK_EQ(is_parent_main_frame_, navigation_handle->IsParentMainFrame());
     DCHECK_EQ(is_synchronous_, navigation_handle->IsSynchronousNavigation());
     DCHECK_EQ(is_renderer_initiated_, navigation_handle->IsRendererInitiated());
     DCHECK_EQ(is_srcdoc_, navigation_handle->IsSrcdoc());
     DCHECK_EQ(frame_tree_node_id_, navigation_handle->GetFrameTreeNodeId());
 
     was_redirected_ = navigation_handle->WasServerRedirect();
 
     if (navigation_handle->HasCommitted()) {
       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;
       }
     } else {
       has_committed_ = false;
       is_error_ = true;
     }
 
     handle_ = nullptr;
   }
 
   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_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_; }
 
  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 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(
       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_);

[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;
   }
 
   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 66 matching lines @@
     // if it came from the omnibox.
     GURL url(embedded_test_server()->GetURL("/title1.html"));
     NavigationHandleObserver observer(shell()->web_contents(), url);
     ui::PageTransition expected_transition = ui::PageTransitionFromInt(
         ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
 
     EXPECT_TRUE(NavigateToURL(shell(), url));
 
     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));
   }
 
   {
     // Test navigating to a page with subframe. The subframe will have
     // PageTransition of type AUTO_SUBFRAME.
     GURL url(
         embedded_test_server()->GetURL("/frame_tree/page_with_one_frame.html"));
     NavigationHandleObserver observer(
         shell()->web_contents(),
         embedded_test_server()->GetURL("/cross-site/baz.com/title1.html"));
     ui::PageTransition expected_transition =
         ui::PageTransitionFromInt(ui::PAGE_TRANSITION_AUTO_SUBFRAME);
 
     EXPECT_TRUE(NavigateToURL(shell(), url));
 
     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());
   }
 }
 
 // Ensure that the following methods on NavigationHandle behave correctly:
 // * IsInMainFrame
 // * IsParentMainFrame
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyFrameTree) {
@@ skipping 10 matching lines @@
 
   EXPECT_TRUE(NavigateToURL(shell(), main_url));
 
   FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
                             ->GetFrameTree()
                             ->root();
 
   // 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(),
             b_observer.frame_tree_node_id());
 
   // 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(),
             c_observer.frame_tree_node_id());
 }
 
 // Ensure that the WasRedirected() method on NavigationHandle behaves correctly.
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyRedirect) {
   {
     GURL url(embedded_test_server()->GetURL("/title1.html"));
@@ skipping 248 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());
+
+  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