[Android WebView] Call shouldInterceptRequest on a background thread

[Android WebView] Call shouldInterceptRequest on a background thread

Move the call to WebViewClient.shouldInterceptRequest from IO thread
to a background thread. This is in order to avoid situations when
embedder's code blocks inside shouldInterceptRequest callback, and
this effectively shuts down IPC messages processing.

BUG=533082
R=boliu@chromium.org, mmenke@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/0f79c573df3eeebd91bb056686bbade31a6aa5fd

[mnaganov (inactive), 2015-09-28 22:41:54]

mnaganov@chromium.org changed reviewers:
+ boliu@chromium.org

[mnaganov (inactive), 2015-09-28 22:41:54]

Hi Bo,

Can you please look at the CL? I plan to show it to the network guys after we
agree on the WebView changes.

[boliu, 2015-09-28 23:26:28]

Mostly small-ish comments I think. Overall makes sense to me.

The part I'm not entirely sure I got is code to prevent the restarted request
from going through the whole shouldIntercept flow again.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_contents_io_thread_client.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_contents_io_thread_client.h:75: const
base::Callback<void(scoped_ptr<AwWebResourceResponse>)>
nit: typedef maybe?

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_handler.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:26: if (job_.get()) return
nullptr;
This probably deserves a comment. I think this is meant to be "only create job
once per request", but casual reader is going to wonder why return null when you
already have a job here.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_handler.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCountedThreadSafe<AwRequestHandler> {
Is this refcounted only because of UserDataAdapter? In that case I think it
doesn't need to be thread safe?

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_interceptor.cc:31: AwRequestHandler* handler
= base::UserDataAdapter<AwRequestHandler>::Get(
nit: typedef

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:113:
MaybeStart();
Does this need to check that Start has indeed been called already? We don't want
to start the request ourselves, right?

https://codereview.chromium.org/1350553005/diff/120001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwContentsBackgroundThreadClient.java
(right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContentsBackgroundThreadClient.java:25:
protected AwWebResourceResponse shouldInterceptRequest(String url, boolean
isMainFrame,
private?

And style nit that overloading these two functions is kinda confusing.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.cc:190:
WebResourceRequest(JNIEnv* env, const net::URLRequest* request)
Nothing calls this I think?

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.h:10: #include
"base/android/jni_weak_ref.h"
needed only in .cc I think?

[mnaganov (inactive), 2015-09-29 00:14:06]

Thanks for the comments, Bo!

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_contents_io_thread_client.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_contents_io_thread_client.h:75: const
base::Callback<void(scoped_ptr<AwWebResourceResponse>)>
On 2015/09/28 23:26:28, boliu wrote:
> nit: typedef maybe?

Done.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_handler.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:26: if (job_.get()) return
nullptr;
On 2015/09/28 23:26:28, boliu wrote:
> This probably deserves a comment. I think this is meant to be "only create job
> once per request", but casual reader is going to wonder why return null when
you
> already have a job here.

Done.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_handler.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCountedThreadSafe<AwRequestHandler> {
On 2015/09/28 23:26:28, boliu wrote:
> Is this refcounted only because of UserDataAdapter? In that case I think it
> doesn't need to be thread safe?

Also because it binds QueryForAwWebResourceResponseDone to itself in order to
pass it to ShouldInterceptRequestAsync. But yes, as the callback is only used on
the IO thread, it doesn't need to be "ThreadSafe".

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/aw_request_interceptor.cc:31: AwRequestHandler* handler
= base::UserDataAdapter<AwRequestHandler>::Get(
On 2015/09/28 23:26:28, boliu wrote:
> nit: typedef

Done.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:113:
MaybeStart();
On 2015/09/28 23:26:28, boliu wrote:
> Does this need to check that Start has indeed been called already? We don't
want
> to start the request ourselves, right?

Good point! Also we don't want to start if we have already been killed :)

https://codereview.chromium.org/1350553005/diff/120001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwContentsBackgroundThreadClient.java
(right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContentsBackgroundThreadClient.java:25:
protected AwWebResourceResponse shouldInterceptRequest(String url, boolean
isMainFrame,
On 2015/09/28 23:26:28, boliu wrote:
> private?
> 
> And style nit that overloading these two functions is kinda confusing.

This is how it was in AwContentsIoThreadClient. But I agree, fixed.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.cc (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.cc:190:
WebResourceRequest(JNIEnv* env, const net::URLRequest* request)
On 2015/09/28 23:26:28, boliu wrote:
> Nothing calls this I think?

AwContentsIoThreadClientImpl::OnReceivedError and
AwContentsIoThreadClientImpl::OnReceivedHttpError. I wouldn't be adding it
otherwise.

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.h (right):

https://codereview.chromium.org/1350553005/diff/120001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.h:10: #include
"base/android/jni_weak_ref.h"
On 2015/09/28 23:26:28, boliu wrote:
> needed only in .cc I think?

Yeah, and it's already there :)

[mnaganov (inactive), 2015-09-29 00:16:42]

mnaganov@chromium.org changed reviewers:
+ mmenke@chromium.org

[mnaganov (inactive), 2015-09-29 00:16:42]

Hi Matt,

Can you please look at aw_request_interceptor*, aw_request_handler*, and
android_stream_reader_url_request_job* to make sure that we don't miss anything
when interacting with the network layer? This is the implementation of the
solution you suggested for crbug.com/533082.

[mmenke, 2015-09-29 15:56:06]

Just looked at the browser/ files.  Only real concerns are with ref counting,
though also suggested some small cleanups.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_handler.cc (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:20: }
nit:  Definition order should match declaration order.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:29: if (job_.get()) return
nullptr;
This should be split on to two lines.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:29: if (job_.get()) return
nullptr;
nit:  .get() not needed.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:33: job_ = new
AndroidStreamReaderURLRequestJob(request, network_delegate);
We're planning on getting rid of refcounting for URLRequestJobs, because
refcounting is, in general, a really bad idea.  Please don't hold onto a
reference to the job.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:67: if (!job_.get()) return;
job_ can't be NULL here - this should really just be a DCHECK.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:72: response.Pass());
optional:  This seems a bit roundabout.  We're calling directly into the job
everywhere else.  Seems to make more sense to just pass the response to the job
directly, and have it create the StreamReaderJobDelegateImpl itself (Could even
make StreamReaderJobDelegateImpl public, and have it call the constructor
directly, rather than a static method create it, though I haven't dug into the
reason for the extra method).

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:74:
job_->DeliverResponseFromNetwork();
BUG:  The request could be cancelled by this point.  You should probably just
use a weak pointer in your bind call.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_handler.h (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCounted<AwRequestHandler> {
This class really needs some documentation, as do its methods.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCounted<AwRequestHandler> {
Refcounting this seems like a bad idea, that's just going to end in bugs and
tears.  We really don't want this outliving a cancelled request.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:33: ~AwRequestHandler();
+override

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_interceptor.h (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_interceptor.h:22: class AwRequestInterceptor
: public net::URLRequestInterceptor {
Should this and the handler be in browser/net?  That's certainly where they'd be
if they were in chrome/browser, and they are implementing net/ interfaces.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:112:
DCHECK(delegate_);
optional:  Think DCHECK(delegate); one or two lines up is clearer.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:146: if
(!has_been_started() || has_been_killed() || !delegate_) return;
Move return to next line.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:146: if
(!has_been_started() || has_been_killed() || !delegate_) return;
has_been_killed() won't be true here - can DCHECK on it if you want, but testing
it seems weird.

[mnaganov (inactive), 2015-09-29 23:45:36]

Thanks for awesome comments, Matt! I think now the code should look much simpler
and hopefully more correct. Please take another look!

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_handler.cc (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:20: }
On 2015/09/29 15:56:05, mmenke wrote:
> nit:  Definition order should match declaration order.

Not relevant anymore.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:29: if (job_.get()) return
nullptr;
On 2015/09/29 15:56:05, mmenke wrote:
> nit:  .get() not needed.

Done.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:29: if (job_.get()) return
nullptr;
On 2015/09/29 15:56:05, mmenke wrote:
> This should be split on to two lines.

Not relevant anymore.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:33: job_ = new
AndroidStreamReaderURLRequestJob(request, network_delegate);
On 2015/09/29 15:56:05, mmenke wrote:
> We're planning on getting rid of refcounting for URLRequestJobs, because
> refcounting is, in general, a really bad idea.  Please don't hold onto a
> reference to the job.

Done. Now only a weak reference to the job is held.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:67: if (!job_.get()) return;
On 2015/09/29 15:56:05, mmenke wrote:
> job_ can't be NULL here - this should really just be a DCHECK.

Not relevant anymore.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:72: response.Pass());
On 2015/09/29 15:56:05, mmenke wrote:
> optional:  This seems a bit roundabout.  We're calling directly into the job
> everywhere else.  Seems to make more sense to just pass the response to the
job
> directly, and have it create the StreamReaderJobDelegateImpl itself (Could
even
> make StreamReaderJobDelegateImpl public, and have it call the constructor
> directly, rather than a static method create it, though I haven't dug into the
> reason for the extra method).

Right. Perhaps there have been historical reasons, but it looks like now we can
change the code to avoid that. Thanks for the suggestion!

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.cc:74:
job_->DeliverResponseFromNetwork();
On 2015/09/29 15:56:05, mmenke wrote:
> BUG:  The request could be cancelled by this point.  You should probably just
> use a weak pointer in your bind call.

Done.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_handler.h (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCounted<AwRequestHandler> {
On 2015/09/29 15:56:05, mmenke wrote:
> This class really needs some documentation, as do its methods.

It seems that we actually can do without it, removed :)

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:22: class AwRequestHandler : public
base::RefCounted<AwRequestHandler> {
On 2015/09/29 15:56:05, mmenke wrote:
> Refcounting this seems like a bad idea, that's just going to end in bugs and
> tears.  We really don't want this outliving a cancelled request.

Not relevant anymore.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_handler.h:33: ~AwRequestHandler();
On 2015/09/29 15:56:05, mmenke wrote:
> +override

Not relevant anymore.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/aw_request_interceptor.h (right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/aw_request_interceptor.h:22: class AwRequestInterceptor
: public net::URLRequestInterceptor {
On 2015/09/29 15:56:05, mmenke wrote:
> Should this and the handler be in browser/net?  That's certainly where they'd
be
> if they were in chrome/browser, and they are implementing net/ interfaces.

Yup--great suggestion, moved!

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:112:
DCHECK(delegate_);
On 2015/09/29 15:56:06, mmenke wrote:
> optional:  Think DCHECK(delegate); one or two lines up is clearer.

Done.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:146: if
(!has_been_started() || has_been_killed() || !delegate_) return;
On 2015/09/29 15:56:06, mmenke wrote:
> has_been_killed() won't be true here - can DCHECK on it if you want, but
testing
> it seems weird.

Done.

https://codereview.chromium.org/1350553005/diff/140001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:146: if
(!has_been_started() || has_been_killed() || !delegate_) return;
On 2015/09/29 15:56:06, mmenke wrote:
> Move return to next line.

Done.

[boliu, 2015-09-30 00:14:47]

Possible to avoid moving *existing* files? Or maybe could tweak git cl upload
--similarity=

Can't really review changing in moved files that are not interpreted by code
review as move.

[mnaganov (inactive), 2015-09-30 00:23:50]

On 2015/09/30 00:14:47, boliu wrote:
> Possible to avoid moving *existing* files? Or maybe could tweak git cl upload
> --similarity=
> 
> Can't really review changing in moved files that are not interpreted by code
> review as move.

Good point. Please look at PS 13. There is now a false positive with "renaming"
of aw_contents_background_thread_client*, but it's quite trivial.

[boliu, 2015-09-30 00:43:20]

lgtm

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (left):

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:67: // See if we've
already found out the aw_web_resource_response for this
keep this comment?

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
File android_webview/browser/net/aw_web_resource_response.h (right):

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
android_webview/browser/net/aw_web_resource_response.h:21: class
AndroidStreamReaderURLRequestJob;
not needed I think

[mnaganov (inactive), 2015-09-30 01:01:50]

Hmmm... Now I'm getting a DCHECK failure because
AndroidStreamReaderURLRequestJob::Start() is getting called twice in a row, when
I'm running other WebView tests.

Will look into that tomorrow. Matt, perhaps you may notice any obvious problem
that can cause that.

I/DEBUG   (21598):     #05 pc 0005f685 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::debug::BreakDebugger()+24)
I/DEBUG   (21598):     #06 pc 00072d47 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(logging::LogMessage::~LogMessage()+570)
I/DEBUG   (21598):     #07 pc 0007a04f 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libstandalonelibwebviewchromium.cr.so
I/DEBUG   (21598):     #08 pc 0027c7c7 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libnet.cr.so
(net::URLRequest::StartJob(net::URLRequestJob*)+598)
I/DEBUG   (21598):     #09 pc 0027c535 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libnet.cr.so
(net::URLRequest::BeforeRequestComplete(int)+400)
I/DEBUG   (21598):     #10 pc 0027c283 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libnet.cr.so
(net::URLRequest::Start()+290)
I/DEBUG   (21598):     #11 pc 0048d4e9 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceLoader::StartRequestInternal()+156)
I/DEBUG   (21598):     #12 pc 0048d429 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceLoader::StartRequest()+184)
I/DEBUG   (21598):     #13 pc 0048bcc3 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceDispatcherHostImpl::StartLoading(content::ResourceRequestInfoImpl*,
linked_ptr<content::ResourceLoader> const&)+82)
I/DEBUG   (21598):     #14 pc 00487845 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceDispatcherHostImpl::BeginRequestInternal(scoped_ptr<net::URLRequest,
base::DefaultDeleter<net::URLRequest> >, scoped_ptr<content::ResourceHandler,
base::DefaultDeleter<content::ResourceHandler> >)+652)
I/DEBUG   (21598):     #15 pc 0048a0a3 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceDispatcherHostImpl::BeginRequest(int, ResourceHostMsg_Request
const&, IPC::Message*, int)+2870)
I/DEBUG   (21598):     #16 pc 004891bb 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceDispatcherHostImpl::OnRequestResource(int, int,
ResourceHostMsg_Request const&)+190)
I/DEBUG   (21598):     #17 pc 004890e9 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
I/DEBUG   (21598):     #18 pc 00488ef7 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::ResourceDispatcherHostImpl::OnMessageReceived(IPC::Message const&,
content::ResourceMessageFilter*)+306)
I/DEBUG   (21598):     #19 pc 0031ab0b 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
I/DEBUG   (21598):     #20 pc 0031a9db 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
I/DEBUG   (21598):     #21 pc 0001ba43 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
I/DEBUG   (21598):     #22 pc 0001ba23 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
I/DEBUG   (21598):     #23 pc 0001297f 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::ChannelProxy::Context::TryFilters(IPC::Message const&)+102)
I/DEBUG   (21598):     #24 pc 00012abf 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::ChannelProxy::Context::OnMessageReceived(IPC::Message const&)+6)
I/DEBUG   (21598):     #25 pc 000149e1 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::internal::ChannelReader::DispatchMessage(IPC::Message*)+108)
I/DEBUG   (21598):     #26 pc 00014735 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::internal::ChannelReader::TranslateInputData(char const*, int)+444)
I/DEBUG   (21598):     #27 pc 0001454d 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::internal::ChannelReader::ProcessIncomingMessages()+112)
I/DEBUG   (21598):     #28 pc 000101df 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libipc.cr.so
(IPC::ChannelPosix::OnFileCanReadWithoutBlocking(int)+154)
I/DEBUG   (21598):     #29 pc 0004b47b 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking(int,
base::MessagePumpLibevent*)+26)
I/DEBUG   (21598):     #30 pc 0004bc65 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::MessagePumpLibevent::OnLibeventNotification(int, short, void*)+156)
I/DEBUG   (21598):     #31 pc 000c5a51 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
I/DEBUG   (21598):     #32 pc 0004bf9b 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+274)
I/DEBUG   (21598):     #33 pc 000788bd 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::MessageLoop::RunHandler()+116)
I/DEBUG   (21598):     #34 pc 00089fed 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::RunLoop::Run()+30)
I/DEBUG   (21598):     #35 pc 00078263 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::MessageLoop::Run()+58)
I/DEBUG   (21598):     #36 pc 00398d59 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::BrowserThreadImpl::IOThreadRun(base::MessageLoop*)+8)
I/DEBUG   (21598):     #37 pc 00398e21 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libcontent.cr.so
(content::BrowserThreadImpl::Run(base::MessageLoop*)+124)
I/DEBUG   (21598):     #38 pc 000a32cd 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so
(base::Thread::ThreadMain()+288)
I/DEBUG   (21598):     #39 pc 0009f63b 
/data/app/org.chromium.android_webview.shell-1/lib/arm/libbase.cr.so

[mnaganov (inactive), 2015-09-30 18:05:30]

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (left):

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:67: // See if we've
already found out the aw_web_resource_response for this
On 2015/09/30 00:43:20, boliu wrote:
> keep this comment?

Well, it's not valid anymore -- we now always create the job (except for the
case when we failed to find IoThreadClient, etc. I just added a short comment
that MaybeInterceptRequest can be called multiple times for the same request.

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
File android_webview/browser/net/aw_web_resource_response.h (right):

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
android_webview/browser/net/aw_web_resource_response.h:21: class
AndroidStreamReaderURLRequestJob;
On 2015/09/30 00:43:20, boliu wrote:
> not needed I think

Done.

[boliu, 2015-09-30 18:06:19]

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (left):

https://codereview.chromium.org/1350553005/diff/240001/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:67: // See if we've
already found out the aw_web_resource_response for this
On 2015/09/30 18:05:30, mnaganov wrote:
> On 2015/09/30 00:43:20, boliu wrote:
> > keep this comment?
> 
> Well, it's not valid anymore -- we now always create the job (except for the
> case when we failed to find IoThreadClient, etc. I just added a short comment
> that MaybeInterceptRequest can be called multiple times for the same request.

Right, that part :p

[mnaganov (inactive), 2015-09-30 18:09:17]

mmenke@: Matt, please take a look!

I figured out what was the problem -- I forgot that C++ doesn't initialize class
fields of primitive types, so when a job was created on top of a previously
killed job, it has this flag already set *facepalm*. Definitely too much Java
programming rots your mind :)

[mmenke, 2015-09-30 18:41:30]

On 2015/09/30 18:09:17, mnaganov wrote:
> mmenke@: Matt, please take a look!
> 
> I figured out what was the problem -- I forgot that C++ doesn't initialize
class
> fields of primitive types, so when a job was created on top of a previously
> killed job, it has this flag already set *facepalm*. Definitely too much Java
> programming rots your mind :)

Will do - I'll try to get to this today, but may not have time until tomorrow.

[mmenke, 2015-09-30 21:13:34]

android_webview/browser/net/ LGTM, just a couple suggestions.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/android_stream_reader_url_request_job.h:83:
scoped_ptr<Delegate> delegate);
nit:  Should probably have a blank line after constructors.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/android_stream_reader_url_request_job.h:85:
GetWebResourceResponseCallback();
need to include callback.h for this.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/android_stream_reader_url_request_job.h:111: void
MaybeStart();
Should provide some documentation of these.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/android_stream_reader_url_request_job.h:121: bool
has_been_started() { return has_been_started_; }
Private accessor seems a little weird...can just access the variable directly. 
If you prefer to keep it, it should be const.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/android_stream_reader_url_request_job.h:132:
DISALLOW_COPY_AND_ASSIGN(AndroidStreamReaderURLRequestJob);
While you're here, should include base/macros.g for DISALLOW_COPY_AND_ASSIGN.

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://chromiumcodereview.appspot.com/1350553005/diff/300001/android_webview...
android_webview/browser/net/aw_request_interceptor.cc:60: request,
job->GetWebResourceResponseCallback());
Up to you, but I think this makes more sense in
AndroidStreamReaderURLRequestJob::Start() - then you can also get rid of
has_been_started_, merge MaybeStart() with WebResourceResponObtained, and inline
GetWebResourceResponseCallback().

Note that you don't really gain any performance from doing this call sooner,
since we call Start() almost immediately after creating the job, anyways.  I
think the simpler code has fewer potential race conditions to worry about.

[mnaganov (inactive), 2015-10-01 18:04:30]

Thanks, Matt! Awesome suggestions again :)

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:83:
scoped_ptr<Delegate> delegate);
On 2015/09/30 21:13:34, mmenke wrote:
> nit:  Should probably have a blank line after constructors.

