Kill vestigial transfer requests

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 61c5ce5b79169c3b236dee4c61d5a08ad82f1000..5372543c7c17b31eba6c8f88a2729074c03dd11b 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -14,11 +14,13 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/path_service.h"
+#include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/pattern.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "content/browser/frame_host/cross_process_frame_connector.h"
@@ -54,6 +56,7 @@
 #include "content/public/test/test_frame_navigation_observer.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 "ipc/ipc.mojom.h"
 #include "ipc/ipc_security_test_util.h"
@@ -8562,4 +8565,55 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 }
 
+// Checks that everything is cleaned up even when the frame tree is destroyed
+// during a transfer.  See also https://crbug.com/657195.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       FrameTreeDestroyedInMiddleOfTransfer) {

[nasko, 2016/11/19 00:16:21]

Since this test is specifically targeting transfers, might be good to return
early when running in PlzNavigate mode, as there are no transfers in that mode.

[Łukasz Anforowicz, 2016/11/19 00:24:59]

That might be a good idea.  csharrison@ - WDYT?  Would you be able to make that
change?

[Charlie Harrison, 2016/11/19 03:32:38]

Done.

+  GURL page_url(embedded_test_server()->GetURL(
+      "main.com", "/frame_tree/page_with_one_frame.html"));
+  GURL initial_frame_url(embedded_test_server()->GetURL(
+      "main.com", "/cross-site/baz.com/title1.html"));
+
+  {
+    // Navigation below is needed to make OpenPopup (next statement) work.
+    EXPECT_TRUE(
+        NavigateToURL(shell(), embedded_test_server()->GetURL("/title2.html")));
+
+    // Create a separate Shell + WebContents - these will be destroyed during
+    // the test at a very special moment.
+    Shell* other_shell = OpenPopup(shell()->web_contents(), GURL(), "popup");
+
+    // Load the test page, while monitoring navigations of the frame (to catch
+    // when the frame navigation will initiate a transfer to another renderer).
+    TestNavigationManager navigation_manager(other_shell->web_contents(),
+                                             initial_frame_url);
+    other_shell->LoadURL(page_url);
+
+    // Wait until |navigation_manager| detects a WillProcessResponse associated
+    // with the frame navigation.
+    ASSERT_TRUE(navigation_manager.WaitForResponse());
+
+    // At this point we have almost (but not quite) trigerred a transfer request
+    // - the transfer will be initiated when resuming the navigation.  Posting a
+    // task to destroy the frame being navigated means that the destruction
+    // won't happen now, but will happen right after initiating the transfer AND
+    // hopefully before the transfer completes (i.e. before we receive

[Charlie Reis, 2016/11/19 00:03:14]

"Hopefully" sounds flaky to me.  :)  If so, is it just that it will always
succeed with the fix in place and usually (but not always) fail without the fix?
 Or is it not actually flaky?

[Łukasz Anforowicz, 2016/11/19 00:16:38]

Maybe "hopefully" is the wrong way here.  The way things work today, we are
guaranteed the right ordering (the ordering that used to trigger the bug):

1. Before we resume the navigation (on line 8610), we have not yet initiated the
transfer / we have not yet sent an IPC to the new renderer.

2. Because of 1, the task we post on line 8602 is guaranteed to be queued by the
message loop *before* the IPC response for DidStartProvisionalLoad.

[Łukasz Anforowicz, 2016/11/19 00:24:59]

I meant s/wrong way/wrong word/

[Charlie Harrison, 2016/11/19 03:32:38]

Updated the comment to be a bit more specific.

+    // DidStartProvisionalLoad from the new renderer).
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&WebContents::Close,
+                              base::Unretained(other_shell->web_contents())));
+
+    // Resume the navigation.  This will 1) initiate the transfer and 2) shortly
+    // after destroy the |other_shell| via WebContents::Close task posted above.
+    // Destroying the NavigationHandle at this special moment used to trigger
+    // https://crbug.com/657195.
+    navigation_manager.WaitForNavigationFinished();
+  }
+
+  // Start a URLRequest to the same URL.  This should succeed.  This would have
+  // hit the 20 seconds delay before https://crbug.com/657195 was fixed.

[nasko, 2016/11/19 00:16:21]

Why not check actually the time it takes? What if the machine is slow and the
test timeout is actually set to more than 20 seconds?

[Łukasz Anforowicz, 2016/11/19 00:24:59]

Hmmmm... good point.  OTOH, I am a bit wary of making the test depend on timing.
 FWIW, in that case the problem would still be caught by a DCHECK (see
https://crbug.com/657195#c31).

[Charlie Harrison, 2016/11/19 03:32:39]

I agree, I don't want to add timing to this suite. I confirmed that we
AssertNoURLRequests() if this test fails but does not time out, so I will just
add that as a comment.

+  EXPECT_TRUE(NavigateToURL(shell(), page_url));
+  EXPECT_EQ(page_url, shell()->web_contents()->GetLastCommittedURL());
+}
+
 }  // namespace content