When cross-site navigations are cancelled, delete the request being transferred

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
===================================================================
--- content/browser/site_per_process_browsertest.cc	(revision 250813)
+++ content/browser/site_per_process_browsertest.cc	(working copy)
@@ -6,12 +6,15 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/browser/resource_throttle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
@@ -20,10 +23,13 @@
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_content_browser_client.h"
+#include "content/shell/browser/shell_resource_dispatcher_host_delegate.h"
 #include "content/test/content_browser_test.h"
 #include "content/test/content_browser_test_utils.h"
 #include "net/base/escape.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_status.h"
 
 namespace content {
 
@@ -159,7 +165,168 @@
   running_ = false;
 }
 
+// Tracks a single request for a specified URL, and allows waiting until the
+// request is destroyed, and then inspecting its final status.
+class TrackingResourceDispatcherHostDelegate

[Charlie Reis, 2014/02/13 22:12:19]

Thanks for figuring out how to test this.

I'm getting the feeling that we're vastly overloading this test class, since it
was designed to test things specific to --site-per-process, while these transfer
tests matter today (as demonstrated by these bugs).  We can split them out to a
new test class in a separate CL if that's too much work for now, but I don't
think they should stay in SitePerProcessBrowserTest.

[mmenke, 2014/02/14 16:30:02]

This is pretty similar to how the network-related browser tests work, except we
test by injecting URLRequestJobs instead.  It's a bit ugly, but it works.
I think it makes sense to separate them, but I don't think it makes sense to do
it in this CL.  I'm happy to do a followup CL after this lands.

+    : public ShellResourceDispatcherHostDelegate {
+ public:
+  TrackingResourceDispatcherHostDelegate() : throttle_created_(false) {
+  }
+
+  virtual void RequestBeginning(
+      net::URLRequest* request,
+      ResourceContext* resource_context,
+      appcache::AppCacheService* appcache_service,
+      ResourceType::Type resource_type,
+      int child_id,
+      int route_id,
+      ScopedVector<ResourceThrottle>* throttles) OVERRIDE {
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    ShellResourceDispatcherHostDelegate::RequestBeginning(
+        request, resource_context, appcache_service, resource_type, child_id,
+        route_id, throttles);
+    // Expect only a single request for the tracked url.
+    ASSERT_FALSE(throttle_created_);
+    // If this is a request for the tracked URL, add a throttle to track it.
+    if (request->url() == tracked_url_)
+      throttles->push_back(new TrackingThrottle(request, this));
+  }
+
+  // Starts tracking a URL.  The request for previously tracked URL, if any,
+  // must have been made and deleted before calling this function.
+  void SetTrackedURL(const GURL& tracked_url) {
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    // Should not currently be tracking any URL.
+    ASSERT_FALSE(run_loop_);
+
+    // Create a RunLoop that will be stopped once the request for the tracked
+    // URL has been destroyed, to allow tracking the URL while also waiting for
+    // other events.
+    run_loop_.reset(new base::RunLoop());
+
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &TrackingResourceDispatcherHostDelegate::SetTrackedURLOnIOThread,
+            base::Unretained(this),
+            tracked_url));
+  }
+
+  // Waits until the tracked URL has been requests, and the request for it has
+  // been destroyed.
+  net::URLRequestStatus WaitForTrackedURL() {
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    run_loop_->Run();
+    run_loop_.reset();
+    return tracked_request_status_;
+  }
+
+ private:
+  // ResourceThrottle attached to request for the tracked URL.  On destruction,
+  // passes the final URLRequestStatus back to the delegate.
+  class TrackingThrottle : public ResourceThrottle {
+   public:
+    TrackingThrottle(net::URLRequest* request,
+                     TrackingResourceDispatcherHostDelegate* tracker)
+        : request_(request), tracker_(tracker) {
+    }
+
+    virtual ~TrackingThrottle() {
+      tracker_->TrackedRequestCompleted(request_->status());
+    }
+
+    // ResourceThrottle implementation:
+    virtual const char* GetNameForLogging() const OVERRIDE {
+      return "TrackingThrottle";
+    }
+
+   private:
+    net::URLRequest* request_;
+    TrackingResourceDispatcherHostDelegate* tracker_;
+
+    DISALLOW_COPY_AND_ASSIGN(TrackingThrottle);
+  };
+
+  void SetTrackedURLOnIOThread(const GURL& tracked_url) {
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    throttle_created_ = false;
+    tracked_url_ = tracked_url;
+  }
+
+  void TrackedRequestCompleted(net::URLRequestStatus status) {
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    tracked_request_status_ = status;
+    tracked_url_ = GURL();
+
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE, run_loop_->QuitClosure());
+  }
+
+  // These live on the IO thread.
+  GURL tracked_url_;
+  bool throttle_created_;
+
+  // This is created and destroyed on the UI thread, but stopped on the IO
+  // thread.
+  scoped_ptr<base::RunLoop> run_loop_;
+
+  // Set on the IO thread while |run_loop_| is non-NULL, read on the UI thread
+  // after deleting run_loop_.
+  net::URLRequestStatus tracked_request_status_;
+
+  DISALLOW_COPY_AND_ASSIGN(TrackingResourceDispatcherHostDelegate);
+};
+
+// WebContentsDelegate that fails to open a URL when there's a request that
+// needs to be transferred between renderers.
+class NoTransferRequestDelegate : public WebContentsDelegate {
+ public:
+  NoTransferRequestDelegate() {}
+
+  virtual WebContents* OpenURLFromTab(WebContents* source,
+                                      const OpenURLParams& params) OVERRIDE {
+    bool is_transfer =
+        (params.transferred_global_request_id != GlobalRequestID());
+    if (is_transfer)
+      return NULL;
+    NavigationController::LoadURLParams load_url_params(params.url);
+    load_url_params.referrer = params.referrer;
+    load_url_params.frame_tree_node_id = params.frame_tree_node_id;
+    load_url_params.transition_type = params.transition;
+    load_url_params.extra_headers = params.extra_headers;
+    load_url_params.should_replace_current_entry =
+        params.should_replace_current_entry;
+    load_url_params.is_renderer_initiated = true;
+    source->GetController().LoadURLWithParams(load_url_params);
+    return source;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NoTransferRequestDelegate);
+};
+
 class SitePerProcessBrowserTest : public ContentBrowserTest {
+ public:
+  SitePerProcessBrowserTest() : old_delegate_(NULL) {
+  }
+
+  // ContentBrowserTest implementation:
+  virtual void SetUpOnMainThread() OVERRIDE {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &SitePerProcessBrowserTest::InjectResourceDisptcherHostDelegate,
+            base::Unretained(this)));
+  }
+
+  virtual void TearDownOnMainThread() OVERRIDE {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &SitePerProcessBrowserTest::RestoreResourceDisptcherHostDelegate,
+            base::Unretained(this)));
+  }
+
  protected:
   // Start at a data URL so each extra navigation creates a navigation entry.
   // (The first navigation will silently be classified as AUTO_SUBFRAME.)