Done.

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:85:
GetWebResourceResponseCallback();
On 2015/09/30 21:13:34, mmenke wrote:
> need to include callback.h for this.

Done.

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:111: void
MaybeStart();
On 2015/09/30 21:13:34, mmenke wrote:
> Should provide some documentation of these.

OK, I have changed them a bit according to your proposal, but I've added docs
for the new methods.

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:121: bool
has_been_started() { return has_been_started_; }
On 2015/09/30 21:13:34, mmenke wrote:
> Private accessor seems a little weird...can just access the variable directly.

> If you prefer to keep it, it should be const.

Got rid of it.

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:132:
DISALLOW_COPY_AND_ASSIGN(AndroidStreamReaderURLRequestJob);
On 2015/09/30 21:13:34, mmenke wrote:
> While you're here, should include base/macros.g for DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/300001/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:60: request,
job->GetWebResourceResponseCallback());
On 2015/09/30 21:13:34, mmenke wrote:
> Up to you, but I think this makes more sense in
> AndroidStreamReaderURLRequestJob::Start() - then you can also get rid of
> has_been_started_, merge MaybeStart() with WebResourceResponObtained, and
inline
> GetWebResourceResponseCallback().
> 
> Note that you don't really gain any performance from doing this call sooner,
> since we call Start() almost immediately after creating the job, anyways.  I
> think the simpler code has fewer potential race conditions to worry about.

