Fix UaF when PlzNavigate is enabled and a NavigationThrottle cancels …

…the request in WillStartRequest.

This brings in a fix that was already in the tree from r460581 to r463510 (reverted for other reasons), and adds a test.

BUG=729832
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2926293002
Cr-Commit-Position: refs/heads/master@{#478060}

content/browser/frame_host/navigation_handle_impl_browsertest.cc

@@ -1079,6 +1079,31 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
   }
 }
 
+// Checks that there's no UAF if NavigationHandleImpl::WillStartRequest cancels
+// the navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
+                       CancelNavigationInWillStartRequest) {
+  const GURL kUrl1 = embedded_test_server()->GetURL("/title1.html");
+  const GURL kUrl2 = embedded_test_server()->GetURL("/title2.html");
+  // First make a successful commit, as this issue only reproduces when there
+  // are existing entries (i.e. when NavigationControllerImpl::GetVisibleEntry
+  // has safe_to_show_pending=false).
+  EXPECT_TRUE(NavigateToURL(shell(), kUrl1));
+
+  // To take the path that doesn't run beforeunload, so that
+  // NavigationControllerImpl::NavigateToPendingEntry is on the botttom of the
+  // stack when NavigationHandleImpl::WillStartRequest is called.
+  CrashTab(shell()->web_contents());
+
+  // Set up a NavigationThrottle that will cancel the navigation in
+  // WillStartRequest.
+  TestNavigationThrottleInstaller installer(
+      shell()->web_contents(), NavigationThrottle::CANCEL_AND_IGNORE,
+      NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+
+  EXPECT_FALSE(NavigateToURL(shell(), kUrl2));
+}
+
 // Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
 // URL from the very beginning of the navigation.
 class NavigationHandleImplHttpsUpgradeBrowserTest

content/browser/frame_host/navigation_request.cc

@@ -28,6 +28,7 @@
 #include "content/common/appcache_interfaces.h"
 #include "content/common/resource_request_body_impl.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/navigation_controller.h"
@@ -322,7 +323,8 @@ NavigationRequest::NavigationRequest(
       bindings_(NavigationEntryImpl::kInvalidBindings),
       response_should_be_rendered_(true),
       associated_site_instance_type_(AssociatedSiteInstanceType::NONE),
-      may_transfer_(may_transfer) {
+      may_transfer_(may_transfer),
+      weak_factory_(this) {
   DCHECK(!browser_initiated || (entry != nullptr && frame_entry != nullptr));
   TRACE_EVENT_ASYNC_BEGIN2("navigation", "NavigationRequest", this,
                            "frame_tree_node",
@@ -738,21 +740,26 @@ void NavigationRequest::OnStartChecksComplete(
 
   if (on_start_checks_complete_closure_)
     on_start_checks_complete_closure_.Run();
-
   // Abort the request if needed. This will destroy the NavigationRequest.
   if (result == NavigationThrottle::CANCEL_AND_IGNORE ||
-      result == NavigationThrottle::CANCEL) {
+      result == NavigationThrottle::CANCEL ||
+      result == NavigationThrottle::BLOCK_REQUEST ||
+      result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE) {
     // TODO(clamy): distinguish between CANCEL and CANCEL_AND_IGNORE.
-    OnRequestFailed(false, net::ERR_ABORTED);
-
-    // DO NOT ADD CODE after this. The previous call to OnRequestFailed has
-    // destroyed the NavigationRequest.
-    return;
-  }
+    int error_code = net::ERR_ABORTED;
+    if (result == NavigationThrottle::BLOCK_REQUEST ||
+        result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE) {
+      error_code = net::ERR_BLOCKED_BY_CLIENT;
+    }
 
-  if (result == NavigationThrottle::BLOCK_REQUEST ||
-      result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE) {
-    OnRequestFailed(false, net::ERR_BLOCKED_BY_CLIENT);
+    // If the start checks completed synchronously, which could happen if there
+    // is no onbeforeunload handler or if a NavigationThrottle cancelled it,
+    // then this could cause reentrancy into NavigationController. So use a
+    // PostTask to avoid that.
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&NavigationRequest::OnRequestFailed,
+                   weak_factory_.GetWeakPtr(), false, error_code));
 
     // DO NOT ADD CODE after this. The previous call to OnRequestFailed has
     // destroyed the NavigationRequest.

content/browser/frame_host/navigation_request.h

@@ -10,6 +10,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/loader/navigation_url_loader_delegate.h"
 #include "content/common/content_export.h"
@@ -285,6 +286,8 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
 
   base::Closure on_start_checks_complete_closure_;
 
+  base::WeakPtrFactory<NavigationRequest> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NavigationRequest);
 };
 

content/public/test/navigation_simulator.cc

@@ -556,13 +556,18 @@ void NavigationSimulator::OnWillProcessResponse() {
 void NavigationSimulator::WaitForThrottleChecksComplete() {
   // If last_throttle_check_result_ is set, then throttle checks completed
   // synchronously.
-  if (last_throttle_check_result_)
-    return;
+  if (!last_throttle_check_result_) {
+    base::RunLoop run_loop;
+    throttle_checks_wait_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+    throttle_checks_wait_closure_.Reset();
+  }
 
-  base::RunLoop run_loop;
-  throttle_checks_wait_closure_ = run_loop.QuitClosure();
-  run_loop.Run();
-  throttle_checks_wait_closure_.Reset();
+  if (IsBrowserSideNavigationEnabled()) {
+    // Run message loop once since NavigationRequest::OnStartChecksComplete
+    // posted a task.
+    base::RunLoop().RunUntilIdle();
+  }
 }
 
 void NavigationSimulator::OnThrottleChecksComplete(