[Android WebView] Implement a better OnReceivedError callback

[Android WebView] Implement a better OnReceivedError callback

The new callback (aptly named OnReceivedError2) is also called for
subresource load failures and provides more information about
the failed request.

Note that calling OnReceivedError2 is not yet implemented for requests
blocked due to security considerations by Blink. This will be
implemented separately.

BUG=456782
Committed: https://crrev.com/a83c3cafa72c29051cbcf2b5a26a33d488559547
Cr-Commit-Position: refs/heads/master@{#320923}

[mnaganov (inactive), 2015-03-13 17:00:19]

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

[mnaganov (inactive), 2015-03-16 20:37:50]

On 2015/03/13 17:00:20, mnaganov (cr) wrote:

Ping!

[sgurun-gerrit only, 2015-03-16 20:54:11]

On 2015/03/16 20:37:50, mnaganov (cr) wrote:
> On 2015/03/13 17:00:20, mnaganov (cr) wrote:
> 
> Ping!

sorry, just reviewing right now.

[sgurun-gerrit only, 2015-03-17 00:29:51]

On 2015/03/16 20:54:11, sgurun wrote:
> On 2015/03/16 20:37:50, mnaganov (cr) wrote:
> > On 2015/03/13 17:00:20, mnaganov (cr) wrote:
> > 
> > Ping!
> 
> sorry, just reviewing right now.

I have mostly questions on it. lgtm though.

[sgurun-gerrit only, 2015-03-17 00:30:04]

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:249:
if (!request_info) {
curious if this can this can actually be null, since
chromeresourcedispatcherhostdelegate does not check for it.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:261:
}
maybe dlog here if there is no io_client, otherwise the callback will silently
be dropped.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:425:
String unreachableWebDataUrl = AwContentsStatics.getUnreachableWebDataUrl();
maybe put a comment in getUnreachableWebDataUrl() that it is called from IO
thread.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:427:
unreachableWebDataUrl != null && unreachableWebDataUrl.equals(request.url);
nit: is the indentation 8 chars here?

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59:
// when WebView receives 204 (No content) reply from the server.
huh, this sentence made me confused because of use or present tense I think. is
onReceivedError supposed to be called when server sends a 204?
consider writing a test case for this condition (i.e. a client still receiving
onpagefinished when server returns a 204).

https://codereview.chromium.org/1001003004/diff/100001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedError2Test.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedError2Test.java:94:
final String badUrl =
"http://man.id.be.really.surprised.if.this.address.existed/a.html";
make this a class member so you don't have to repeat.

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

https://codereview.chromium.org/1001003004/diff/100001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.cc:280:
web_request.jstring_url = ConvertUTF8ToJavaString(env, location.spec());
question: do you really need to overwrite this? when/how is location different
than request->url?

[mnaganov (inactive), 2015-03-17 15:29:09]

Thank you very much, Selim! I really appreciate the questions you asked.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:249:
if (!request_info) {
On 2015/03/17 00:30:04, sgurun wrote:
> curious if this can this can actually be null, since
> chromeresourcedispatcherhostdelegate does not check for it.

Thanks for pointing this out! ChromeResourceDispatcherHostDelegate also checks
for null requests. Changed to match it.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:261:
}
On 2015/03/17 00:30:04, sgurun wrote:
> maybe dlog here if there is no io_client, otherwise the callback will silently
> be dropped.

Done.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:425:
String unreachableWebDataUrl = AwContentsStatics.getUnreachableWebDataUrl();
On 2015/03/17 00:30:04, sgurun wrote:
> maybe put a comment in getUnreachableWebDataUrl() that it is called from IO
> thread. 

Good point. This may cause a race between two calls to
getUnreachableWebDataUrl() on IO and UI threads (as didFinishLoad also uses it),
but as they will be retrieving the same value, in the worst case, they will
retrieve it both simultaneously and update the cache variable one over another,
which should not harm anyone.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:427:
unreachableWebDataUrl != null && unreachableWebDataUrl.equals(request.url);
On 2015/03/17 00:30:04, sgurun wrote:
> nit: is the indentation 8 chars here?

Hm. Yes -- it should be 8 as its a long line wrap, and "boolean " is 8 chars
long.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59:
// when WebView receives 204 (No content) reply from the server.
On 2015/03/17 00:30:04, sgurun wrote:
> huh, this sentence made me confused because of use or present tense I think.
is
> onReceivedError supposed to be called when server sends a 204?
> consider writing a test case for this condition (i.e. a client still receiving
> onpagefinished when server returns a 204). 

I rethought this and change an approach a bit. To make sure that we don't have a
race between posting onReceivedError from AwContents.IoThreadClientImpl and
onPageFinished from here (I encountered some flakiness in tests because of
that), I made IoThreadClientImpl to post both events, except for the case when
the error code is ERR_ABORTED. But as in this case only onPageFinished must be
emitted, and since we only emit it for the main frame, we will catch all the
possible situations here, in WebContentsObserver.didFailLoad.

I actually discovered this fact through a test that opens a popup window which
opens a url that returns 204. But I also added a test that checks this for the
main resource.

https://codereview.chromium.org/1001003004/diff/100001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedError2Test.java
(right):

https://codereview.chromium.org/1001003004/diff/100001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedError2Test.java:94:
final String badUrl =
"http://man.id.be.really.surprised.if.this.address.existed/a.html";
On 2015/03/17 00:30:04, sgurun wrote:
> make this a class member so you don't have to repeat.

Done.

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

https://codereview.chromium.org/1001003004/diff/100001/android_webview/native...
android_webview/native/aw_contents_io_thread_client_impl.cc:280:
web_request.jstring_url = ConvertUTF8ToJavaString(env, location.spec());
On 2015/03/17 00:30:04, sgurun wrote:
> question: do you really need to overwrite this? when/how is location different
> than request->url?

Great question! I've looked up where we are getting 'location', and found this
code (AwRequestInterceptor::MaybeInterceptRequest):

  scoped_ptr<AwWebResourceResponse> aw_web_resource_response =
      QueryForAwWebResourceResponse(request->url(), request);

Which means, it's effectively the same thing :) Thus, I'm removing this argument
throughout the whole chain to avoid confusion.

[mnaganov (inactive), 2015-03-17 15:30:05]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-03-17 15:30:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/1001003004/#ps140001
(title: "Addressed the last comment")

[commit-bot: I haz the power, 2015-03-17 15:30:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001003004/140001

[commit-bot: I haz the power, 2015-03-17 15:57:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-17 15:57:39]

Patchset 8 (id:??) landed as
https://crrev.com/a83c3cafa72c29051cbcf2b5a26a33d488559547
Cr-Commit-Position: refs/heads/master@{#320923}