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

[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),
         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();

[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;
     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

[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(
       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

[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(
       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_);

[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;
   }
 
   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:
   void DidStartNavigation(NavigationHandle* handle) override {

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

If we make it work for one navigation only, we should probably not install a
throttle if will_start was called.

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

I am currently relying on that behavior as it updates the internal counters.

     std::unique_ptr<NavigationThrottle> throttle(new TestNavigationThrottle(
         handle, will_start_result_, will_redirect_result_, will_process_result_,
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillStartRequest,
                    base::Unretained(this)),
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillRedirectRequest,
                    base::Unretained(this)),
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillProcessResponse,
                    base::Unretained(this))));
     navigation_throttle_ = static_cast<TestNavigationThrottle*>(throttle.get());
     handle->RegisterThrottleForTesting(std::move(throttle));
@@ skipping 392 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) {

[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