@@ -195,7 +362,8 @@
 
   void NavigateToURLContentInitiated(Shell* window,
                                      const GURL& url,
-                                     bool should_replace_current_entry) {
+                                     bool should_replace_current_entry,
+                                     bool should_wait_for_navigation) {
     std::string script;
     if (should_replace_current_entry)
       script = base::StringPrintf("location.replace('%s')", url.spec().c_str());
@@ -204,7 +372,8 @@
     TestNavigationObserver load_observer(shell()->web_contents(), 1);
     bool result = ExecuteScript(window->web_contents(), script);
     EXPECT_TRUE(result);
-    load_observer.Wait();
+    if (should_wait_for_navigation)
+      load_observer.Wait();
   }
 
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
@@ -213,6 +382,26 @@
     // TODO(creis): Remove this when GTK is no longer a supported platform.
     command_line->AppendSwitch(switches::kForceCompositingMode);
   }
+
+  void InjectResourceDisptcherHostDelegate() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    old_delegate_ = ResourceDispatcherHostImpl::Get()->delegate();
+    ResourceDispatcherHostImpl::Get()->SetDelegate(&tracking_delegate_);
+  }
+
+  void RestoreResourceDisptcherHostDelegate() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    ResourceDispatcherHostImpl::Get()->SetDelegate(old_delegate_);
+    old_delegate_ = NULL;
+  }
+
+  TrackingResourceDispatcherHostDelegate& tracking_delegate() {
+    return tracking_delegate_;
+  }
+
+ private:
+  TrackingResourceDispatcherHostDelegate tracking_delegate_;
+  ResourceDispatcherHostDelegate* old_delegate_;
 };
 
 // Ensure that we can complete a cross-process subframe navigation.
@@ -498,13 +687,19 @@
   GURL url2 = test_server()->GetURL("files/site_isolation/blank.html?2");
   replace_host.SetHostStr(a_com);
   url2 = url2.ReplaceComponents(replace_host);
-  NavigateToURLContentInitiated(shell(), url2, true);
+  // Used to make sure the request for url2 succeeds, and there was only one of
+  // them.
+  tracking_delegate().SetTrackedURL(url2);
+  NavigateToURLContentInitiated(shell(), url2, true, true);
 
   // There should be one history entry. url2 should have replaced url1.
   EXPECT_TRUE(controller.GetPendingEntry() == NULL);
   EXPECT_EQ(1, controller.GetEntryCount());
   EXPECT_EQ(0, controller.GetCurrentEntryIndex());
   EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
+  // Make sure the request succeeded.
+  net::URLRequestStatus status = tracking_delegate().WaitForTrackedURL();
+  EXPECT_TRUE(status.is_success());
 
   // Now navigate as before to a page on B.com, but normally (without
   // replacement). This will still perform a double process-swap as above, via
@@ -512,7 +707,10 @@
   GURL url3 = test_server()->GetURL("files/site_isolation/blank.html?3");
   replace_host.SetHostStr(b_com);
   url3 = url3.ReplaceComponents(replace_host);
