[Android] Fix handling of asset files loading.

chrome/browser/android/android_protocol_adapter.cc

Index: chrome/browser/android/android_protocol_adapter.cc
diff --git a/chrome/browser/android/android_protocol_adapter.cc b/chrome/browser/android/android_protocol_adapter.cc
index aea709200073d5f5cd0a09c3e0502cf58347cf01..d11f793b7d2d4084718faac55e951039a0257e31 100644
--- a/chrome/browser/android/android_protocol_adapter.cc
+++ b/chrome/browser/android/android_protocol_adapter.cc
@@ -12,6 +12,7 @@
 #include "base/string_util.h"
 #include "chrome/browser/android/android_stream_reader_url_request_job.h"
 #include "chrome/common/url_constants.h"
+#include "content/public/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
 #include "jni/AndroidProtocolAdapter_jni.h"
 #include "net/base/io_buffer.h"
@@ -19,8 +20,9 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/http/http_util.h"
-#include "net/url_request/url_request_error_job.h"
-#include "net/url_request/url_request_file_job.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_job_factory.h"
 #include "net/url_request/url_request_job_manager.h"
 
 using base::android::AttachCurrentThread;
@@ -64,6 +66,22 @@ class AndroidStreamReaderURLRequestJobDelegateImpl
   virtual ~AndroidStreamReaderURLRequestJobDelegateImpl();
 };
 
+class AssetFileProtocolInterceptor :
+      public net::URLRequestJobFactory::Interceptor {
+ public:
+  AssetFileProtocolInterceptor();
+  virtual ~AssetFileProtocolInterceptor() OVERRIDE;
+  virtual net::URLRequestJob* MaybeIntercept(net::URLRequest* request) const OVERRIDE;
+  virtual net::URLRequestJob* MaybeInterceptRedirect(
+      const GURL& location,
+      net::URLRequest* request) const OVERRIDE;
+  virtual net::URLRequestJob* MaybeInterceptResponse(
+      net::URLRequest* request) const OVERRIDE;
+
+ private:
+  const std::string kAssetPrefix;
+  const std::string kResourcePrefix;
+};
 
 } // namespace
 
