Convert a bunch of resource IPCs to be control messages. They didn't need to be routed.

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 <set>
 #include <vector>
@@ skipping 120 matching lines @@
 // are all positive (since histograms expect positive sample values).
 const int kAllNetErrorCodes[] = {
 #define NET_ERROR(label, value) -(value),
 #include "net/base/net_error_list.h"
 #undef NET_ERROR
 };
 
 // Aborts a request before an URLRequest has actually been created.
 void AbortRequestBeforeItStarts(ResourceMessageFilter* filter,
                                 IPC::Message* sync_result,
-                                int route_id,
                                 int request_id) {
   if (sync_result) {
     SyncLoadResult result;
     result.error_code = net::ERR_ABORTED;
     ResourceHostMsg_SyncLoad::WriteReplyParams(sync_result, result);
     filter->Send(sync_result);
   } else {
     // Tell the renderer that this request was disallowed.
     filter->Send(new ResourceMsg_RequestComplete(
-        route_id,
         request_id,
         net::ERR_ABORTED,
         false,
         std::string(),   // No security info needed, connection not established.
         base::TimeTicks()));
   }
 }
 
 void SetReferrerForRequest(net::URLRequest* request, const Referrer& referrer) {
   if (!referrer.url.is_valid() ||
@@ skipping 675 matching lines @@
     DCHECK(ok);
     GlobalRequestID id(filter_->child_id(), request_id);
     DelegateMap::iterator it = delegate_map_.find(id);
     if (it != delegate_map_.end()) {
       ObserverList<ResourceMessageDelegate>::Iterator del_it(*it->second);
       ResourceMessageDelegate* delegate;
       while (!handled && (delegate = del_it.GetNext()) != NULL) {
         handled = delegate->OnMessageReceived(message, message_was_ok);
       }
     }
+
+    // There could be race conditions in receiving ACKs after a request is
+    // removed. If the control IPC isn't handled here, an assert will fire in
+    // RenderProcessHostImpl::OnMessageReceived.
+    handled = true;

[awong, 2013/09/06 17:04:11]

If I read this correctly, any ResourceHostMsg will be considered handled now.
That feels overly broad.  Can this be restricted to just the set of ack messages
that we expect to be racy?

Also, is this a problem for all _ACK messages?  If not, then I think the comment
could use a little more detail.

[jam, 2013/09/06 17:12:04]

right
I didn't want to play whac-a-mole with different resource IPCs triggering that
dcheck. I couldn't think of any issues of handling all IPCs here. can you?

Another alternative is to remove that dcheck in RenderProcessHost. It seems odd
that we dcheck for control IPCs, but not routed.
I saw this for ResourceHostMsg_DataReceived_ACK, but it wasn't clear to me what
other IPCs could trigger this.

[jam, 2013/09/06 17:22:17]

btw I've uploaded a new patchset that does this now. the more I think about it,
the more that seems better. wdyt?

[awong, 2013/09/06 17:37:34]

It's consistent now so that bit is better.

However, I was personally would have expected that we should go the other
direction -- all unhandled IPCs result in an error rather than just black
holing.  With this change the failure mode becomes a timeout/hang instead of a
crash which is more annoying to debug.  It also seems like it might mask people
making having non-benign cross-process synchronization and races (even though
the case you cited may be benign, but I worry others might not).

I was actually hoping to error on unhandled messages for RenderFrameHost's
OnMessageReceived, but maybe that policy to too strict for this level of the
stack?

Having said all that, I don't have a super strong opinion on this so whatever
you think is best.

[jam, 2013/09/06 18:00:20]

Given the asynchronous nature of our code base, I think we naturally have a lot
of handled IPCs around teardown. We've been ignoring them all for now, other
than control IPCs. I guess we can come up with alternate ways of ignoring them
while not ignoring the non-teardown case, perhaps by telling the channel to
ignore route X from now on every time an object is deleted. Before we go down
that extra complexity, I'd prefer to see examples of bugs that happened because
IPCs were dropped silently.

   }
 
   if (message.type() == ViewHostMsg_DidLoadResourceFromMemoryCache::ID) {
     // We just needed to peek at this message. We still want it to reach its
     // normal destination.
     handled = false;
   }
 
   filter_ = NULL;
   return handled;
@@ skipping 65 matching lines @@
     }
   }
 
   ResourceContext* resource_context = filter_->resource_context();
   // http://crbug.com/90971
   CHECK(ContainsKey(active_resource_contexts_, resource_context));
 
   if (is_shutdown_ ||
       !ShouldServiceRequest(process_type, child_id, request_data,
                             filter_->file_system_context())) {
-    AbortRequestBeforeItStarts(filter_, sync_result, route_id, request_id);
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id);
     return;
   }
 
   const Referrer referrer(request_data.referrer, request_data.referrer_policy);
 
   // Allow the observer to block/handle the request.
   if (delegate_ && !delegate_->ShouldBeginRequest(child_id,
                                                   route_id,
                                                   request_data.method,
                                                   request_data.url,
                                                   request_data.resource_type,
                                                   resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, route_id, request_id);
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id);
     return;
   }
 
   bool is_sync_load = sync_result != NULL;
   int load_flags =
       BuildLoadFlagsForRequest(request_data, child_id, is_sync_load);
 
   GlobalRoutingID id(child_id, route_id);
   if (!offline_policy_map_[id])
     offline_policy_map_[id] = new OfflinePolicy();
@@ skipping 78 matching lines @@
   appcache::AppCacheInterceptor::SetExtraRequestInfo(
       request, filter_->appcache_service(), child_id,
       request_data.appcache_host_id, request_data.resource_type);
 
   // Construct the IPC resource handler.
   scoped_ptr<ResourceHandler> handler;
   if (sync_result) {
     handler.reset(new SyncResourceHandler(
         filter_, request, sync_result, this));
   } else {
-    handler.reset(new AsyncResourceHandler(
-        filter_, route_id, request, this));
+    handler.reset(new AsyncResourceHandler(filter_, request, this));
   }
 
   // The RedirectToFileResourceHandler depends on being next in the chain.
   if (request_data.download_to_file) {
     handler.reset(
         new RedirectToFileResourceHandler(handler.Pass(), child_id, this));
   }
 
   // Install a CrossSiteResourceHandler if this request is coming from a
   // RenderViewHost with a pending cross-site request.  We only check this for
@@ skipping 823 matching lines @@
   if ((load_flags & net::LOAD_REPORT_RAW_HEADERS)
       && !policy->CanReadRawCookies(child_id)) {
     VLOG(1) << "Denied unauthorized request for raw headers";
     load_flags &= ~net::LOAD_REPORT_RAW_HEADERS;
   }
 
   return load_flags;
 }
 
 }  // namespace content