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

content/browser/loader/cross_site_resource_handler.cc

Index: content/browser/loader/cross_site_resource_handler.cc
===================================================================
--- content/browser/loader/cross_site_resource_handler.cc	(revision 250813)
+++ content/browser/loader/cross_site_resource_handler.cc	(working copy)
@@ -13,6 +13,7 @@
 #include "content/browser/cross_site_request_manager.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/renderer_host/cross_site_request_transfer.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/public/browser/browser_thread.h"
@@ -30,17 +31,21 @@
 
 namespace {
 
+bool leak_requests_for_testing_ = false;

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

This seems unfortunate, but I don't have a better idea off the top of my head.

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

I agree.  I couldn't think of something better to do, either.  We could avoid
making it a global, but it's a bit of a pain to pass it around, and it's still
ugly.

+
 // The parameters to OnCrossSiteResponseHelper exceed the number of arguments
 // base::Bind supports.
 struct CrossSiteResponseParams {
-  CrossSiteResponseParams(int render_view_id,
-                          const GlobalRequestID& global_request_id,
-                          bool is_transfer,
-                          const std::vector<GURL>& transfer_url_chain,
-                          const Referrer& referrer,
-                          PageTransition page_transition,
-                          int64 frame_id,
-                          bool should_replace_current_entry)
+  CrossSiteResponseParams(
+      int render_view_id,
+      const GlobalRequestID& global_request_id,
+      bool is_transfer,
+      const std::vector<GURL>& transfer_url_chain,
+      const Referrer& referrer,
+      PageTransition page_transition,
+      int64 frame_id,
+      bool should_replace_current_entry,
+      CrossSiteResourceHandler* cross_site_resource_handler)
       : render_view_id(render_view_id),
         global_request_id(global_request_id),
         is_transfer(is_transfer),
@@ -48,7 +53,8 @@
         referrer(referrer),
         page_transition(page_transition),
         frame_id(frame_id),
-        should_replace_current_entry(should_replace_current_entry) {
+        should_replace_current_entry(should_replace_current_entry),
+        cross_site_resource_handler(cross_site_resource_handler) {
   }
 
   int render_view_id;
@@ -59,18 +65,29 @@
   PageTransition page_transition;
   int64 frame_id;
   bool should_replace_current_entry;
+  CrossSiteResourceHandler* cross_site_resource_handler;
 };
 
 void OnCrossSiteResponseHelper(const CrossSiteResponseParams& params) {
+  scoped_ptr<CrossSiteRequestTransfer> cross_site_request_transfer;
+  if (params.is_transfer) {
+    cross_site_request_transfer.reset(new CrossSiteRequestTransfer(
+        params.cross_site_resource_handler));
+  }
+
   RenderViewHostImpl* rvh =
       RenderViewHostImpl::FromID(params.global_request_id.child_id,
                                  params.render_view_id);
   if (rvh) {
     rvh->OnCrossSiteResponse(
-        params.global_request_id, params.is_transfer,
+        params.global_request_id, cross_site_request_transfer.Pass(),
         params.transfer_url_chain, params.referrer,
         params.page_transition, params.frame_id,
         params.should_replace_current_entry);
+  } else if (leak_requests_for_testing_ && cross_site_request_transfer) {
+    // Some unit tests expect requests to be leaked in this case, so they can
+    // pass them along manually.
+    cross_site_request_transfer->ReleaseRequest();
   }
 }
 
@@ -83,8 +100,7 @@
       has_started_response_(false),
       in_cross_site_transition_(false),
       completed_during_transition_(false),
-      did_defer_(false),
-      completed_status_() {
+      did_defer_(false) {
 }
 
 CrossSiteResourceHandler::~CrossSiteResourceHandler() {
@@ -259,6 +275,27 @@
   }
 }
 
+void CrossSiteResourceHandler::AbortTransfer() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // Cancelling the request is not strictly necessary, since nothing is

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

Is it not done by the CancelRequest call below?  (Or is it skipped there because
it's considered a transfer?)

I'm wondering if there's an alternative that avoids passing the CSRH to the UI
thread.  For example, add a ResourceLoader::CancelTransfer, which then lets us
call RDH::CancelRequest with the child ID and request ID to clean it up.  Would
be nice to stick to passing IDs between threads rather than references to
objects.

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

No, it's not.  Despite its name, ResourceDispatcherHostImpl::CancelRequest
deletes a request without cancelling it.  In the test, the throttle, on
deletion, checks the status of the request.  If we just delete the request,
while not waiting for a callback (Like when it's paused after reading the
headers), we get a status indicating success, which makes for bizarre and
regression prone test.

It looks like I can check "is_pending" instead, so I've removed the real cancel.
I initially just kept the IDs, and used RDH::RemovePendingRequest.

I was thinking that this design would make it easier to handle cleanup in the
deletion case - just have the RenderFrameHost keep ownership of the cross site
request transfer, until some point at which is must have been claimed or is no
longer needed.  Then hop over to the UI thread, and if it exists, see if it was
reclaimed, etc....

That could also be done through the RDH, now that I think about it, if we wanted
to.  I've switched back to going through the RDH.  I've added a new function to
the RDH to specifically cancel a pending transfer, just so we can make sure it's
a transfer (And it exists).  I think that the extra check is important, as a
check there's no case where we accidentally delete a request we shouldn't.

I'm open to other proposals.

[Charlie Reis, 2014/02/15 01:25:44]

Ah.
Ah, I like that better.  It looks like you've left AbortTransfer in the code
unused, though.  Can you remove it?

+  // listening to it, but it makes for clearer logs and cleaner tests.
+  request()->Cancel();
+
+  // Clean up the request.
+  ResourceRequestInfoImpl* info = GetRequestInfo();
+  ResourceDispatcherHostImpl::Get()->CancelRequest(info->GetChildID(),
+                                                   info->GetRequestID());
+
+  // |this| should now be deleted.
+}
+
+// static
+void CrossSiteResourceHandler::SetLeakRequestsForTesting(
+    bool leak_requests_for_testing) {
+  leak_requests_for_testing_ = leak_requests_for_testing;
+}
+
 // Prepare to render the cross-site response in a new RenderViewHost, by
 // telling the old RenderViewHost to run its onunload handler.
 void CrossSiteResourceHandler::StartCrossSiteTransition(
@@ -306,7 +343,8 @@
                                   referrer,
                                   info->GetPageTransition(),
                                   frame_id,
-                                  info->should_replace_current_entry())));
+                                  info->should_replace_current_entry(),
+                                  this)));
 }
 
 void CrossSiteResourceHandler::ResumeIfDeferred() {