WebUI: shuffle less between threads when responding to requests

content/browser/webui/url_data_manager_backend.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.
 
 #include "content/browser/webui/url_data_manager_backend.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 251 matching lines @@
 URLRequestChromeJob::~URLRequestChromeJob() {
   CHECK(!backend_->HasPendingJob(this));
 }
 
 void URLRequestChromeJob::Start() {
   int render_process_id, unused;
   bool is_renderer_request = ResourceRequestInfo::GetRenderFrameForRequest(
       request_, &render_process_id, &unused);
   if (!is_renderer_request)
     render_process_id = kNoRenderProcessId;
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&URLRequestChromeJob::CheckStoragePartitionMatches,
-                 render_process_id, request_->url(),
-                 weak_factory_.GetWeakPtr()));
+
+  if (!is_renderer_request || request_->url().SchemeIs(kChromeUIScheme)) {
+    StartAsync(true);
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&URLRequestChromeJob::CheckStoragePartitionMatches,
+                   render_process_id, request_->url(),
+                   weak_factory_.GetWeakPtr()));
+  }
+
   TRACE_EVENT_ASYNC_BEGIN1("browser", "DataManager:Request", this, "URL",
       request_->url().possibly_invalid_spec());
 }
 
 void URLRequestChromeJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
   backend_->RemoveRequest(this);
   URLRequestJob::Kill();
 }
 
@@ skipping 101 matching lines @@
 void URLRequestChromeJob::CheckStoragePartitionMatches(
     int render_process_id,
     const GURL& url,
     const base::WeakPtr<URLRequestChromeJob>& job) {
   // The embedder could put some webui pages in separate storage partition.
   // RenderProcessHostImpl::IsSuitableHost would guard against top level pages
   // being in the same process. We do an extra check to guard against an
   // exploited renderer pretending to add them as a subframe. We skip this check
   // for resources.
   bool allowed = false;
-  std::vector<std::string> hosts;
-  GetContentClient()->
-      browser()->GetAdditionalWebUIHostsToIgnoreParititionCheck(&hosts);
-  if (url.SchemeIs(kChromeUIScheme) &&
-      (url.SchemeIs(kChromeUIScheme) ||
-       std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end())) {

[Dan Beam, 2016/05/17 03:24:14]

|hosts| was never used because

  if (thing && (thing || other_thing))

never hits other_thing. this check was effectively:

  if (url.SchemeIs(kChromeUIScheme)) {

which can be performed synchronously from the IO thread without posting to UI
thread and back.  same with the PID check.

[Avi (use Gerrit), 2016/05/17 03:30:20]

Right, but what CL introduced this silliness? What were they intending to do?
Were what they were worrying about something we should fix rather than just rip
out?

[Dan Beam, 2016/05/17 03:32:16]

https://codereview.chromium.org/183803023/#msg10

[Charlie Reis, 2016/05/17 21:08:10]

Yeah, this code is definitely broken after
https://codereview.chromium.org/183803023/.

That was a refactor to https://codereview.chromium.org/130963006, which moved
chrome://chrome-signin from a webview to an iframe.

Looks like the intent was to make sure that only whitelisted WebUI hosts could
get past the StoragePartition check, but after the refactor *any* chrome:// URL
could (which meant powerful pages like chrome://settings, etc).  That kind of
defeated the point of the check.  (I'm not sure how we were able to have an
iframe with a different StoragePartition in the first place-- I don't understand
that part of the original CL's description.)

However, if I understand correctly, we have since moved back to the webview
approach for sign-in, and the iframe approach is no longer used.  And yes, the
old iframe based approach looks like it was removed in rogerta's
https://codereview.chromium.org/1412453002.

I'm not sure why this code wasn't removed along with the rest-- hopefully it
isn't necessary to load any WebUI pages (within the host whitelist or not)
inside the webview, so this check shouldn't be needed.  Maybe the entire
CheckStoragePartitionMatches function can go away, since it was only introduced
for the iframe version?  (Indeed, it seems a bit unsafe to leave it here,
certainly as written.  It looks like we block navigating the webview to
chrome:// URLs in other ways when I try it out, but it feels risky to have this
path around.)

Roger, can you confirm whether this has any value now?

-    allowed = true;
-  } else if (render_process_id == kNoRenderProcessId) {
-    // Request was not issued by renderer.
-    allowed = true;
-  } else {
-    RenderProcessHost* process = RenderProcessHost::FromID(render_process_id);
-    if (process) {
-      StoragePartition* partition = BrowserContext::GetStoragePartitionForSite(
-          process->GetBrowserContext(), url);
-      allowed = partition == process->GetStoragePartition();
-    }
+  RenderProcessHost* process = RenderProcessHost::FromID(render_process_id);
+  if (process) {
+    StoragePartition* partition = BrowserContext::GetStoragePartitionForSite(
+        process->GetBrowserContext(), url);
+    allowed = partition == process->GetStoragePartition();
   }
-
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&URLRequestChromeJob::StartAsync, job, allowed));
 }
 
 void URLRequestChromeJob::StartAsync(bool allowed) {
   if (!request_)
     return;
 
@@ skipping 368 matching lines @@
 
 }  // namespace
 
 net::URLRequestJobFactory::ProtocolHandler*
 CreateDevToolsProtocolHandler(content::ResourceContext* resource_context,
                               bool is_incognito) {
   return new DevToolsJobFactory(resource_context, is_incognito);
 }
 
 }  // namespace content