[android_webview] Don't intercept resource and asset URLRequests.

[android_webview] Don't intercept resource and asset URLRequests.

WebViewClassic doesn't allow for request intercepting of resource
and asset file:// URLRequests. This change makes AwContents exhibit
the same behavior.

BUG=179411
Touches only android code, builds fine on android trybots.
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186723

[mkosiba (inactive), 2013-03-01 18:12:00]

This CL has two unfortunate consequences:

 1) We have to check whether the resource and assets can be opened in
MaybeCreateJob, that's unfortunate as it may block the IO thread.

 2) Out URLRequestJobFactory setup gets more complicated, I've gone with one
approach and outlined an alternative - interested in your thoughts.

[mkosiba (inactive), 2013-03-01 18:21:00]

https://codereview.chromium.org/12377051/diff/1/android_webview/native/androi...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/native/androi...
android_webview/native/android_protocol_handler.cc:233: if
(!reader_delegate->CanOpenInputStream(AttachCurrentThread(), request)) {
Just thought of the right way to solve this - use a ResourceThrottle to delay
the URLRequest while we perform this check on the file thread and resume the
request when we get the result. I'd probably make it so the throttle creates the
AndroidStreamReaderURLRequestJobDelegateImpl so we won't have to open the
inputstream twice.

[boliu, 2013-03-01 18:44:11]

Man, the quirks of webviewclassic...

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:78:
CreateCookieMonster(url_request_context_.get());
This bit is already fixed in https://codereview.chromium.org/12387028/

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:165: // Alternative
approach:
I like this alternative approach better, because it's more explicit than setting
up a chain of JobFactories.

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/f...
File android_webview/browser/net/fallback_protocol_handler.h (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/f...
android_webview/browser/net/fallback_protocol_handler.h:23: // default priority
and <F2>
<F2>?

[mnaganov (inactive), 2013-03-04 09:53:44]

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:165: // Alternative
approach:
On 2013/03/01 18:44:11, boliu wrote:
> I like this alternative approach better, because it's more explicit than
setting
> up a chain of JobFactories.

But on the other hand, it's more consistent with the rest of Chromium code. See
the monstrous https://chromiumcodereview.appspot.com/11308362

Also, we don't need FallbackProtocolHandler in this case. Right, Marcin?

I'm just not sure, if we really need a function for initializing this. Perhaps,
having the loop inlined here will look simpler.

[boliu, 2013-03-04 19:55:46]

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:165: // Alternative
approach:
On 2013/03/04 09:53:44, Mikhail Naganov (Chromium) wrote:
> On 2013/03/01 18:44:11, boliu wrote:
> > I like this alternative approach better, because it's more explicit than
> setting
> > up a chain of JobFactories.
> 
> But on the other hand, it's more consistent with the rest of Chromium code.
See
> the monstrous https://chromiumcodereview.appspot.com/11308362
> 
> Also, we don't need FallbackProtocolHandler in this case. Right, Marcin?
> 
> I'm just not sure, if we really need a function for initializing this.
Perhaps,
> having the loop inlined here will look simpler.

I don't feel very strongly about this. Seems too minor to worry about.

There is no example in chrome of chaining ProtocolInterceptJobFactory *at one
spot*. They do use this flexibility for initializing AppRequestContext, but
that's only one layer and it's conditional upon if it's an app context. We've
got 3 static ones, so maybe we have the freedom to do this differently.

Perhaps we should just load a bunch of urls and see which way is faster :p

[mkosiba (inactive), 2013-03-05 16:24:56]

On 2013/03/04 19:55:46, boliu wrote:
>
https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
> File android_webview/browser/net/aw_url_request_context_getter.cc (right):
> 
>
https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
> android_webview/browser/net/aw_url_request_context_getter.cc:165: //
Alternative
> approach:
> On 2013/03/04 09:53:44, Mikhail Naganov (Chromium) wrote:
> > On 2013/03/01 18:44:11, boliu wrote:
> > > I like this alternative approach better, because it's more explicit than
> > setting
> > > up a chain of JobFactories.
> > 
> > But on the other hand, it's more consistent with the rest of Chromium code.
> See
> > the monstrous https://chromiumcodereview.appspot.com/11308362
> > 
> > Also, we don't need FallbackProtocolHandler in this case. Right, Marcin?
> > 
> > I'm just not sure, if we really need a function for initializing this.
> Perhaps,
> > having the loop inlined here will look simpler.
> 
> I don't feel very strongly about this. Seems too minor to worry about.

In that case I think I'll side with Mikhail. The ProtocolInterceptJobFactory
already
introduces a concept of ordered handler execution and we already piggy-back on
that and 
I think the comments I've added make this sufficiently clear.

[mkosiba (inactive), 2013-03-05 16:34:43]

Ok. New version is out for review. The two changes from the previous iteration
are:

1) the AndroidStreamReaderURLRequestJob opens the stream on the blocking pool.
This should reduce IO thread jank

2) instead of trying to somehow get the InputStream before we try and resolve a
Job for the request simply try going with the default and restart on failure. We
mark the request as failed so we don't try and restart with the same job. I like
this much more since it doesn't introduce latency for the jobs that will be
handled successfully.

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:78:
CreateCookieMonster(url_request_context_.get());
On 2013/03/01 18:44:11, boliu wrote:
> This bit is already fixed in https://codereview.chromium.org/12387028/

