Make SyncLoad result handling pluggable

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index f2c1ac98e8cd6387941ada630c8b712ed8d68600..ad158efe52c074bcfac76251c2cfb1a88cbc808f 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -118,6 +118,8 @@ using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 using storage::ShareableFileReference;
+using SyncLoadResultCallback =
+    content::ResourceDispatcherHostImpl::SyncLoadResultCallback;
 
 // ----------------------------------------------------------------------------
 
@@ -220,15 +222,15 @@ bool IsDetachableResourceType(ResourceType type) {
 }
 
 // Aborts a request before an URLRequest has actually been created.
-void AbortRequestBeforeItStarts(ResourceMessageFilter* filter,
-                                IPC::Message* sync_result,
-                                int request_id,
-                                mojom::URLLoaderClientPtr url_loader_client) {
-  if (sync_result) {
+void AbortRequestBeforeItStarts(
+    ResourceMessageFilter* filter,
+    const SyncLoadResultCallback& sync_result_handler,
+    int request_id,
+    mojom::URLLoaderClientPtr url_loader_client) {
+  if (sync_result_handler) {
     SyncLoadResult result;
     result.error_code = net::ERR_ABORTED;
-    ResourceHostMsg_SyncLoad::WriteReplyParams(sync_result, result);
-    filter->Send(sync_result);
+    sync_result_handler.Run(&result);
   } else {
     // Tell the renderer that this request was disallowed.
     ResourceRequestCompletionStatus request_complete_data;
@@ -413,6 +415,19 @@ void NotifyForEachFrameFromUI(
                                      base::Passed(std::move(routing_ids))));
 }
 
+void HandleSyncLoadResult(base::WeakPtr<ResourceMessageFilter> filter,

[mmenke, 2016/10/05 15:21:33]

Need to document this.

[tzik, 2016/10/06 12:52:03]

Done.

+                          std::unique_ptr<IPC::Message> sync_result,
+                          const SyncLoadResult* result) {
+  if (!filter)
+    return;
+
+  if (result)
+    ResourceHostMsg_SyncLoad::WriteReplyParams(sync_result.get(), *result);
+  else
+    sync_result->set_reply_error();

[mmenke, 2016/10/05 15:21:33]

nit:  Use braces with if else

[tzik, 2016/10/06 12:52:03]

Done.

+  filter->Send(sync_result.release());
+}
+
 }  // namespace
 
 ResourceDispatcherHostImpl::LoadInfo::LoadInfo() {}
@@ -1058,7 +1073,7 @@ void ResourceDispatcherHostImpl::OnRequestResourceInternal(
                    request_data.render_frame_id,
                    request_data.url));
   }
-  BeginRequest(request_id, request_data, NULL, routing_id,
+  BeginRequest(request_id, request_data, SyncLoadResultCallback(), routing_id,
                std::move(mojo_request), std::move(url_loader_client));
 }
 