@@ -74,27 +92,8 @@ static bool InitJNIBindings(JNIEnv* env) {
 
 // static
 net::URLRequestJob* AndroidProtocolAdapter::Factory(
-    net::URLRequest* request,
-    net::NetworkDelegate* network_delegate,
-    const std::string& scheme) {
-  DCHECK(scheme == chrome::kFileScheme ||
-         scheme == chrome::kContentScheme);
-  JNIEnv* env = AttachCurrentThread();
-  DCHECK(env);
-  // If this is a file:// URL we cannot handle, fall back to the default
-  // handler.
-  const std::string& url = request->url().spec();
-  std::string assetPrefix = std::string(chrome::kFileScheme) + "://" +
-      chrome::kAndroidAssetPath;
-  std::string resourcePrefix = std::string(chrome::kFileScheme) + "://" +
-      chrome::kAndroidResourcePath;
-
-  if (scheme == chrome::kFileScheme &&
-      !StartsWithASCII(url, assetPrefix, /*case_sensitive=*/ true) &&
-      !StartsWithASCII(url, resourcePrefix,  /*case_sensitive=*/ true)) {
-    return net::URLRequestFileJob::Factory(request, network_delegate, scheme);
-  }
-
+    net::URLRequest* request, const std::string& scheme) {
+  DCHECK(scheme == chrome::kContentScheme);
   return new AndroidStreamReaderURLRequestJob(
       request,
       network_delegate,

[joth, 2012/08/24 17:28:28]

did you mean this? Looks like a compile error.

[mnaganov (inactive), 2012/08/24 18:17:22]

The upstream android_stream_reader_url_request_job.cc still has 3 arguments.

[joth, 2012/08/24 23:22:27]

OK you're passing |network_delegate| as param 2, but where is that variable
declared? It used to be passed in as a param to this
AndroidProtocolAdapter::Factory() function (old linke 78) but you have deleted
that.

@@ -102,25 +101,37 @@ net::URLRequestJob* AndroidProtocolAdapter::Factory(
           new AndroidStreamReaderURLRequestJobDelegateImpl()));
 }
 
+static void AddFileSchemeInterceptorOnIOThread(
+    net::URLRequestContextGetter* context_getter) {
+  // The job factory takes ownership of the interceptor.
+  const_cast<net::URLRequestJobFactory*>(
+      context_getter->GetURLRequestContext()->job_factory())->AddInterceptor(
+          new AssetFileProtocolInterceptor());
+}
+
 // static
-bool AndroidProtocolAdapter::RegisterProtocols(JNIEnv* env) {
+void AndroidProtocolAdapter::RegisterProtocols(
+    JNIEnv* env, net::URLRequestContextGetter* context) {

[joth, 2012/08/24 17:28:28]

nit: context => context_getter.

[mnaganov (inactive), 2012/08/24 18:17:22]

Done.

   DCHECK(env);
+  InitJNIBindings(env);
 
-  if (!InitJNIBindings(env))
-    return false;
-
-  // Register content:// and file://. Note that even though a scheme is
+  // Register content://. Note that even though a scheme is
   // registered here, it cannot be used by child processes until access to it is
   // granted via ChildProcessSecurityPolicy::GrantScheme(). This is done in
   // RenderViewHost.
+  //
+  // TODO(mnaganov): Convert into a ProtocolHandler.
   net::URLRequestJobManager* job_manager =
       net::URLRequestJobManager::GetInstance();
   job_manager->RegisterProtocolFactory(chrome::kContentScheme,
       &AndroidProtocolAdapter::Factory);
-  job_manager->RegisterProtocolFactory(
-      chrome::kFileScheme, &AndroidProtocolAdapter::Factory);
 
-  return true;
+  // Register a file: scheme interceptor for application assets.
+  content::BrowserThread::PostTask(

[joth, 2012/08/24 17:28:28]

I think the modern style for this is:

context_getter->GetNetworkTaskRunner()->PostTask(FROM_HERE, Bind(...))

(you can remove the  #include browser_thread.h then)

[mnaganov (inactive), 2012/08/24 18:17:22]

OK, changed.

+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&AddFileSchemeInterceptorOnIOThread,
+                 base::Unretained(context)));

[joth, 2012/08/24 17:28:28]

I believe the Unretained() shouldn't be needed as URLRequestContextGetter is
ref-counted, and indeed could cause use-after-free error.

[mnaganov (inactive), 2012/08/24 18:17:22]

It is ref-counted, but the profile returns a raw pointer to it, so I will need
to make proper wrapping into a reference. I don't think the getter will be freed
during application initialization.

[joth, 2012/08/24 23:22:27]

make_scoped_refptr(foo) seems no more onerous than base::Unretained(foo), and
avoids me having to stop and think will go out of scope (although, I agree,
during startup it's extremely unlikely too).

(and obv change  AddFileSchemeInterceptorOnIOThread to accept scoped_refptr<Foo>
)

+  // TODO(mnaganov): Add an interceptor for the incognito profile?
 }
 
 // Set a context object to be used for resolving resource queries. This can
@@ -225,3 +236,42 @@ bool AndroidStreamReaderURLRequestJobDelegateImpl::GetCharset(
   // TODO: We should probably be getting this from the managed side.
   return false;
 }
+
+AssetFileProtocolInterceptor::AssetFileProtocolInterceptor() :
+    kAssetPrefix(std::string(chrome::kFileScheme) +
+                 std::string(content::kStandardSchemeSeparator) +
+                 chrome::kAndroidAssetPath),
+    kResourcePrefix(std::string(chrome::kFileScheme) +
+                    std::string(content::kStandardSchemeSeparator) +
+                    chrome::kAndroidResourcePath) {
+}
+
+AssetFileProtocolInterceptor::~AssetFileProtocolInterceptor() {
+}
+
+net::URLRequestJob* AssetFileProtocolInterceptor::MaybeIntercept(
+    net::URLRequest* request) const {
+  if (!request->url().SchemeIsFile()) return NULL;
+
+  const std::string& url = request->url().spec();
+  if (!StartsWithASCII(url, kAssetPrefix, /*case_sensitive=*/ true) &&
+      !StartsWithASCII(url, kResourcePrefix,  /*case_sensitive=*/ true)) {
+    return NULL;
+  }
+
+  return new AndroidStreamReaderURLRequestJob(
+      request,
+      scoped_ptr<AndroidStreamReaderURLRequestJob::Delegate>(
+          new AndroidStreamReaderURLRequestJobDelegateImpl()));
+}
+
+net::URLRequestJob* AssetFileProtocolInterceptor::MaybeInterceptRedirect(
+    const GURL& location,
+    net::URLRequest* request) const {
+  return NULL;
+}
+
+net::URLRequestJob* AssetFileProtocolInterceptor::MaybeInterceptResponse(
+    net::URLRequest* request) const {
+  return NULL;
+}