Thanks for suggestion! I've made that. This also allows
AndroidStreamReaderURLRequestJob not to know about
AwStreamReaderJobDelegateImpl, and instead deal with something that returns a
Delegate asynchronously, I called this DelegateObtainer.

[mmenke, 2015-10-01 18:34:40]

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:81: Callback
callback) = 0;
nit:  const Callback&

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:83: virtual
~DelegateObtainer() {}
nit:  Destructors (And constructors) should go before other methods.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:87:
io_thread_client_->ShouldInterceptRequestAsync(
Is it a problem if io_thread_client_ is deleted while we're waiting for this
call?

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:92:
base::Unretained(this)));
This should be a weak_ptr, no?  It is a bit unfortunate that we need to
double-weakptr this, but the job may just be cancelled (So we need the other
one), or it may be destroyed (So we need this one).

Other option would be to make the callback itself an argument to
WebResourceResponseObtained, and make that method static, and depend on callback
using a weak ptr, though that seems uglier to me.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:152: .Pass(),
nit:  .Pass() isn't needed with make_scoped_ptr, in general, even if you're
passing as a more general scoped_ptr type.

[mmenke, 2015-10-01 18:41:35]

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:92:
base::Unretained(this)));
On 2015/10/01 18:34:40, mmenke wrote:
> This should be a weak_ptr, no?  It is a bit unfortunate that we need to
> double-weakptr this, but the job may just be cancelled (So we need the other
> one), or it may be destroyed (So we need this one).
> 
> Other option would be to make the callback itself an argument to
> WebResourceResponseObtained, and make that method static, and depend on
callback
> using a weak ptr, though that seems uglier to me.