@@ -1073,7 +1088,10 @@ void ResourceDispatcherHostImpl::OnRequestResourceInternal(
 void ResourceDispatcherHostImpl::OnSyncLoad(int request_id,
                                             const ResourceRequest& request_data,
                                             IPC::Message* sync_result) {
-  BeginRequest(request_id, request_data, sync_result, sync_result->routing_id(),
+  SyncLoadResultCallback cb = base::Bind(

[mmenke, 2016/10/05 15:21:33]

I'd suggest just writing out callback.  (I consider cb a not too common
abbreviation, at least in loader/)

[tzik, 2016/10/06 12:52:03]

Done.

+      &HandleSyncLoadResult, filter_->GetWeakPtr(),
+      base::Passed(WrapUnique(sync_result)));

[mmenke, 2016/10/05 15:21:33]

This doesn't seem legal.  We don't own sync_result (Or at least the old code
doesn't delete it, even when it doesn't send the message), so I don't think we
can legally wrap it in a sync pointer, unless the old code was actually leaking
it.

The API docs aren't clear, but I think we just need to keep around the raw
pointer?  Don't ask me what we should do if the filter is deleted.  This seems
underspecified to me.

[kinuko, 2016/10/05 15:52:57]

It looks we could also just copy the message by value (*sync_result) and let the
callback hold it too, which might make the code feel safer (adding callback
indirection makes its lifetime obscure).

[kinuko, 2016/10/05 20:57:49]

^^ sorry please ignore this comment, that was bogus

[tzik, 2016/10/06 12:52:03]

This is called by MessageT<>::DispatchDelayReply via OnMessageReceived.
https://cs.chromium.org/chromium/src/ipc/ipc_message_templates.h?sq=package:c...

|sync_result| is created as |reply| by SyncMessage::GenerateReply in MT::DDR
above, and eventually deleted in Send().
So, I think the old code is leaky and it's safe to store it in std::unique_ptr.
The IPC system implicitly passes the ownership of it.

+  BeginRequest(request_id, request_data, cb, sync_result->routing_id(),
                nullptr, nullptr);
 }
 
@@ -1177,7 +1195,7 @@ void ResourceDispatcherHostImpl::UpdateRequestForTransfer(
 void ResourceDispatcherHostImpl::BeginRequest(
     int request_id,
     const ResourceRequest& request_data,
-    IPC::Message* sync_result,  // only valid for sync
+    const SyncLoadResultCallback& sync_result_handler, // only valid for sync
     int route_id,
     mojo::InterfaceRequest<mojom::URLLoader> mojo_request,
     mojom::URLLoaderClientPtr url_loader_client) {
@@ -1252,7 +1270,7 @@ void ResourceDispatcherHostImpl::BeginRequest(
   if (is_shutdown_ ||
       !ShouldServiceRequest(process_type, child_id, request_data, headers,
                             filter_, resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, request_id,
+    AbortRequestBeforeItStarts(filter_, sync_result_handler, request_id,
                                std::move(url_loader_client));
     return;
   }
@@ -1277,22 +1295,22 @@ void ResourceDispatcherHostImpl::BeginRequest(
             it.name(), it.value(), child_id, resource_context,
             base::Bind(&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
                        base::Unretained(this), request_id, request_data,
-                       sync_result, route_id, headers,
+                       sync_result_handler, route_id, headers,
                        base::Passed(std::move(mojo_request)),
                        base::Passed(std::move(url_loader_client))));
         return;
       }
     }
   }
-  ContinuePendingBeginRequest(request_id, request_data, sync_result, route_id,
-                              headers, std::move(mojo_request),
+  ContinuePendingBeginRequest(request_id, request_data, sync_result_handler,
+                              route_id, headers, std::move(mojo_request),
                               std::move(url_loader_client), true, 0);
 }
 
 void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
     int request_id,
     const ResourceRequest& request_data,
-    IPC::Message* sync_result,  // only valid for sync
+    const SyncLoadResultCallback& sync_result_handler, // only valid for sync
     int route_id,
     const net::HttpRequestHeaders& headers,
     mojo::InterfaceRequest<mojom::URLLoader> mojo_request,
@@ -1303,7 +1321,7 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
     // TODO(ananta): Find a way to specify the right error code here.  Passing
     // in a non-content error code is not safe.
     bad_message::ReceivedBadMessage(filter_, bad_message::RDH_ILLEGAL_ORIGIN);
-    AbortRequestBeforeItStarts(filter_, sync_result, request_id,
+    AbortRequestBeforeItStarts(filter_, sync_result_handler, request_id,
                                std::move(url_loader_client));
     return;
   }
@@ -1326,7 +1344,7 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
                                                   request_data.url,
                                                   request_data.resource_type,
                                                   resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, request_id,
+    AbortRequestBeforeItStarts(filter_, sync_result_handler, request_id,
                                std::move(url_loader_client));
     return;
   }
@@ -1399,7 +1417,7 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
   bool allow_download = request_data.allow_download &&
       IsResourceTypeFrame(request_data.resource_type);
   bool do_not_prompt_for_login = request_data.do_not_prompt_for_login;
-  bool is_sync_load = sync_result != NULL;
+  bool is_sync_load = !sync_result_handler.is_null();

[mmenke, 2016/10/05 15:21:33]

Higher up, you just rely on implicit conversion of SyncLoadResultCallback to a
bool.  Should be consistent.  Either remove the is_null() here, or add it
further up.

[tzik, 2016/10/06 12:52:03]

That is an implicit use of an explicit conversion operator :p).
To use it here, we need explicit static_cast<bool>(result_handler) or
!!result_handler.

Updated it to `!!result_handler` as it looks cleaner to me.

 
   // Raw headers are sensitive, as they include Cookie/Set-Cookie, so only
   // allow requesting them if requester has ReadRawCookies permission.
@@ -1511,8 +1529,8 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
       request_data.should_reset_appcache);
 
   std::unique_ptr<ResourceHandler> handler(CreateResourceHandler(
-      new_request.get(), request_data, sync_result, route_id, process_type,
-      child_id, resource_context, std::move(mojo_request),
+      new_request.get(), request_data, sync_result_handler, route_id,
+      process_type, child_id, resource_context, std::move(mojo_request),
       std::move(url_loader_client)));
 
   if (handler)
@@ -1523,7 +1541,7 @@ std::unique_ptr<ResourceHandler>
 ResourceDispatcherHostImpl::CreateResourceHandler(
     net::URLRequest* request,
     const ResourceRequest& request_data,
-    IPC::Message* sync_result,
+    const SyncLoadResultCallback& sync_result_handler,
     int route_id,
     int process_type,
     int child_id,
@@ -1536,7 +1554,7 @@ ResourceDispatcherHostImpl::CreateResourceHandler(
           "456331 ResourceDispatcherHostImpl::CreateResourceHandler"));
   // Construct the IPC resource handler.
   std::unique_ptr<ResourceHandler> handler;
-  if (sync_result) {
+  if (sync_result_handler) {
     // download_to_file is not supported for synchronous requests.
     if (request_data.download_to_file) {
       bad_message::ReceivedBadMessage(filter_, bad_message::RDH_BAD_DOWNLOAD);
@@ -1545,7 +1563,7 @@ ResourceDispatcherHostImpl::CreateResourceHandler(
 
     DCHECK(!mojo_request.is_pending());
     DCHECK(!url_loader_client);
-    handler.reset(new SyncResourceHandler(request, sync_result, this));
+    handler.reset(new SyncResourceHandler(request, sync_result_handler, this));
   } else {
     if (mojo_request.is_pending()) {
       handler.reset(new MojoAsyncResourceHandler(request, this,
@@ -1565,8 +1583,9 @@ ResourceDispatcherHostImpl::CreateResourceHandler(
   bool start_detached = request_data.download_to_network_cache_only;
 
   // Prefetches and <a ping> requests outlive their child process.
-  if (!sync_result && (start_detached ||
-                       IsDetachableResourceType(request_data.resource_type))) {
+  if (!sync_result_handler &&
+      (start_detached ||
+       IsDetachableResourceType(request_data.resource_type))) {
     std::unique_ptr<DetachableResourceHandler> detachable_handler =
         base::MakeUnique<DetachableResourceHandler>(
             request,