Extra browser-side validation of transferred_request_child_id / reque…

…st_id.

BUG=656179

Review-Url: https://codereview.chromium.org/2442793002
Cr-Commit-Position: refs/heads/master@{#427510}

content/browser/bad_message.h

@@ -167,6 +167,8 @@ enum BadMessageReason {
   SWDH_ENABLE_NAVIGATION_PRELOAD_NO_HOST = 143,
   SWDH_ENABLE_NAVIGATION_PRELOAD_INVALID_ORIGIN = 144,
   SWDH_ENABLE_NAVIGATION_PRELOAD_BAD_REGISTRATION_ID = 145,
+  RDH_TRANSFERRING_REQUEST_NOT_FOUND = 146,
+  RDH_TRANSFERRING_NONNAVIGATIONAL_REQUEST = 147,
 
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. RenderFrameHost becomes RFH) plus a unique description of the

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -1195,6 +1195,56 @@ void ResourceDispatcherHostImpl::UpdateRequestForTransfer(
   }
 }
 
+void ResourceDispatcherHostImpl::CompleteTransfer(
+    int request_id,
+    const ResourceRequest& request_data,
+    int route_id) {
+  // Caller should ensure that |request_data| is associated with a transfer.
+  DCHECK(request_data.transferred_request_child_id != -1 ||
+         request_data.transferred_request_request_id != -1);
+
+  bool is_navigational_request =
+      request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME ||
+      request_data.resource_type == RESOURCE_TYPE_SUB_FRAME;
+  if (!is_navigational_request) {
+    // Transfers apply only to navigational requests - the renderer seems to
+    // have sent bogus IPC data.
+    bad_message::ReceivedBadMessage(
+        filter_, bad_message::RDH_TRANSFERRING_NONNAVIGATIONAL_REQUEST);
+    return;
+  }
+
+  // Attempt to find a loader associated with the deferred transfer request.
+  LoaderMap::iterator it = pending_loaders_.find(
+      GlobalRequestID(request_data.transferred_request_child_id,
+                      request_data.transferred_request_request_id));
+  if (it == pending_loaders_.end()) {
+    // Renderer sent transferred_request_request_id and/or
+    // transferred_request_child_id that doesn't have a corresponding entry on
+    // the browser side.
+    bad_message::ReceivedBadMessage(
+        filter_, bad_message::RDH_TRANSFERRING_REQUEST_NOT_FOUND);
+    return;
+  }
+  ResourceLoader* pending_loader = it->second.get();
+
+  if (!pending_loader->is_transferring()) {
+    // Renderer sent transferred_request_request_id and/or
+    // transferred_request_child_id that doesn't correspond to an actually
+    // transferring loader on the browser side.
+    base::debug::Alias(pending_loader);
+    bad_message::ReceivedBadMessage(filter_,
+                                    bad_message::RDH_REQUEST_NOT_TRANSFERRING);
+    return;
+  }
+
+  // If the request is transferring to a new process, we can update our
+  // state and let it resume with its existing ResourceHandlers.
+  UpdateRequestForTransfer(filter_->child_id(), route_id, request_id,
+                           request_data, it);
+  pending_loader->CompleteTransfer();
+}
+
 void ResourceDispatcherHostImpl::BeginRequest(
     int request_id,
     const ResourceRequest& request_data,
@@ -1239,24 +1289,12 @@ void ResourceDispatcherHostImpl::BeginRequest(
 
   // If the request that's coming in is being transferred from another process,
   // we want to reuse and resume the old loader rather than start a new one.
-  LoaderMap::iterator it = pending_loaders_.find(
-      GlobalRequestID(request_data.transferred_request_child_id,
-                      request_data.transferred_request_request_id));
-  if (it != pending_loaders_.end()) {
+  if (request_data.transferred_request_child_id != -1 ||
+      request_data.transferred_request_request_id != -1) {
     // TODO(yhirano): Make mojo work for this case.
     DCHECK(!url_loader_client);
 
-    // If the request is transferring to a new process, we can update our
-    // state and let it resume with its existing ResourceHandlers.
-    if (it->second->is_transferring()) {
-      ResourceLoader* deferred_loader = it->second.get();
-      UpdateRequestForTransfer(child_id, route_id, request_id,
-                               request_data, it);
-      deferred_loader->CompleteTransfer();
-    } else {
-      bad_message::ReceivedBadMessage(
-          filter_, bad_message::RDH_REQUEST_NOT_TRANSFERRING);
-    }
+    CompleteTransfer(request_id, request_data, route_id);
     return;
   }
 

content/browser/loader/resource_dispatcher_host_impl.h

@@ -549,6 +549,12 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
                                 const ResourceRequest& request_data,
                                 LoaderMap::iterator iter);
 
+  // If |request_data| is for a request being transferred from another process,
+  // then CompleteTransfer method can be used to complete the transfer.
+  void CompleteTransfer(int request_id,
+                        const ResourceRequest& request_data,
+                        int route_id);
+
   void BeginRequest(
       int request_id,
       const ResourceRequest& request_data,

content/renderer/render_frame_impl.cc

@@ -4227,12 +4227,6 @@ void RenderFrameImpl::willSendRequest(blink::WebLocalFrame* frame,
       navigation_state->common_params().allow_download);
   extra_data->set_transition_type(transition_type);
   extra_data->set_should_replace_current_entry(should_replace_current_entry);
-  // TODO(lukasza): https://crbug.com/656179: Navigational things (e.g.
-  // StartNavigationParams) should not apply to subresource requests.
-  extra_data->set_transferred_request_child_id(
-      navigation_state->start_params().transferred_request_child_id);
-  extra_data->set_transferred_request_request_id(
-      navigation_state->start_params().transferred_request_request_id);
   extra_data->set_service_worker_provider_id(provider_id);
   extra_data->set_stream_override(std::move(stream_override));
   bool is_prefetch =
@@ -4244,6 +4238,17 @@ void RenderFrameImpl::willSendRequest(blink::WebLocalFrame* frame,
   WebString error;
   extra_data->set_initiated_in_secure_context(
       frame->document().isSecureContext(error));
+
+  // Renderer process transfers apply only to navigational requests.
+  bool is_navigational_request =
+      request.getFrameType() != WebURLRequest::FrameTypeNone;
+  if (is_navigational_request) {
+    extra_data->set_transferred_request_child_id(
+        navigation_state->start_params().transferred_request_child_id);
+    extra_data->set_transferred_request_request_id(
+        navigation_state->start_params().transferred_request_request_id);
+  }
+
   request.setExtraData(extra_data);
 
   if (request.getLoFiState() == WebURLRequest::LoFiUnspecified) {
@@ -4267,8 +4272,8 @@ void RenderFrameImpl::willSendRequest(blink::WebLocalFrame* frame,
   // to subresource requests).  For example - Content-Type header provided via
   // OpenURLParams::extra_headers should only be applied to the original POST
   // navigation request (and not to subresource requests).
-  if (!navigation_state->start_params().extra_headers.empty() &&
-      request.getFrameType() != WebURLRequest::FrameTypeNone) {
+  if (is_navigational_request &&
+      !navigation_state->start_params().extra_headers.empty()) {
     for (net::HttpUtil::HeadersIterator i(
              navigation_state->start_params().extra_headers.begin(),
              navigation_state->start_params().extra_headers.end(), "\n");