On second thought, if IOThreadClient handles being deleted while we're waiting
on the callback safely, and doesn't invoke it's callback, then we're fined with
Unretained here.

[boliu, 2015-10-01 19:13:56]

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:297:
delegate_obtainer_.reset(nullptr);
nit: just reset()

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:299:
delegate_.swap(delegate);
DCHECK(!delegate_) before swap? I think that's correct..?

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:92:
base::Unretained(this)));
On 2015/10/01 18:41:35, mmenke wrote:
> On 2015/10/01 18:34:40, mmenke wrote:
> > This should be a weak_ptr, no?  It is a bit unfortunate that we need to
> > double-weakptr this, but the job may just be cancelled (So we need the other
> > one), or it may be destroyed (So we need this one).
> > 
> > Other option would be to make the callback itself an argument to
> > WebResourceResponseObtained, and make that method static, and depend on
> callback
> > using a weak ptr, though that seems uglier to me.
> 
> On second thought, if IOThreadClient handles being deleted while we're waiting
> on the callback safely, and doesn't invoke it's callback, then we're fined
with
> Unretained here.

I don't think IOThreadClient does that. So this does need to be a weakptr.

I was checking if request pointer might become dangling too. It's safe because
it's only used inside ShouldInterceptRequestAsync call itself, not in the
callback. Worth a comment somewhere though.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.h (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.h:73:
base::android::ScopedJavaGlobalRef<jobject> bg_thread_client_object_;
Sorry.. just noticed this will prevent AwContents from gc-ed. But I guess it's
ok since this is short-lived, only during the duration of the callback.

Just to be safe, maybe code the java class to only hold a reference to
mContentClient instead of all of AwContents?

Definitely worth a comment though, either way.

[mnaganov (inactive), 2015-10-01 19:51:04]

Really appreciated that you have took another look, guys! I addressed your
comments, planning to try landing this later today. Please shout if you find
anything wrong.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:297:
delegate_obtainer_.reset(nullptr);
On 2015/10/01 19:13:56, boliu wrote:
> nit: just reset()

Done.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:299:
delegate_.swap(delegate);
On 2015/10/01 19:13:56, boliu wrote:
> DCHECK(!delegate_) before swap? I think that's correct..?

Look three lines above :)

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:81: Callback
callback) = 0;
On 2015/10/01 18:34:40, mmenke wrote:
> nit:  const Callback&

