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..172828f6cd00efa09fa71bb684ef6a7a020f2471 100644
--- a/content/browser/frame_host/navigation_handle_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_handle_impl_browsertest.cc
@@ -21,9 +21,13 @@ namespace content {
 
 namespace {
 
+// Gathers data from the NavigationHandle assigned to navigations that start
+// with the expected URL. Overlapping navigations are ignored; create new

[clamy, 2016/07/04 11:22:09]

It's not clear to me whether this comment means "You should create new instances
as needed" or "The NavigationHandleObserver creates new instances as needed".
Also I don't really get what you mean by "Overlapping navigations are ignored".

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

I agree this was confusing so I just removed that last sentence.
This was a leftover from my previous change where I had misunderstood the
behavior.

+// 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),
@@ -36,11 +40,13 @@ class NavigationHandleObserver : public WebContentsObserver {
         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();

[clamy, 2016/07/04 11:22:09]

Rather than putting a new member for the start urls, which doesn't really fit
with what the class is doing, how about creating a new Observer that just
records all urls it has seen (in something like a vector). Then we can check if
the url was never in, which seems a bit more robust to me.

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

Done.

       return;
+    }
 
     handle_ = navigation_handle;
     has_committed_ = false;
@@ -96,6 +102,7 @@ class NavigationHandleObserver : public WebContentsObserver {
   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_; }
@@ -115,12 +122,15 @@ class NavigationHandleObserver : public WebContentsObserver {
   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

[clamy, 2016/07/04 11:22:09]

Is there any case where it is actually instantiated directly in tests? If not,
the comment should reflect that.

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

Done.

+// TestNavigationThrottleInstaller.
 class TestNavigationThrottle : public NavigationThrottle {
  public:
   TestNavigationThrottle(
@@ -170,7 +180,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

[clamy, 2016/07/04 11:22:09]

I think it be better to rephrase the first sentence as "Install a
TestNavigationThrottle on the next navigation and allows waiting for various
NavigationThrottle related events." This way it's clearer it only works for one
navigation.

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

That would be incorrect as it continues to install them for all following
navigations and keeps on incrementing the internal counters. See my current
usage of it.

But the waiting is still really impossible for the 2nd and further navigations.

+// immediately next navigation; create new instances as needed.
 class TestNavigationThrottleInstaller : public WebContentsObserver {
  public:
   TestNavigationThrottleInstaller(
@@ -191,24 +202,21 @@ class TestNavigationThrottleInstaller : public WebContentsObserver {
   TestNavigationThrottle* navigation_throttle() { return navigation_throttle_; }
 
   void WaitForThrottleWillStart() {
-    if (will_start_called_)
-      return;
+    CHECK(!will_start_called_);

[clamy, 2016/07/04 11:22:09]

This is not right. It's likely to fail with PlzNavigate enabled, as it's
perfectly possible to have will_start_called _before_ we actually start to wait
for this to happen (first navigation, it gets called right away).

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

Reverted.

Now I understand why this was done this way. It had ran correctly with
PlzNavigate enabled so I guess this is a timing issue.

     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;
@@ -632,4 +640,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) {

[clamy, 2016/07/04 11:22:09]

Nit: how about renaming it "StartWithUpgradedUrl". This way once we make it work
with PlzNavigate it still has meaning. That would also require to rewrite the
comment: "Tests that the NavigationHandle gets the url upgraded to HTTPS from
the beginning of the navigation." which is what we want to ensure.

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

Done. I changed the structure of the test a bit but kept the concept you
suggested.

+  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);

[clamy, 2016/07/04 11:22:09]

I think this can be rewritten using the TestNavigationManager from
content/test/content_browser_test_utils_internal.h. Instantiate a
TestNavigationManager for upgraded url and WaitForWillStartRequest().

Then as mentioned above, you can have another observer that just records the
urls seen, and check that the unupgraded one was never received.

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

Done.

+  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