WebUI: shuffle less between threads when responding to requests

content/browser/webui/url_data_manager_backend.cc

Index: content/browser/webui/url_data_manager_backend.cc
diff --git a/content/browser/webui/url_data_manager_backend.cc b/content/browser/webui/url_data_manager_backend.cc
index ff5d602d4225626ca13b2b0a8f099e6024688e2c..c74928dc8268fca28fb70675bd72dd4a9bef8e34 100644
--- a/content/browser/webui/url_data_manager_backend.cc
+++ b/content/browser/webui/url_data_manager_backend.cc
@@ -269,12 +269,18 @@ void URLRequestChromeJob::Start() {
       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());
 }
@@ -396,25 +402,12 @@ void URLRequestChromeJob::CheckStoragePartitionMatches(
   // 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,