Done.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.h:83: virtual
~DelegateObtainer() {}
On 2015/10/01 18:34:40, mmenke wrote:
> nit:  Destructors (And constructors) should go before other methods.

Done.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:87:
io_thread_client_->ShouldInterceptRequestAsync(
On 2015/10/01 18:34:40, mmenke wrote:
> Is it a problem if io_thread_client_ is deleted while we're waiting for this
> call?

ShouldInterceptRequestAdaptor owns io_thread_client_.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:92:
base::Unretained(this)));
On 2015/10/01 18:34:40, mmenke wrote:
> This should be a weak_ptr, no?  It is a bit unfortunate that we need to
> double-weakptr this, but the job may just be cancelled (So we need the other
> one), or it may be destroyed (So we need this one).
> 
> Other option would be to make the callback itself an argument to
> WebResourceResponseObtained, and make that method static, and depend on
callback
> using a weak ptr, though that seems uglier to me.

Right, I have also realized that over the lunch. Using WeakPtr here seems like
the easiest option. Thanks for noticing this!

Re: request -- sure, added a comment.

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:152: .Pass(),
On 2015/10/01 18:34:40, mmenke wrote:
> nit:  .Pass() isn't needed with make_scoped_ptr, in general, even if you're
> passing as a more general scoped_ptr type.

