Avoid leaking NavigationRequest when committing an error page.

Sometimes the commit IPC really is for |navigation_request_| even when
the URL checks at the top of RFHI::TakeNavigationHandleForCommit
indicate a mismatched URL.  Examples of when this can happen are XFO
checks (see https://crbug.com/870815) and CSP/frame-ancestors checks
(see https://crbug.com/759184).

Before this CL, in the cases above |navigation_request_| would be
leaked.  This CL fixes the leak.

Bug: 872803
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.perf:obbs_fyi
Change-Id: Ia09f355e69f182479386b3deeea1f7f0c8996813
Reviewed-on: https://chromium-review.googlesource.com/1169865
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582289}

content/browser/frame_host/render_frame_host_impl.cc

@@ -5165,6 +5165,9 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
       navigation_request_ ? navigation_request_->navigation_handle() : nullptr;
 
   // Determine if the current NavigationHandle can be used.
+  //
+  // TODO(lukasza, clamy): https://crbug.com/784904: Match commit IPC to proper
+  // NavigationHandle without requiring URLs to match.
   if (navigation_handle && navigation_handle->GetURL() == params.url) {
     std::unique_ptr<NavigationHandleImpl> result_navigation_handle =
         navigation_request()->TakeNavigationHandle();
@@ -5173,15 +5176,29 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
     return result_navigation_handle;
   }
 
-  // If the URL does not match what the NavigationHandle expects, treat the
-  // commit as a new navigation. This can happen when loading a Data
-  // navigation with LoadDataWithBaseURL.
+  // At this point we know that the right/matching |navigation_request_| has
+  // already been found based on navigation id look-up performed by
+  // RFHI::OnCrossDocumentCommitProcessed.  OTOH, we cannot use
+  // |navigation_handle|, because it has a mismatched URL (which would cause
+  // DCHECKs - for example in NavigationHandleImpl::DidCommitNavigation).
+  //
+  // Because of the above, if the URL does not match what the NavigationHandle
+  // expects, we want to treat the commit as a new navigation.
+  // This mostly works, but there are some remaining issues here tracked
+  // by https://crbug.com/872803.
   //
+  // The URL mismatch can happen when loading a Data navigation with
+  // LoadDataWithBaseURL.
   // TODO(csharrison): Data navigations loaded with LoadDataWithBaseURL get
   // reset here, because the NavigationHandle tracks the URL but the params.url
   // tracks the data. The trick of saving the old entry ids for these
   // navigations should go away when this is properly handled.
-  // See crbug.com/588317.
+  // See https://crbug.com/588317.
+  //
+  // Other cases are where URL mismatch can happen is when committing an error
+  // page - for example this can happen during CSP/frame-ancestors checks (see
+  // https://crbug.com/759184).
+
   int entry_id_for_data_nav = 0;
   bool is_renderer_initiated = true;
 
@@ -5205,6 +5222,11 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
       entry_id_for_data_nav = navigation_handle->pending_nav_entry_id();
       is_renderer_initiated = pending_entry->is_renderer_initiated();
     }
+
+    // Going forward we'll use the NavigationHandle created below.  Therefore we
+    // should destroy the old |navigation_request_| and the NavigationHandle it
+    // owns.  This avoids the leak reported in https://crbug.com/872803.
+    navigation_request_.reset();
   }
 
   // There is no pending NavigationEntry in these cases, so pass 0 as the

content/browser/frame_host/render_frame_host_impl_browsertest.cc

@@ -28,6 +28,7 @@
 #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/navigation_handle_observer.h"
 #include "content/public/test/test_frame_navigation_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
@@ -1831,4 +1832,44 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
   main_frame->GetCanonicalUrlForSharing(base::DoNothing());
 }
 
+// This test makes sure that when a blocked frame commits with a different URL,
+// it doesn't lead to a leaked NavigationHandle.  This is a regression test for
+// https://crbug.com/872803.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+                       ErrorPagesShouldntLeakNavigationHandles) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "foo.com", "/frame_tree/page_with_one_frame.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  GURL blocked_url(embedded_test_server()->GetURL(
+      "blocked.com", "/frame-ancestors-none.html"));
+  WebContents* web_contents = shell()->web_contents();
+  NavigationHandleObserver nav_handle_observer(web_contents, blocked_url);
+  EXPECT_TRUE(NavigateIframeToURL(web_contents, "child0", blocked_url));
+
+  // Verify that the NavigationHandle / NavigationRequest didn't leak.
+  RenderFrameHostImpl* frame = static_cast<RenderFrameHostImpl*>(
+      shell()->web_contents()->GetAllFrames()[1]);
+  EXPECT_EQ(0u, frame->GetNavigationEntryIdsPendingCommit().size());
+
+  // TODO(lukasza, clamy): https://crbug.com/784904: Verify that
+  // WebContentsObserver::DidFinishNavigation was called with the same
+  // NavigationHandle as WebContentsObserver::DidStartNavigation.  This requires
+  // properly matching the commit IPC to the NavigationHandle (ignoring that
+  // their URLs do not match - matching instead using navigation id or mojo
+  // interface identity).
+  //
+  // Subsequent checks don't make sense before WCO::DidFinishNavigation is
+  // called - this is why ASSERT_TRUE is used here.
+  //   ASSERT_TRUE(nav_handle_observer.has_committed());
+  //   EXPECT_EQ(net::ERR_BLOCKED_BY_RESPONSE,
+  //       nav_handle_observer.net_error_code());
+
+  // TODO(lukasza): https://crbug.com/759184: Verify
+  // |nav_handle_observer.last_committed_url()| below - this should be possible
+  // once we handle frame-ancestors CSP in the browser process and commit it
+  // with the original URL.
+  //   EXPECT_EQ(blocked_url, nav_handle_observer.last_committed_url());
+}
+
 }  // namespace content