Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE.

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 "base/memory/weak_ptr.h"
 #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/common/request_context_type.h"
@@ skipping 129 matching lines @@
       : NavigationThrottle(handle),
         will_start_result_(will_start_result),
         will_redirect_result_(will_redirect_result),
         will_process_result_(will_process_result),
         did_call_will_start_(did_call_will_start),
         did_call_will_redirect_(did_call_will_redirect),
         did_call_will_process_(did_call_will_process) {}
   ~TestNavigationThrottle() override {}
 
   void Resume() { navigation_handle()->Resume(); }
+  void Cancel(NavigationThrottle::ThrottleCheckResult result) {
+    navigation_handle()->CancelDeferredNavigation(result);
+  }
 
   RequestContextType request_context_type() { return request_context_type_; }
 
  private:
   // NavigationThrottle implementation.
   NavigationThrottle::ThrottleCheckResult WillStartRequest() override {
     NavigationHandleImpl* navigation_handle_impl =
         static_cast<NavigationHandleImpl*>(navigation_handle());
     CHECK_NE(REQUEST_CONTEXT_TYPE_UNSPECIFIED,
              navigation_handle_impl->request_context_type());
@@ skipping 27 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_;
   RequestContextType request_context_type_;
 };
 
-// Install a TestNavigationThrottle on all following requests and allows waiting
-// for various NavigationThrottle related events. Waiting works only for the
-// immediately next navigation. New instances are needed to wait for further
-// navigations.
+// Installs a TestNavigationThrottle either on all following requests or on
+// requests with an expected starting URL, and allows waiting for various
+// NavigationThrottle related events. Waiting works only for the immediately
+// next navigation. New instances are needed to wait for further navigations.
 class TestNavigationThrottleInstaller : public WebContentsObserver {
  public:
   TestNavigationThrottleInstaller(
       WebContents* web_contents,
       NavigationThrottle::ThrottleCheckResult will_start_result,
       NavigationThrottle::ThrottleCheckResult will_redirect_result,
-      NavigationThrottle::ThrottleCheckResult will_process_result)
+      NavigationThrottle::ThrottleCheckResult will_process_result,
+      GURL expected_start_url = GURL())
       : 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),
+        expected_start_url_(expected_start_url),
         weak_factory_(this) {}
   ~TestNavigationThrottleInstaller() override {}
 
   TestNavigationThrottle* navigation_throttle() { return navigation_throttle_; }
 
   void WaitForThrottleWillStart() {
     if (will_start_called_)
       return;
     will_start_loop_runner_ = new MessageLoopRunner();
     will_start_loop_runner_->Run();
     will_start_loop_runner_ = nullptr;
   }
 
   void WaitForThrottleWillRedirect() {
     if (will_redirect_called_)
       return;
     will_redirect_loop_runner_ = new MessageLoopRunner();
     will_redirect_loop_runner_->Run();
     will_redirect_loop_runner_ = nullptr;
   }
 
   void WaitForThrottleWillProcess() {
     if (will_process_called_)
       return;
     will_process_loop_runner_ = new MessageLoopRunner();
     will_process_loop_runner_->Run();
     will_process_loop_runner_ = nullptr;
   }
 
+  void Continue(NavigationThrottle::ThrottleCheckResult result) {
+    ASSERT_NE(NavigationThrottle::DEFER, result);
+    if (result == NavigationThrottle::PROCEED)
+      navigation_throttle()->Resume();
+    else
+      navigation_throttle()->Cancel(result);
+  }
+
   int will_start_called() { return will_start_called_; }
   int will_redirect_called() { return will_redirect_called_; }
   int will_process_called() { return will_process_called_; }
 
+ protected:
+  virtual void DidCallWillStartRequest() {
+    will_start_called_++;
+    if (will_start_loop_runner_)
+      will_start_loop_runner_->Quit();
+  }
+
+  virtual void DidCallWillRedirectRequest() {
+    will_redirect_called_++;
+    if (will_redirect_loop_runner_)
+      will_redirect_loop_runner_->Quit();
+  }
+
+  virtual void DidCallWillProcessResponse() {
+    will_process_called_++;
+    if (will_process_loop_runner_)
+      will_process_loop_runner_->Quit();
+  }
+
  private:
   void DidStartNavigation(NavigationHandle* handle) override {
+    if (!expected_start_url_.is_empty() &&
+        handle->GetURL() != expected_start_url_)
+      return;
+
     std::unique_ptr<NavigationThrottle> throttle(new TestNavigationThrottle(
         handle, will_start_result_, will_redirect_result_, will_process_result_,
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillStartRequest,
                    weak_factory_.GetWeakPtr()),
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillRedirectRequest,
                    weak_factory_.GetWeakPtr()),
         base::Bind(&TestNavigationThrottleInstaller::DidCallWillProcessResponse,
                    weak_factory_.GetWeakPtr())));
     navigation_throttle_ = static_cast<TestNavigationThrottle*>(throttle.get());
     handle->RegisterThrottleForTesting(std::move(throttle));
   }
 
   void DidFinishNavigation(NavigationHandle* handle) override {
     if (!navigation_throttle_)
       return;
 
     if (handle == navigation_throttle_->navigation_handle())
       navigation_throttle_ = nullptr;
   }
 
