Offload disk accesses to WorkerPool in ExtensionProtocolHandler

chrome/browser/extensions/extension_protocols.cc

Index: chrome/browser/extensions/extension_protocols.cc
===================================================================
--- chrome/browser/extensions/extension_protocols.cc	(revision 145483)
+++ chrome/browser/extensions/extension_protocols.cc	(working copy)
@@ -9,11 +9,13 @@
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/logging.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/threading/worker_pool.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
@@ -58,6 +60,12 @@
   return new net::HttpResponseHeaders(raw_headers);
 }
 
+void ReadMimeTypeFromFile(const FilePath& filename,
+                          std::string* mime_type,
+                          bool* result) {
+  *result = net::GetMimeTypeFromFile(filename, mime_type);
+}
+
 class URLRequestResourceBundleJob : public net::URLRequestSimpleJob {
  public:
   URLRequestResourceBundleJob(
@@ -65,35 +73,38 @@
       const std::string& content_security_policy, bool send_cors_header)
       : net::URLRequestSimpleJob(request),
         filename_(filename),
-        resource_id_(resource_id) {
+        resource_id_(resource_id),
+        weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
     response_info_.headers = BuildHttpHeaders(content_security_policy,
                                               send_cors_header);
   }
 
   // Overridden from URLRequestSimpleJob:
-  virtual bool GetData(std::string* mime_type,
-                       std::string* charset,
-                       std::string* data) const OVERRIDE {
+  virtual int GetData(std::string* mime_type,
+                      std::string* charset,
+                      std::string* data,
+                      const net::CompletionCallback& callback) const OVERRIDE {
     const ResourceBundle& rb = ResourceBundle::GetSharedInstance();
     *data = rb.GetRawDataResource(
         resource_id_, ui::SCALE_FACTOR_NONE).as_string();
 
-    // Requests should not block on the disk!  On Windows this goes to the
-    // registry.
-    //   http://code.google.com/p/chromium/issues/detail?id=59849
-    bool result;
-    {
-      base::ThreadRestrictions::ScopedAllowIO allow_io;
-      result = net::GetMimeTypeFromFile(filename_, mime_type);
-    }
+    std::string* read_mime_type = new std::string;
+    bool* read_result = new bool;
+    bool posted = base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&ReadMimeTypeFromFile, filename_,
+                   base::Unretained(read_mime_type),
+                   base::Unretained(read_result)),
+        base::Bind(&URLRequestResourceBundleJob::OnMimeTypeRead,
+                   weak_factory_.GetWeakPtr(),
+                   mime_type, charset, data,
+                   base::Owned(read_mime_type),
+                   base::Owned(read_result),
+                   callback),
+        true /* task is slow */);
+    DCHECK(posted);
 
-    if (StartsWithASCII(*mime_type, "text/", false)) {
-      // All of our HTML files should be UTF-8 and for other resource types
-      // (like images), charset doesn't matter.
-      DCHECK(IsStringUTF8(*data));
-      *charset = "utf-8";
-    }
-    return result;
+    return net::ERR_IO_PENDING;
   }
 
   virtual void GetResponseInfo(net::HttpResponseInfo* info) {
@@ -103,6 +114,23 @@
  private:
   virtual ~URLRequestResourceBundleJob() { }
 
+  void OnMimeTypeRead(std::string* out_mime_type,
+                      std::string* charset,
+                      std::string* data,
+                      std::string* read_mime_type,
+                      bool* read_result,
+                      const net::CompletionCallback& callback) {
+    *out_mime_type = *read_mime_type;
+    if (StartsWithASCII(*read_mime_type, "text/", false)) {
+      // All of our HTML files should be UTF-8 and for other resource types
+      // (like images), charset doesn't matter.
+      DCHECK(IsStringUTF8(*data));
+      *charset = "utf-8";
+    }
+    int result = *read_result? net::OK: net::ERR_INVALID_URL;

[wtc, 2012/07/13 22:55:14]

The use of net::ERR_INVALID_URL here was not obvious.  (I
know it was meant to match the error code used in the original
code.)  I would think that the most common reason for
net::GetMimeTypeFromFile to fail is ERR_FILE_NOT_FOUND.
I guess ERR_INVALID_URL can be interpreted to mean the file
URL is invalid?

[wtc, 2012/07/14 00:08:55]

Thank you for the explanation.  I didn't think about this
hard enough.  I saw net::GetMimeTypeFromFile takes a 'filename'
argument and just assumed the most common reason of failure is
"file not found".

I agree that ERR_INVALID_URL is appropriate here.

+    callback.Run(result);
+  }
+
   // We need the filename of the resource to determine the mime type.
   FilePath filename_;
 
@@ -110,6 +138,8 @@
   int resource_id_;
 
   net::HttpResponseInfo response_info_;
+
+  mutable base::WeakPtrFactory<URLRequestResourceBundleJob> weak_factory_;
 };
 
 class GeneratedBackgroundPageJob : public net::URLRequestSimpleJob {
@@ -125,9 +155,10 @@
   }
 
   // Overridden from URLRequestSimpleJob:
-  virtual bool GetData(std::string* mime_type,
-                       std::string* charset,
-                       std::string* data) const OVERRIDE {
+  virtual int GetData(std::string* mime_type,
+                      std::string* charset,
+                      std::string* data,
+                      const net::CompletionCallback& callback) const OVERRIDE {
     *mime_type = "text/html";
     *charset = "utf-8";
 
@@ -138,7 +169,7 @@
       *data += "\"></script>\n";
     }
 
-    return true;
+    return net::OK;
   }
 
   virtual void GetResponseInfo(net::HttpResponseInfo* info) {
@@ -152,13 +183,25 @@
   net::HttpResponseInfo response_info_;
 };
 
+void ReadResourceFilePath(const ExtensionResource& resource,
+                          FilePath* file_path) {
+  *file_path = resource.GetFilePath();
+}
+
 class URLRequestExtensionJob : public net::URLRequestFileJob {
  public:
   URLRequestExtensionJob(net::URLRequest* request,
-                         const FilePath& filename,
+                         const std::string& extension_id,
+                         const FilePath& directory_path,
                          const std::string& content_security_policy,
                          bool send_cors_header)
-    : net::URLRequestFileJob(request, filename) {
+    : net::URLRequestFileJob(request, FilePath()),
+      // TODO(tc): Move all of these files into resources.pak so we don't break
+      // when updating on Linux.
+      resource_(extension_id, directory_path,
+                extension_file_util::ExtensionURLToRelativeFilePath(
+                    request->url())),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
       response_info_.headers = BuildHttpHeaders(content_security_policy,
                                                 send_cors_header);
   }
@@ -167,10 +210,30 @@
     *info = response_info_;
   }
 
+  virtual void Start() OVERRIDE {
+    FilePath* read_file_path = new FilePath;
+    bool posted = base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&ReadResourceFilePath, resource_,
+                   base::Unretained(read_file_path)),
+        base::Bind(&URLRequestExtensionJob::OnFilePathRead,
+                   weak_factory_.GetWeakPtr(),
+                   base::Owned(read_file_path)),
+        true /* task is slow */);
+    DCHECK(posted);
+  }
+
  private:
   virtual ~URLRequestExtensionJob() {}
 
+  void OnFilePathRead(FilePath* read_file_path) {
+    file_path_ = *read_file_path;
+    URLRequestFileJob::Start();
+  }
+
   net::HttpResponseInfo response_info_;
+  ExtensionResource resource_;
+  base::WeakPtrFactory<URLRequestExtensionJob> weak_factory_;
 };
 
 bool ExtensionCanLoadInIncognito(const ResourceRequestInfo* info,
@@ -322,20 +385,8 @@
       }
     }
   }
-  // TODO(tc): Move all of these files into resources.pak so we don't break
-  // when updating on Linux.
-  ExtensionResource resource(extension_id, directory_path,
-      extension_file_util::ExtensionURLToRelativeFilePath(request->url()));
 
-  FilePath resource_file_path;
-  {
-    // Getting the file path will touch the file system.  Fixing
-    // crbug.com/59849 would also fix this.  Suppress the error for now.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    resource_file_path = resource.GetFilePath();
-  }
-
-  return new URLRequestExtensionJob(request, resource_file_path,
+  return new URLRequestExtensionJob(request, extension_id, directory_path,
                                     content_security_policy, send_cors_header);
 }