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

Index: content/browser/loader/resource_dispatcher_host_impl.cc
===================================================================
--- content/browser/loader/resource_dispatcher_host_impl.cc	(revision 221543)
+++ content/browser/loader/resource_dispatcher_host_impl.cc	(working copy)
@@ -138,7 +138,6 @@
 // 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;
@@ -148,7 +147,6 @@
   } else {
     // Tell the renderer that this request was disallowed.
     filter->Send(new ResourceMsg_RequestComplete(
-        route_id,
         request_id,
         net::ERR_ABORTED,
         false,
@@ -844,6 +842,11 @@
         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) {
@@ -929,7 +932,7 @@
   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;
   }
 
@@ -942,7 +945,7 @@
                                                   request_data.url,
                                                   request_data.resource_type,
                                                   resource_context)) {
-    AbortRequestBeforeItStarts(filter_, sync_result, route_id, request_id);
+    AbortRequestBeforeItStarts(filter_, sync_result, request_id);
     return;
   }
 
@@ -1041,8 +1044,7 @@
     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.