Neat, thanks!

https://codereview.chromium.org/1350553005/diff/330024/android_webview/native...
File android_webview/native/aw_contents_io_thread_client_impl.h (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.h:73:
base::android::ScopedJavaGlobalRef<jobject> bg_thread_client_object_;
On 2015/10/01 19:13:56, boliu wrote:
> Sorry.. just noticed this will prevent AwContents from gc-ed. But I guess it's
> ok since this is short-lived, only during the duration of the callback.
> 
> Just to be safe, maybe code the java class to only hold a reference to
> mContentClient instead of all of AwContents?
> 
> Definitely worth a comment though, either way.

The lifetime and properties of BackgroundThreadClientImpl are absolutely the
same as of IoThreadClientImpl. We already hold a reference to IoThreadClientImpl
Java object here, so adding a reference to BackgroundThreadClientImpl doesn't
hurt.

I agree with your note about only holding mContentsClient rather than full
AwContents, but let's make that in another CL.

[mmenke, 2015-10-01 20:06:53]

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:87:
io_thread_client_->ShouldInterceptRequestAsync(
On 2015/10/01 19:51:04, mnaganov wrote:
> On 2015/10/01 18:34:40, mmenke wrote:
> > Is it a problem if io_thread_client_ is deleted while we're waiting for this
> > call?
> 
> ShouldInterceptRequestAdaptor owns io_thread_client_.

Right, and the Job owns ShouldInterceptRequestAdaptor, so if the job is deleted,
so is the ShouldInterceptRequestAdaptor.  I was just wondering if the client is
happy to be deleted in the middle of a call.

[boliu, 2015-10-01 20:28:37]

lgtm

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
File android_webview/browser/net/aw_request_interceptor.cc (right):

https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
android_webview/browser/net/aw_request_interceptor.cc:87:
io_thread_client_->ShouldInterceptRequestAsync(
On 2015/10/01 20:06:53, mmenke wrote:
> On 2015/10/01 19:51:04, mnaganov wrote:
> > On 2015/10/01 18:34:40, mmenke wrote:
> > > Is it a problem if io_thread_client_ is deleted while we're waiting for
this
> > > call?
> > 
> > ShouldInterceptRequestAdaptor owns io_thread_client_.
> 
> Right, and the Job owns ShouldInterceptRequestAdaptor, so if the job is
deleted,
> so is the ShouldInterceptRequestAdaptor.  I was just wondering if the client
is
> happy to be deleted in the middle of a call.

That case looks fine :)

[mnaganov (inactive), 2015-10-01 20:32:33]

On 2015/10/01 20:06:53, mmenke wrote:
>
https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
> File android_webview/browser/net/aw_request_interceptor.cc (right):
> 
>
https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
> android_webview/browser/net/aw_request_interceptor.cc:87:
> io_thread_client_->ShouldInterceptRequestAsync(
> On 2015/10/01 19:51:04, mnaganov wrote:
> > On 2015/10/01 18:34:40, mmenke wrote:
> > > Is it a problem if io_thread_client_ is deleted while we're waiting for
this
> > > call?
> > 
> > ShouldInterceptRequestAdaptor owns io_thread_client_.
> 
> Right, and the Job owns ShouldInterceptRequestAdaptor, so if the job is
deleted,
> so is the ShouldInterceptRequestAdaptor.  I was just wondering if the client
is
> happy to be deleted in the middle of a call.

Ah, sorry :) I think, it should be OK: the role of AwContentsIoThreadClientImpl
is to be a bridge to the corresponding Java object. The native object holds a
scoped local ref to a Java object. When it posts a task to background thread to
commence a call, it gives the task a weak global ref. Thus, would the adaptor
and the native IoThreadClient object die before the task is executed, the weak
global ref will expire, and the task will just return without trying to reach
the java object. If the task will succeed obtaining a strong reference, it will
be holding it until the call on the background thread finishes, so even if
IoThreadClient object dies during the call, the java object will be still alive,
and then, after returning to C++, the callbacks will just decay due to our usage
of base::weak pointers.

So, fingers crossed, we are doing everything right. :)

[mmenke, 2015-10-01 20:36:20]

On 2015/10/01 20:32:33, mnaganov wrote:
> On 2015/10/01 20:06:53, mmenke wrote:
> >
>
https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
> > File android_webview/browser/net/aw_request_interceptor.cc (right):
> > 
> >
>
https://codereview.chromium.org/1350553005/diff/330024/android_webview/browse...
> > android_webview/browser/net/aw_request_interceptor.cc:87:
> > io_thread_client_->ShouldInterceptRequestAsync(
> > On 2015/10/01 19:51:04, mnaganov wrote:
> > > On 2015/10/01 18:34:40, mmenke wrote:
> > > > Is it a problem if io_thread_client_ is deleted while we're waiting for
> this
> > > > call?
> > > 
> > > ShouldInterceptRequestAdaptor owns io_thread_client_.
> > 
> > Right, and the Job owns ShouldInterceptRequestAdaptor, so if the job is
> deleted,
> > so is the ShouldInterceptRequestAdaptor.  I was just wondering if the client
> is
> > happy to be deleted in the middle of a call.
> 
> Ah, sorry :) I think, it should be OK: the role of
AwContentsIoThreadClientImpl
> is to be a bridge to the corresponding Java object. The native object holds a
> scoped local ref to a Java object. When it posts a task to background thread
to
> commence a call, it gives the task a weak global ref. Thus, would the adaptor
> and the native IoThreadClient object die before the task is executed, the weak
> global ref will expire, and the task will just return without trying to reach
> the java object. If the task will succeed obtaining a strong reference, it
will
> be holding it until the call on the background thread finishes, so even if
> IoThreadClient object dies during the call, the java object will be still
alive,
> and then, after returning to C++, the callbacks will just decay due to our
usage
> of base::weak pointers.
> 
> So, fingers crossed, we are doing everything right. :)

