ResourceScheduler: remove dependency on ResourceRequestInfo and request_id

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 <algorithm>
 #include <set>
@@ skipping 960 matching lines @@
   filter_ = filter;
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ResourceDispatcherHostImpl, message)
     IPC_MESSAGE_HANDLER(ResourceHostMsg_RequestResource, OnRequestResource)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(ResourceHostMsg_SyncLoad, OnSyncLoad)
     IPC_MESSAGE_HANDLER(ResourceHostMsg_ReleaseDownloadedFile,
                         OnReleaseDownloadedFile)
     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()
 
   if (!handled && IPC_MESSAGE_ID_CLASS(message.type()) == ResourceMsgStart) {
     base::PickleIterator iter(message);
     int request_id = -1;
     bool ok = iter.ReadInt(&request_id);
     DCHECK(ok);
     GlobalRequestID id(filter_->child_id(), request_id);
     DelegateMap::iterator it = delegate_map_.find(id);
@@ skipping 437 matching lines @@
                                 appcache_service,
                                 resource_type,
                                 &throttles);
   }
 
   if (request->has_upload()) {
     // Block power save while uploading data.
     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()));
 
   return handler.Pass();
 }
 
 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
 }
 
 void ResourceDispatcherHostImpl::RegisterDownloadedTempFile(
     int child_id, int request_id, const base::FilePath& file_path) {
   scoped_refptr<ShareableFileReference> reference =
       ShareableFileReference::Get(file_path);
   DCHECK(reference.get());
 
@@ skipping 893 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