Make SyncLoad result handling pluggable

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 100 matching lines @@
 #include "storage/browser/blob/blob_url_request_job_factory.h"
 #include "storage/browser/blob/shareable_file_reference.h"
 #include "storage/browser/fileapi/file_permission_policy.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "url/url_constants.h"
 
 using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 using storage::ShareableFileReference;
+using SyncLoadResultCallback =
+    content::ResourceDispatcherHostImpl::SyncLoadResultCallback;
 
 // ----------------------------------------------------------------------------
 
 namespace content {
 
 namespace {
 
 static ResourceDispatcherHostImpl* g_resource_dispatcher_host;
 
 // The interval for calls to ResourceDispatcherHostImpl::UpdateLoadStates
@@ skipping 82 matching lines @@
     case RESOURCE_TYPE_PREFETCH:
     case RESOURCE_TYPE_PING:
     case RESOURCE_TYPE_CSP_REPORT:
       return true;
     default:
       return false;
   }
 }
 
 // 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;
     request_complete_data.error_code = net::ERR_ABORTED;
     request_complete_data.was_ignored_by_handler = false;
     request_complete_data.exists_in_cache = false;
     // No security info needed, connection not established.
     request_complete_data.completion_time = base::TimeTicks();
     request_complete_data.encoded_data_length = 0;
     if (url_loader_client) {
@@ skipping 164 matching lines @@
     if (frame_host)
       routing_ids->insert(frame_host->GetGlobalFrameRoutingId());
     if (pending_frame_host)
       routing_ids->insert(pending_frame_host->GetGlobalFrameRoutingId());
   }
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&NotifyForRouteSetOnIO, frame_callback,
                                      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() {}
 ResourceDispatcherHostImpl::LoadInfo::LoadInfo(const LoadInfo& other) = default;
 ResourceDispatcherHostImpl::LoadInfo::~LoadInfo() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::HeaderInterceptorInfo() {}
 
 ResourceDispatcherHostImpl::HeaderInterceptorInfo::~HeaderInterceptorInfo() {}
 
