ResourceScheduler: remove dependency on ResourceRequestInfo and request_id

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 5600a23455fc80f8f2a6592077a78be2babcb165..6f66055296c57547c44a6f21719932c4fd67551e 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -978,6 +978,7 @@ bool ResourceDispatcherHostImpl::OnMessageReceived(
     IPC_MESSAGE_HANDLER(ResourceHostMsg_DataDownloaded_ACK, OnDataDownloadedACK)
     IPC_MESSAGE_HANDLER(ResourceHostMsg_UploadProgress_ACK, OnUploadProgressACK)
     IPC_MESSAGE_HANDLER(ResourceHostMsg_CancelRequest, OnCancelRequest)
+    IPC_MESSAGE_HANDLER(ResourceHostMsg_DidChangePriority, OnDidChangePriority)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
@@ -1435,8 +1436,13 @@ scoped_ptr<ResourceHandler> ResourceDispatcherHostImpl::AddStandardHandlers(
     throttles.push_back(new PowerSaveBlockResourceThrottle());
   }
 
-  throttles.push_back(
-      scheduler_->ScheduleRequest(child_id, route_id, request).release());
+  // TODO(ricea): Stop looking this up so much.
+  ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(request);
+  scoped_ptr<ScheduledResourceRequest> scheduled_resource_request(
+      scheduler_->ScheduleRequest(child_id, route_id, info->IsAsync(),
+                                  request));
+  info->set_scheduled_resource_request(scheduled_resource_request.get());

[davidben, 2015/08/12 23:58:11]

There is a sliiiiight awkwardness in that it's not locally obvious that the
throttle outlives info. (In fact, it doesn't. There's a short window where the
pointer is dangling because the handlers go away before the URLRequest.)

I don't have much better of a suggestion here, because there isn't really a
single object that knows everything. The RDH knows about ResourceScheduler but
doesn't know that we don't drop this throttle early. (We don't, but if the
scheduler sat beneath the MimeTypeResourceHandler, we would.)
DetachableResourceHandler, ugly as it is, works because it unsets it in its
destructor.

Maybe stick it on a URLRequest UserData for now? I'm not super-happy with it,
but it's the best I've come up with. (Maybe you can come up with something
better?) Then ScheduledResourceRequest can clear the pointer in its destructor.
I'll try to make sure there's a more sensible way to do this when I refactor
this stack. I have some plans to make the stuff with the handlers a little more
controlled.

(Sorry I keep asking you to do different things. I should have foreseen this
one.)

[Adam Rice, 2015/08/13 21:21:02]

If we can guarantee that no-one forges a DidChangePriority message or runs an
event loop while inside the ResourceLoader destructor, then extra care is not
needed. But I'm not super-confident of that, so I have put it on a separate
UserData key as you suggested.

[davidben, 2015/08/13 23:08:52]

Yeah, I think right now we are guaranteed to not have a problem. But it would
break as soon as, say, we stick above a ResourceHandler like the
MimeTypeResourceHandler that drops its child throttles.

(What do you mean by forge a DidChangePriority message here? I don't follow.)

[Adam Rice, 2015/08/13 23:42:28]

The only think that actually looked at that field was
ResourceDispatcherHostImpl::OnDidChangePriority(), which is private and can only
be called by creating a DidChangePriority message and passing it to
OnMessageReceived.

I forgot that some ResourceHandler could just decide to poke at the
ScheduledResourceRequest directly.

+  throttles.push_back(scheduled_resource_request.Pass());
 
   handler.reset(
       new ThrottlingResourceHandler(handler.Pass(), request, throttles.Pass()));
@@ -1448,6 +1454,19 @@ void ResourceDispatcherHostImpl::OnReleaseDownloadedFile(int request_id) {
   UnregisterDownloadedTempFile(filter_->child_id(), request_id);
 }
 
+void ResourceDispatcherHostImpl::OnDidChangePriority(
+    int request_id,
+    net::RequestPriority new_priority,
+    int intra_priority_value) {

[davidben, 2015/08/12 23:58:11]

Since this is coming from the renderer, it should check for both GetLoader
returning NULL and scheduled_resource_request being NULL and handle it
gracefully.

[Adam Rice, 2015/08/13 21:21:02]

I should have spotted that. Done.

+  ScheduledResourceRequest* scheduled_resource_request =
+      GetLoader(filter_->child_id(), request_id)
+          ->GetRequestInfo()
+          ->scheduled_resource_request();
+  DCHECK(scheduled_resource_request);
+  scheduled_resource_request->ChangePriority(new_priority,
+                                             intra_priority_value);
+}
+
 void ResourceDispatcherHostImpl::OnDataDownloadedACK(int request_id) {
   // TODO(michaeln): maybe throttle DataDownloaded messages
 }