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

content/browser/renderer_host/cross_site_request_transfer.h

Index: content/browser/renderer_host/cross_site_request_transfer.h
===================================================================
--- content/browser/renderer_host/cross_site_request_transfer.h	(revision 0)
+++ content/browser/renderer_host/cross_site_request_transfer.h	(revision 0)
@@ -0,0 +1,40 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
+#define CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_

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

I think this would belong in frame_host rather than renderer_host if we decide
to keep it.  All navigation stuff is ending up there.

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

I tried to have it in frame_host.  Unfortunately, renderer_host can't depend on
frame_host files, and we currently pass this through renderer_host files.

We could just have this object exist in frame_host, and pass it around
unprotected in renderer_host, but that makes me very nervous about regressions.

[mmenke, 2014/02/14 17:10:20]

"Pass it around" == Pass the request ID around without having anything own it.

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

Oh, RVH needs it for now.  Ok, you can leave it here and we'll move it over when
we move the rest of the navigation stuff to RenderFrameHost.  (Nasko has CLs in
progress for this.)

+
+#include "base/basictypes.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+class CrossSiteResourceHandler;
+
+// A UI thread object that owns a request being transferred.  Deleting the
+// object without releasing the request will delete the underlying URLRequest.
+// This is needed to clean up the URLRequest when a cross site navigation is
+// cancelled.
+class CONTENT_EXPORT CrossSiteRequestTransfer {

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

CrossSiteRequest or CrossSiteTransferringRequest, perhaps?  The Request is the
main noun, not the Transfer.

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

I considered "CrossSiteRequest", but I didn't like it because the actual request
object is on the IO thread, and this lives for the duration of the transfer,
rather than the duration of the request.

I've switched to CrossSiteTransferringRequest.

+ public:
+  explicit CrossSiteRequestTransfer(
+      CrossSiteResourceHandler* cross_site_resource_handler);
+  ~CrossSiteRequestTransfer();
+
+  // Relinquishes ownership of the request, so another process can take
+  // control of it.
+  void ReleaseRequest();
+
+ private:
+  // No need for a weak pointer here - nothing should have ownership of the
+  // cross site request until after |this| is deleted, or ReleaseRequest is
+  // called.
+  CrossSiteResourceHandler* cross_site_resource_handler_;

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

I'm not yet convinced this is safe, because CrossSiteResourceHandler is only
supposed to be accessed from the IO thread.  Couldn't it be deleted on the IO
thread when canceled in some way?

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

If it can be, then we don't actually own it, and there may be a bug.  I actually
want the crashing behavior, as a way to ensure there aren't any bugs.  In this
case, crashing isn't a bug of the implementation, it's a feature!

We don't delete transferring requests on process shutdown (We have a DCHECK to
make sure of that), and the only case we do delete them, with the exception of
them being claimed, is when the RDH shuts down, which only happens after we've
closed all tabs, which would have already cancelled any request owned by one of
these.

+
+  DISALLOW_COPY_AND_ASSIGN(CrossSiteRequestTransfer);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
Property changes on: content\browser\renderer_host\cross_site_request_transfer.h
___________________________________________________________________
Added: svn:eol-style
   + LF