Great!  Still LGTM.

[mnaganov (inactive), 2015-10-01 20:37:22]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-10-01 20:38:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350553005/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350553005/350001

[commit-bot: I haz the power, 2015-10-01 20:49:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 20:49:42]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mnaganov (inactive), 2015-10-01 20:55:26]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-10-01 20:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1350553005/#ps370001
(title: "Re-uploading with standard diff settings to work around Windows patch
issues")

[commit-bot: I haz the power, 2015-10-01 20:56:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350553005/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350553005/370001

[commit-bot: I haz the power, 2015-10-01 21:02:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 21:02:27]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[boliu, 2015-10-01 21:04:01]

On 2015/10/01 21:02:27, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

I think patch failure is just bot flake.. brettw reported it on #chromium. I
think it's being looked into.

Mean time, retry and hope you get lucky..

[mnaganov (inactive), 2015-10-01 21:05:30]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-10-01 21:06:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350553005/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350553005/370001

[mnaganov (inactive), 2015-10-01 21:07:08]

Yeah, discovered that in the logs -- the bots are just failing while trying
to URL-fetch it :( Retrying...

On Thu, Oct 1, 2015 at 2:04 PM, <boliu@chromium.org> wrote:

> On 2015/10/01 21:02:27, commit-bot: I haz the power wrote:
>
>> Try jobs failed on following builders:
>>    linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
>> (JOB_FAILED,
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
>>    win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
> )
>
> I think patch failure is just bot flake.. brettw reported it on #chromium.
> I
> think it's being looked into.
>
> Mean time, retry and hope you get lucky..
>
> https://codereview.chromium.org/1350553005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-10-01 21:07:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 21:07:37]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mnaganov (inactive), 2015-10-01 21:36:05]

On 2015/10/01 21:07:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
>   win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

OK, since all the bots are green except for Windows bots (which we couldn't
possibly break), and we had 'win_chromium_compile_dbg_ng' green for PS 19, I'll
try landing manually.

[boliu, 2015-10-01 21:39:18]

On 2015/10/01 21:36:05, mnaganov wrote:
> On 2015/10/01 21:07:37, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
> >   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
> >   win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> 
> OK, since all the bots are green except for Windows bots (which we couldn't
> possibly break), and we had 'win_chromium_compile_dbg_ng' green for PS 19,
I'll
> try landing manually.

Android-only changes can break windows bots:
https://codereview.chromium.org/1357643002#msg14

But.. not doing any case-sensitive renames here, so probably ok

[commit-bot: I haz the power, 2015-10-01 21:40:48]

Patchset 20 (id:??) landed as
https://crrev.com/0f79c573df3eeebd91bb056686bbade31a6aa5fd
Cr-Commit-Position: refs/heads/master@{#351894}

[mnaganov (inactive), 2015-10-01 21:41:13]

Committed patchset #20 (id:370001) manually as
0f79c573df3eeebd91bb056686bbade31a6aa5fd (presubmit successful).

[mnaganov (inactive), 2015-10-01 21:41:59]

Right, and win_chromium_compile_dbg_ng was green once, so should be OK.

On Thu, Oct 1, 2015 at 2:39 PM, <boliu@chromium.org> wrote:

> On 2015/10/01 21:36:05, mnaganov wrote:
>
>> On 2015/10/01 21:07:37, commit-bot: I haz the power wrote:
>> > Try jobs failed on following builders:
>> >   win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
>> >
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...
> )
>
>> >   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>> >
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...
> )
>
>> >   win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
>> >
>>
>
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
> )
>
> OK, since all the bots are green except for Windows bots (which we couldn't
>> possibly break), and we had 'win_chromium_compile_dbg_ng' green for PS 19,
>>
> I'll
>
>> try landing manually.
>>
>
> Android-only changes can break windows bots:
> https://codereview.chromium.org/1357643002#msg14
>
> But.. not doing any case-sensitive renames here, so probably ok
>
> https://codereview.chromium.org/1350553005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.