Ok, looks like I need to rebase.

https://codereview.chromium.org/12377051/diff/1/android_webview/native/androi...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/12377051/diff/1/android_webview/native/androi...
android_webview/native/android_protocol_handler.cc:233: if
(!reader_delegate->CanOpenInputStream(AttachCurrentThread(), request)) {
On 2013/03/01 18:21:00, Martin Kosiba wrote:
> Just thought of the right way to solve this - use a ResourceThrottle to delay
> the URLRequest while we perform this check on the file thread and resume the
> request when we get the result. I'd probably make it so the throttle creates
the
> AndroidStreamReaderURLRequestJobDelegateImpl so we won't have to open the
> inputstream twice.

and turns out doing this is a major pain. opted to simply restart the request on
failure.

[boliu, 2013-03-06 02:30:18]

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:126:
base::Passed(&delegate_),
How bad is it to make delegate RefCountedThreadSafe..? This passing ownership
around feels a bit too weird

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.h:10: #include
"base/memory/ref_counted.h"
How did scoped_refptr ever compile without this..?

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:78:
DCHECK(false);
NOTREACHED?

https://codereview.chromium.org/12377051/diff/9001/android_webview/native/and...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/native/and...
android_webview/native/android_protocol_handler.cc:163:
MarkRequestAsFailed(request);
Should put a DCHECK(!HasRequestPreviouslyFailed(request)) before? I'm worried
about someone inadvertently doing something stupid and making this an infinite
loop on failure case.

[mnaganov (inactive), 2013-03-06 09:26:47]

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:75: struct
AndroidStreamReaderURLRequestJob::OpenInputStreamResult {
Is this about C++ or Java unittests? If C++, perhaps it would be better to make
the test to have an IO thread? See for example WebContentsAudioInputStreamTest.

[mkosiba (inactive), 2013-03-06 19:05:27]

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:75: struct
AndroidStreamReaderURLRequestJob::OpenInputStreamResult {
On 2013/03/06 09:26:47, Mikhail Naganov (Chromium) wrote:
> Is this about C++ or Java unittests? If C++, perhaps it would be better to
make
> the test to have an IO thread? See for example
WebContentsAudioInputStreamTest.

C++ unittests. So it looks all of the remaining job implementations are
thread-agnostic, so I'd prefer to keep it like that. I'll pass in a
MessageLoopProxy instead of using the struct.

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.cc:126:
base::Passed(&delegate_),
On 2013/03/06 02:30:18, boliu wrote:
> How bad is it to make delegate RefCountedThreadSafe..? This passing ownership
> around feels a bit too weird

but the delegate _isn't_ thread safe. I actually like passing the ownership as
it makes it impossible for the Job to access the delegate at the wrong time
without having to rely on any mutexes or anything.
I'll expand the comment to include this additional bit of information.

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job.h:10: #include
"base/memory/ref_counted.h"
On 2013/03/06 02:30:18, boliu wrote:
> How did scoped_refptr ever compile without this..?

by being included from something else (most likely
net/url_request/url_request_job.h)

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/browser/ne...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:78:
DCHECK(false);
On 2013/03/06 02:30:18, boliu wrote:
> NOTREACHED?

actually should be a no-op since I'm adding a test that will cause this to be
called in a different CL.

https://codereview.chromium.org/12377051/diff/9001/android_webview/native/and...
File android_webview/native/android_protocol_handler.cc (right):

https://codereview.chromium.org/12377051/diff/9001/android_webview/native/and...
android_webview/native/android_protocol_handler.cc:163:
MarkRequestAsFailed(request);
On 2013/03/06 02:30:18, boliu wrote:
> Should put a DCHECK(!HasRequestPreviouslyFailed(request)) before? I'm worried
> about someone inadvertently doing something stupid and making this an infinite
> loop on failure case.

good idea!

[mnaganov (inactive), 2013-03-07 10:33:03]

lgtm

[commit-bot: I haz the power, 2013-03-07 11:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/12377051/25001

[commit-bot: I haz the power, 2013-03-07 13:20:39]

Failed to apply patch for
android_webview/browser/net/aw_url_request_context_getter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file android_webview/browser/net/aw_url_request_context_getter.cc
  Hunk #2 FAILED at 125.
  1 out of 2 hunks FAILED -- saving rejects to file
android_webview/browser/net/aw_url_request_context_getter.cc.rej

Patch:       android_webview/browser/net/aw_url_request_context_getter.cc
Index: android_webview/browser/net/aw_url_request_context_getter.cc
diff --git a/android_webview/browser/net/aw_url_request_context_getter.cc
b/android_webview/browser/net/aw_url_request_context_getter.cc
index
1b7177b641a846cb6198c2bd460a835ab24a48ec..b762c3080a45be5af97cbbf51bbc768e7f2d6b24
100644
--- a/android_webview/browser/net/aw_url_request_context_getter.cc
+++ b/android_webview/browser/net/aw_url_request_context_getter.cc
@@ -4,6 +4,8 @@
 
 #include "android_webview/browser/net/aw_url_request_context_getter.h"
 
+#include <vector>
+
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_request_interceptor.h"
 #include "android_webview/browser/net/aw_network_delegate.h"
@@ -123,18 +125,41 @@ net::URLRequestContext*
AwURLRequestContextGetter::GetURLRequestContext() {
         chrome::kChromeDevToolsScheme,
         chrome_devtools_protocol_handler_.release());
     DCHECK(set_protocol);
-    // Create a chain of URLRequestJobFactories.  Keep |job_factory_| pointed
-    // at the beginning of the chain.
-    job_factory_ = CreateAndroidJobFactory(job_factory.Pass());
-    job_factory_.reset(new net::ProtocolInterceptJobFactory(
-        job_factory_.Pass(),
-        scoped_ptr<net::URLRequestJobFactory::ProtocolHandler>(
-            new AwRequestInterceptor())));
-    job_factory_.reset(new net::ProtocolInterceptJobFactory(
-        job_factory_.Pass(),
-        developer_protocol_handler_.Pass()));
+
+    // Create a chain of URLRequestJobFactories. The handlers will be invoked
+    // in the order in which they appear in the protocol_handlers vector.
+    typedef std::vector<net::URLRequestJobFactory::ProtocolHandler*>
+        ProtocolHandlerVector;
+    ProtocolHandlerVector protocol_handlers;
+
+    // Note that even though the content:// scheme handler is created here,
+    // it cannot be used by child processes until access to it is granted via
+    // ChildProcessSecurityPolicy::GrantScheme(). This is done in
+    // AwContentBrowserClient.
+    protocol_handlers.push_back(
+        CreateAndroidContentProtocolHandler().release());
+    protocol_handlers.push_back(
+        CreateAndroidAssetFileProtocolHandler().release());
+    protocol_handlers.push_back(developer_protocol_handler_.release());
+    // The AwRequestInterceptor must come after the content and asset file job
+    // factories. This for WebViewClassic compatibility where it was not
+    // possible to intercept resource loads to resolvable content:// and
+    // file:// URIs.
+    protocol_handlers.push_back(new AwRequestInterceptor());
+    job_factory_ = job_factory.PassAs<net::URLRequestJobFactory>();
+
+    // The chain of responsibility will execute the handlers in reverse to the
+    // order in which the elements of the chain are created.
+    for (ProtocolHandlerVector::reverse_iterator i =
protocol_handlers.rbegin();
+         i != protocol_handlers.rend();
+         ++i) {
+      job_factory_.reset(new net::ProtocolInterceptJobFactory(
+          job_factory_.Pass(), make_scoped_ptr(*i)));
+    }
+
     url_request_context_->set_job_factory(job_factory_.get());
   }
+
   return url_request_context_.get();
 }

[commit-bot: I haz the power, 2013-03-07 16:13:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/12377051/40001

[commit-bot: I haz the power, 2013-03-07 16:14:49]

Change committed as 186723