-  void DidCallWillStartRequest() {
-    will_start_called_++;
-    if (will_start_loop_runner_)
-      will_start_loop_runner_->Quit();
-  }
-
-  void DidCallWillRedirectRequest() {
-    will_redirect_called_++;
-    if (will_redirect_loop_runner_)
-      will_redirect_loop_runner_->Quit();
-  }
-
-  void DidCallWillProcessResponse() {
-    will_process_called_++;
-    if (will_process_loop_runner_)
-      will_process_loop_runner_->Quit();
-  }
-
   NavigationThrottle::ThrottleCheckResult will_start_result_;
   NavigationThrottle::ThrottleCheckResult will_redirect_result_;
   NavigationThrottle::ThrottleCheckResult will_process_result_;
   int will_start_called_;
   int will_redirect_called_;
   int will_process_called_;
   TestNavigationThrottle* navigation_throttle_;
   scoped_refptr<MessageLoopRunner> will_start_loop_runner_;
   scoped_refptr<MessageLoopRunner> will_redirect_loop_runner_;
   scoped_refptr<MessageLoopRunner> will_process_loop_runner_;
+  GURL expected_start_url_;
 
   // The throttle installer can be deleted before all tasks posted by its
   // throttles are run, so it must be referenced via weak pointers.
   base::WeakPtrFactory<TestNavigationThrottleInstaller> weak_factory_;
 };
 
+// Same as above, but installs NavigationThrottles that do not directly return
+// the pre-programmed check results, but first DEFER the navigation at each
+// stage and then resume/cancel asynchronously.
+class TestDeferringNavigationThrottleInstaller
+    : public TestNavigationThrottleInstaller {
+ public:
+  TestDeferringNavigationThrottleInstaller(
+      WebContents* web_contents,
+      NavigationThrottle::ThrottleCheckResult will_start_result,
+      NavigationThrottle::ThrottleCheckResult will_redirect_result,
+      NavigationThrottle::ThrottleCheckResult will_process_result,
+      GURL expected_start_url = GURL())
+      : TestNavigationThrottleInstaller(web_contents,
+                                        NavigationThrottle::DEFER,
+                                        NavigationThrottle::DEFER,
+                                        NavigationThrottle::DEFER,
+                                        expected_start_url),
+        will_start_deferred_result_(will_start_result),
+        will_redirect_deferred_result_(will_redirect_result),
+        will_process_deferred_result_(will_process_result) {}
+
+ protected:
+  void DidCallWillStartRequest() override {
+    TestNavigationThrottleInstaller::DidCallWillStartRequest();
+    Continue(will_start_deferred_result_);
+  }
+
+  void DidCallWillRedirectRequest() override {
+    TestNavigationThrottleInstaller::DidCallWillStartRequest();
+    Continue(will_redirect_deferred_result_);
+  }
+
+  void DidCallWillProcessResponse() override {
+    TestNavigationThrottleInstaller::DidCallWillStartRequest();
+    Continue(will_process_deferred_result_);
+  }
+
+ private:
+  NavigationThrottle::ThrottleCheckResult will_start_deferred_result_;
+  NavigationThrottle::ThrottleCheckResult will_redirect_deferred_result_;
+  NavigationThrottle::ThrottleCheckResult will_process_deferred_result_;
+};
+
 // Records all navigation start URLs from the WebContents.
 class NavigationStartUrlRecorder : public WebContentsObserver {
  public:
   NavigationStartUrlRecorder(WebContents* web_contents)
       : WebContentsObserver(web_contents) {}
 
   void DidStartNavigation(NavigationHandle* navigation_handle) override {
     urls_.push_back(navigation_handle->GetURL());
   }
 
   const std::vector<GURL>& urls() const { return urls_; }
 
  private:
   std::vector<GURL> urls_;
 };
 