@@ skipping 625 matching lines @@
       !IsBrowserSideNavigationEnabled()) {
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&LogResourceRequestTimeOnUI,
                    TimeTicks::Now(),
                    filter_->child_id(),
                    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));
 }
 
 // Begins a resource request with the given params on behalf of the specified
 // child process.  Responses will be dispatched through the given receiver. The
 // process ID is used to lookup WebContentsImpl from routing_id's in the case of
 // a request from a renderer.  request_context is the cookie/cache context to be
 // used for this request.
 //
 // If sync_result is non-null, then a SyncLoad reply will be generated, else
 // a normal asynchronous set of response messages will be generated.
 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);
 }
 
 bool ResourceDispatcherHostImpl::IsRequestIDInUse(
     const GlobalRequestID& id) const {
   if (pending_loaders_.find(id) != pending_loaders_.end())
     return true;
   for (const auto& blocked_loaders : blocked_loaders_map_) {
     for (const auto& loader : *blocked_loaders.second.get()) {
       ResourceRequestInfoImpl* info = loader->GetRequestInfo();
@@ skipping 83 matching lines @@
     } else {
       handler->CompleteCrossSiteTransfer(
           child_id, request_data.service_worker_provider_id);
     }
   }
 }
 
 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) {
   int process_type = filter_->process_type();
   int child_id = filter_->child_id();
 
   // Reject request id that's currently in use.
   if (IsRequestIDInUse(GlobalRequestID(child_id, request_id))) {
     bad_message::ReceivedBadMessage(filter_,
                                     bad_message::RDH_INVALID_REQUEST_ID);
@@ skipping 54 matching lines @@
                        &request_context);
 
   // Parse the headers before calling ShouldServiceRequest, so that they are
   // available to be validated.
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(request_data.headers);
 
   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;
   }
   // Check if we have a registered interceptor for the headers passed in. If
   // yes then we need to mark the current request as pending and wait for the
   // interceptor to invoke the callback with a status code indicating whether
   // the request needs to be aborted or continued.
   for (net::HttpRequestHeaders::Iterator it(headers); it.GetNext();) {
     HeaderInterceptorMap::iterator index =
         http_header_interceptor_map_.find(it.name());
     if (index != http_header_interceptor_map_.end()) {
       HeaderInterceptorInfo& interceptor_info = index->second;
 
       bool call_interceptor = true;
       if (!interceptor_info.starts_with.empty()) {
         call_interceptor =
             base::StartsWith(it.value(), interceptor_info.starts_with,
                              base::CompareCase::INSENSITIVE_ASCII);
       }
       if (call_interceptor) {
         interceptor_info.interceptor.Run(
             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,
     mojom::URLLoaderClientPtr url_loader_client,
     bool continue_request,
     int error_code) {
   if (!continue_request) {
     // 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;
   }
 
   int process_type = filter_->process_type();
   int child_id = filter_->child_id();
 
   bool is_navigation_stream_request =
       IsBrowserSideNavigationEnabled() &&
       IsResourceTypeFrame(request_data.resource_type) &&
       process_type == PROCESS_TYPE_RENDERER;
 
   ResourceContext* resource_context = NULL;
   net::URLRequestContext* request_context = NULL;
   filter_->GetContexts(request_data.resource_type, &resource_context,
                        &request_context);
 
   // Allow the observer to block/handle the request.
   if (delegate_ && !delegate_->ShouldBeginRequest(request_data.method,
                                                   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;
   }
 
   // Construct the request.
   std::unique_ptr<net::URLRequest> new_request = request_context->CreateRequest(
       is_navigation_stream_request ? request_data.resource_body_stream_url
                                    : request_data.url,
       request_data.priority, nullptr);
 
@@ skipping 52 matching lines @@
     }
     new_request->set_upload(UploadDataStreamBuilder::Build(
         request_data.request_body.get(), blob_context,
         filter_->file_system_context(),
         BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE).get()));
   }
 
   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.
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
   bool report_raw_headers = request_data.report_raw_headers;
   if (report_raw_headers && !policy->CanReadRawCookies(child_id)) {
     // TODO: crbug.com/523063 can we call bad_message::ReceivedBadMessage here?
     VLOG(1) << "Denied unauthorized request for raw headers";
     report_raw_headers = false;
@@ skipping 91 matching lines @@
       request_data.fetch_frame_type, request_data.request_body,
       request_data.initiated_in_secure_context);
 
   // Have the appcache associate its extra info with the request.
   AppCacheInterceptor::SetExtraRequestInfo(
       new_request.get(), filter_->appcache_service(), child_id,
       request_data.appcache_host_id, request_data.resource_type,
       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)
     BeginRequestInternal(std::move(new_request), std::move(handler));
 }
 
 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,
     ResourceContext* resource_context,
     mojo::InterfaceRequest<mojom::URLLoader> mojo_request,
     mojom::URLLoaderClientPtr url_loader_client) {
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/456331 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "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);
       return std::unique_ptr<ResourceHandler>();
     }
 
     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,
                                                  std::move(mojo_request),
                                                  std::move(url_loader_client)));
     } else {
       handler.reset(new AsyncResourceHandler(request, this));
     }
 
     // The RedirectToFileResourceHandler depends on being next in the chain.
     if (request_data.download_to_file) {
       handler.reset(
           new RedirectToFileResourceHandler(std::move(handler), request));
     }
   }
 
   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,
             base::TimeDelta::FromMilliseconds(kDefaultDetachableCancelDelayMs),
             std::move(handler));
     if (start_detached)
       detachable_handler->Detach();
     handler = std::move(detachable_handler);
   }
 
@@ skipping 1123 matching lines @@
                                  &throttles);
     if (!throttles.empty()) {
       handler.reset(new ThrottlingResourceHandler(std::move(handler), request,
                                                   std::move(throttles)));
     }
   }
   return handler;
 }
 
 }  // namespace content