-  NavigateToURLContentInitiated(shell(), url3, false);
+  // Used to make sure the request for url3 succeeds, and there was only one of
+  // them.
+  tracking_delegate().SetTrackedURL(url3);
+  NavigateToURLContentInitiated(shell(), url3, false, true);
 
   // There should be two history entries. url2 should have replaced url1. url2
   // should not have replaced url3.
@@ -521,6 +719,10 @@
   EXPECT_EQ(1, controller.GetCurrentEntryIndex());
   EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL());
   EXPECT_EQ(url3, controller.GetEntryAtIndex(1)->GetURL());
+
+  // Make sure the request succeeded.
+  status = tracking_delegate().WaitForTrackedURL();
+  EXPECT_TRUE(status.is_success());
 }
 
 // Tests that the |should_replace_current_entry| flag persists correctly across
@@ -546,7 +748,7 @@
   // Navigate in-process with entry replacement. It will then be transferred
   // into a new one due to the call above.
   GURL url2 = test_server()->GetURL("files/site_isolation/blank.html?2");
-  NavigateToURLContentInitiated(shell(), url2, true);
+  NavigateToURLContentInitiated(shell(), url2, true, true);
 
   // There should be one history entry. url2 should have replaced url1.
   EXPECT_TRUE(controller.GetPendingEntry() == NULL);
@@ -556,7 +758,7 @@
 
   // Now navigate as before, but without replacement.
   GURL url3 = test_server()->GetURL("files/site_isolation/blank.html?3");
-  NavigateToURLContentInitiated(shell(), url3, false);
+  NavigateToURLContentInitiated(shell(), url3, false, true);
 
   // There should be two history entries. url2 should have replaced url1. url2
   // should not have replaced url3.
@@ -597,7 +799,7 @@
       "server-redirect?" + net::EscapeQueryParamValue(url2b.spec(), false));
   replace_host.SetHostStr(a_com);
   url2a = url2a.ReplaceComponents(replace_host);
-  NavigateToURLContentInitiated(shell(), url2a, true);
+  NavigateToURLContentInitiated(shell(), url2a, true, true);
 
   // There should be one history entry. url2b should have replaced url1.
   EXPECT_TRUE(controller.GetPendingEntry() == NULL);
@@ -613,7 +815,7 @@
       "server-redirect?" + net::EscapeQueryParamValue(url3b.spec(), false));
   replace_host.SetHostStr(a_com);
   url3a = url3a.ReplaceComponents(replace_host);
-  NavigateToURLContentInitiated(shell(), url3a, false);
+  NavigateToURLContentInitiated(shell(), url3a, false, true);
 
   // There should be two history entries. url2b should have replaced url1. url2b
   // should not have replaced url3b.
@@ -624,4 +826,56 @@
   EXPECT_EQ(url3b, controller.GetEntryAtIndex(1)->GetURL());
 }
 
+// Tests that the request is destroyed when a cross process navigation is
+// cancelled.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NoLeakOnCrossSiteCancel) {
+  const NavigationController& controller =
+      shell()->web_contents()->GetController();
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+
+  // These must all stay in scope with replace_host.
+  GURL::Replacements replace_host;
+  std::string a_com("A.com");
+  std::string b_com("B.com");
+
+  // Navigate to a starting URL, so there is a history entry to replace.
+  GURL url1 = test_server()->GetURL("files/site_isolation/blank.html?1");
+  NavigateToURL(shell(), url1);
+
+  // Force all future navigations to transfer.
+  ShellContentBrowserClient::SetSwapProcessesForRedirect(true);
+
+  NoTransferRequestDelegate no_transfer_request_delegate;
+  WebContentsDelegate* old_delegate = shell()->web_contents()->GetDelegate();
+  shell()->web_contents()->SetDelegate(&no_transfer_request_delegate);
+
+  // Navigate to a page on A.com with entry replacement. This navigation is
+  // cross-site, so the renderer will send it to the browser via OpenURL to give
+  // to a new process. It will then be transferred into yet another process due
+  // to the call above.
+  GURL url2 = test_server()->GetURL("files/site_isolation/blank.html?2");
+  replace_host.SetHostStr(a_com);
+  url2 = url2.ReplaceComponents(replace_host);
+  // Used to make sure the second request is cancelled, and there is only one
+  // request for url2.
+  tracking_delegate().SetTrackedURL(url2);
+
+  // Don't want for the navigation to complete, since that never happens in

[Charlie Reis, 2014/02/13 22:12:19]

nit: wait

[mmenke, 2014/02/14 16:30:02]

Done.

+  // this case.
+  NavigateToURLContentInitiated(shell(), url2, false, false);
+
+  // There should be one history entry, with url1.
+  EXPECT_EQ(1, controller.GetEntryCount());
+  EXPECT_EQ(0, controller.GetCurrentEntryIndex());
+  EXPECT_EQ(url1, controller.GetEntryAtIndex(0)->GetURL());
+
+  // Make sure the request for url2 was cancelled.
+  net::URLRequestStatus status = tracking_delegate().WaitForTrackedURL();
+  EXPECT_FALSE(status.is_success());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, status.status());
+
+  shell()->web_contents()->SetDelegate(old_delegate);
+}
+
 }  // namespace content