+bool IsChildFrameCollapsed(Shell* shell, const char* element_id) {
+  const char kScript[] =
+      "window.domAutomationController.send("
+      "  document.getElementById(\"%s\").clientWidth"
+      ");";
+  int client_width = 0;
+  EXPECT_TRUE(ExecuteScriptAndExtractInt(
+      shell, base::StringPrintf(kScript, element_id), &client_width));
+  return !client_width;
+}
+
 }  // namespace
 
 class NavigationHandleImplBrowserTest : public ContentBrowserTest {
  protected:
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
     SetupCrossSiteRedirector(embedded_test_server());
     ASSERT_TRUE(embedded_test_server()->Start());
   }
 };
@@ skipping 387 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 a NavigationThrottle can block the navigation and collapse the
+// frame owner both on request start as well as after a redirect. Plus, ensure
+// that the frame is restored on the subsequent non-error-page navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
+                       ThrottleBlockAndCollapse) {
+  const char kChildFrameId[] = "child0";
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/frame_tree/page_with_one_frame.html"));
+  GURL blocked_subframe_url(embedded_test_server()->GetURL(
+      "a.com", "/cross-site/baz.com/title1.html"));
+  GURL allowed_subframe_url(embedded_test_server()->GetURL(
+      "a.com", "/cross-site/baz.com/title2.html"));

[clamy, 2017/02/21 15:12:49]

It's not clear from these urls that they will redirect. Is that the case?

[engedy, 2017/02/22 13:14:58]

Yes, the testing throttle would only return BLOCK_AND_COLLAPSE from
WillRedirectRequest() in the iteration when |block_on_redirect| is set, so the
test would inevitably fail otherwise.

The `/cross-site/' prefix is called the `HandleCrossSiteRedirect` handler, so I
think it should be okay to use. Especially so given it is important here to make
these cross-site navigations, and using the `/server-redirect?' handler to
change the host is awkward because we would need to manually supply the correct
port the embedded server is running on.

[clamy, 2017/02/22 13:18:29]

Acknowledged.

+  GURL allowed_subframe_final_url(
+      embedded_test_server()->GetURL("baz.com", "/title2.html"));
+
+  // Exercise both synchronous and deferred throttle check results, and both on
+  // WillStartRequest and on WillRedirectRequest.
+  for (const bool deferred_block : {false, true}) {
+    for (const bool block_on_redirect : {false, true}) {
+      SCOPED_TRACE(deferred_block ? "Direct block" : "Deferred block");
+      SCOPED_TRACE(block_on_redirect ? "Block on WillStartRequest"
+                                     : "Block on WillRedirectRequest");
+
+      NavigationThrottle::ThrottleCheckResult will_start_result =
+          block_on_redirect ? NavigationThrottle::PROCEED
+                            : NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE;
+      NavigationThrottle::ThrottleCheckResult will_redirect_result =
+          block_on_redirect ? NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE
+                            : NavigationThrottle::PROCEED;
+
+      std::unique_ptr<TestNavigationThrottleInstaller>
+          subframe_throttle_installer;
+      if (deferred_block) {
+        subframe_throttle_installer.reset(
+            new TestDeferringNavigationThrottleInstaller(
+                shell()->web_contents(), will_start_result,
+                will_redirect_result, NavigationThrottle::PROCEED,
+                blocked_subframe_url));
+      } else {
+        subframe_throttle_installer.reset(new TestNavigationThrottleInstaller(
+            shell()->web_contents(), will_start_result, will_redirect_result,
+            NavigationThrottle::PROCEED, blocked_subframe_url));
+      }
+
+      {
+        SCOPED_TRACE("Initial navigation blocked on main frame load.");
+        NavigationHandleObserver subframe_observer(shell()->web_contents(),
+                                                   blocked_subframe_url);
+
+        ASSERT_TRUE(NavigateToURL(shell(), main_url));
+        EXPECT_TRUE(subframe_observer.is_error());
+        EXPECT_TRUE(IsChildFrameCollapsed(shell(), kChildFrameId));
+      }
+
+      {
+        SCOPED_TRACE("Subsequent subframe navigation is allowed.");
+        NavigationHandleObserver subframe_observer(shell()->web_contents(),
+                                                   allowed_subframe_url);
+
+        ASSERT_TRUE(NavigateIframeToURL(shell()->web_contents(), kChildFrameId,
+                                        allowed_subframe_url));
+        EXPECT_TRUE(subframe_observer.has_committed());
+        EXPECT_FALSE(subframe_observer.is_error());
+        EXPECT_EQ(allowed_subframe_final_url,
+                  subframe_observer.last_committed_url());
+        EXPECT_FALSE(IsChildFrameCollapsed(shell(), kChildFrameId));
+      }
+
+      {
+        SCOPED_TRACE("Subsequent subframe navigation is blocked.");
+        NavigationHandleObserver subframe_observer(shell()->web_contents(),
+                                                   blocked_subframe_url);
+
+        ASSERT_TRUE(NavigateIframeToURL(shell()->web_contents(), kChildFrameId,
+                                        blocked_subframe_url));
+
+        EXPECT_TRUE(subframe_observer.is_error());
+        EXPECT_TRUE(IsChildFrameCollapsed(shell(), kChildFrameId));
+      }
+    }
+  }
+}
+
 // Checks that the RequestContextType value is properly set.
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
                        VerifyRequestContextTypeForFrameTree) {
   GURL main_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b(c))"));
   GURL b_url(embedded_test_server()->GetURL(
       "b.com", "/cross_site_iframe_factory.html?b(c())"));
   GURL c_url(embedded_test_server()->GetURL(
       "c.com", "/cross_site_iframe_factory.html?c()"));
 
   TestNavigationThrottleInstaller installer(
       shell()->web_contents(), NavigationThrottle::PROCEED,
       NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
   TestNavigationManager main_manager(shell()->web_contents(), main_url);
   TestNavigationManager b_manager(shell()->web_contents(), b_url);
   TestNavigationManager c_manager(shell()->web_contents(), c_url);
   NavigationStartUrlRecorder url_recorder(shell()->web_contents());
   TestNavigationThrottle* previous_throttle = nullptr;
 
   // Starts and verifies the main frame navigation.
   shell()->LoadURL(main_url);
   EXPECT_TRUE(main_manager.WaitForRequestStart());
   // The throttle should not be null.
   EXPECT_NE(previous_throttle, installer.navigation_throttle());
-  // Checks the only URL recorded so far is the one expected for the main frame.
+  // Checks the only URL recorded so far is the one expected for the main
+  // frame.
   EXPECT_EQ(main_url, url_recorder.urls().back());
   EXPECT_EQ(1ul, url_recorder.urls().size());
   // Checks the main frame RequestContextType.
   EXPECT_EQ(REQUEST_CONTEXT_TYPE_LOCATION,
             installer.navigation_throttle()->request_context_type());
   // For each navigations the throttle should be a different instance.
   previous_throttle = installer.navigation_throttle();
 
   // Ditto for frame b navigation.
   main_manager.WaitForNavigationFinished();
@@ skipping 51 matching lines @@
   EXPECT_EQ(REQUEST_CONTEXT_TYPE_HYPERLINK,
             installer.navigation_throttle()->request_context_type());
 
   // Finishes the last navigation.
   link_manager.WaitForNavigationFinished();
   EXPECT_FALSE(installer.navigation_throttle());
 }
 
 // Checks that the RequestContextType value is properly set for an form (POST).
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
                        VerifyFormRequestContextType) {

[clamy, 2017/02/21 15:12:49]

Could you also update this test to check that we get the expected error code on
the throttle? Thanks!

[engedy, 2017/02/22 13:14:58]

I'm somewhat puzzled by this comment, could you please elaborate?

[clamy, 2017/02/22 13:18:29]

Sorry I posted this comment on the wrong test. I meant the thing above, but with
ErrorCodeOnThrottleCancelNavigation (ie check that when the Throttle returns
this new result, we have an appropriate error code on the NavigationHandle).

[engedy, 2017/02/22 14:34:06]

Done, thanks for requesting this, this uncovered a bug in this CL, which I have
fixed.

Unfortunately, it also uncovered a more fundamental issue present when
PlzNavigate is disabled. The ResourceMsg_ReceivedRedirect message is never sent
if the NavigationThrottle blocks the load, only ResourceMsg_RequestComplete is
sent. Hence the renderer never learns of the redirect, so the provisional load
fails on the renderer side with the pre-redirect URL.

This means that the error page is also committed under the pre-redirect URL,
which causes a mismatch in TakeNavigationHandleForCommit, because the
NavigationHandle already has the post-redirect URL.

What do you suggest?

   GURL document_url(
       embedded_test_server()->GetURL("/session_history/form.html"));
   GURL post_url(embedded_test_server()->GetURL("/echotitle"));
 
   TestNavigationThrottleInstaller installer(
       shell()->web_contents(), NavigationThrottle::PROCEED,
       NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
   TestNavigationManager post_manager(shell()->web_contents(), post_url);
   NavigationStartUrlRecorder url_recorder(shell()->web_contents());
 
@@ skipping 208 matching lines @@
         NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
     NavigationHandleObserver observer(shell()->web_contents(), url);
     EXPECT_TRUE(NavigateToURL(shell(), url));
     EXPECT_EQ(1, installer.will_start_called());
     EXPECT_EQ(1, installer.will_process_called());
     EXPECT_FALSE(observer.is_same_page());
   }
 }
 
 }  // namespace content