[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

After CL https://codereview.chromium.org/1381003004,
WebContentsObserver::didStopLoading has become the last event fired
when loading is finished.

WebView was using WebContentsObserver::didFinishLoad event
when it is fired for the main frame, but this callback now happens
before WebContentsImpl has updated its internal state. This causes
subtle race issues, e.g. unwanted reloading of the currently
loading page due to UA string update, which caused flakiness of
AwSettingsTest#testUserAgentStringOverrideForHistory test.

However, as didStopLoading is fired in some cases when didFinishLoad
isn't, we still need to use didFinishLoad to gate posting of
onPageFinishedEvent.

BUG=553762
Committed: https://crrev.com/8f1314f6d90ebcc24506cd036e76f98c3881e8ba
Cr-Commit-Position: refs/heads/master@{#359642}

[mnaganov (inactive), 2015-11-12 16:36:28]

Description was changed from

==========
[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

Before this CL, WebView was using WebContentsObserver::didFinishLoad
for the main frame, but this callback happens before WebContentsImpl
has updated its internal state. Thus, from WebView's POV, the page has
been "loaded" before WebContentsImpl thought so, which caused curious
races.

BUG=553762
==========

to

==========
[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

After CL https://codereview.chromium.org/1381003004, 
WebContentsObserver::didStopLoading has become the last event fired
when loading is finished.

WebView was using WebContentsObserver::didFinishLoad event
when it is fired for the main frame, but this callback now happens
before WebContentsImpl has updated its internal state. This causes
subtle race issues, e.g. unwanted reloading of the currently
loading page due to UA string update, which caused flakiness of
AwSettingsTest#testUserAgentStringOverrideForHistory test.

However, as didStopLoading is fired in some cases when didFinishLoad
isn't, we still need to use didFinishLoad to gate posting of
onPageFinishedEvent. 

BUG=553762
==========

[mnaganov (inactive), 2015-11-12 16:36:42]

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

[mnaganov (inactive), 2015-11-12 16:36:59]

Description was changed from

==========
[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

After CL https://codereview.chromium.org/1381003004, 
WebContentsObserver::didStopLoading has become the last event fired
when loading is finished.

WebView was using WebContentsObserver::didFinishLoad event
when it is fired for the main frame, but this callback now happens
before WebContentsImpl has updated its internal state. This causes
subtle race issues, e.g. unwanted reloading of the currently
loading page due to UA string update, which caused flakiness of
AwSettingsTest#testUserAgentStringOverrideForHistory test.

However, as didStopLoading is fired in some cases when didFinishLoad
isn't, we still need to use didFinishLoad to gate posting of
onPageFinishedEvent. 

BUG=553762
==========

to

==========
[Android WebView] Fire onPageFinished from WebContentsObserver::didStopLoading

After CL https://codereview.chromium.org/1381003004, 
WebContentsObserver::didStopLoading has become the last event fired
when loading is finished.

WebView was using WebContentsObserver::didFinishLoad event
when it is fired for the main frame, but this callback now happens
before WebContentsImpl has updated its internal state. This causes
subtle race issues, e.g. unwanted reloading of the currently
loading page due to UA string update, which caused flakiness of
AwSettingsTest#testUserAgentStringOverrideForHistory test.

However, as didStopLoading is fired in some cases when didFinishLoad
isn't, we still need to use didFinishLoad to gate posting of
onPageFinishedEvent. 

BUG=553762
==========

[gsennton, 2015-11-13 02:35:39]

As is made clear by my comments here, I am not familiar with every part of this
code (so I guess my approval is mostly needed for my concern relating to
regressing crbug.com/534758) :)
So I am not clear on all the details but the idea of posting onPageFinished from
didStopLoad when having called didFinishLoad seems correct. lgtm.

I have tried this CL together with https://codereview.chromium.org/1440933004/
and that seems to fix the regression of crbug.com/534758 that your CL would
introduce. (the regression is flaky so I tried the combined patch 20+ times).

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:56:
String currentUrl = awContents != null ? awContents.getUrl() : validatedUrl;
Why was this not the case before https://codereview.chromium.org/1381003004
?

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:63:
if (validatedUrl.length() == 0) validatedUrl = "about:blank";
When does this happen?

https://codereview.chromium.org/1432083004/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java
(right):

https://codereview.chromium.org/1432083004/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:39:
null, mContentsClient);
I guess this is to avoid the whole loadDataWithBaseUrl case, should there be a
comment here about that?

[mnaganov (inactive), 2015-11-13 17:41:37]

Thanks!

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java
(right):

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:56:
String currentUrl = awContents != null ? awContents.getUrl() : validatedUrl;
On 2015/11/13 02:35:39, gsennton wrote:
> Why was this not the case before https://codereview.chromium.org/1381003004
> ?

Because we were only using 'didFinishLoad' before. Now we need to orchestrate
two callbacks, and as it turns out, they receive different urls if the loading
is done via loadDataWithBaseURL, as the comment says.

https://codereview.chromium.org/1432083004/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:63:
if (validatedUrl.length() == 0) validatedUrl = "about:blank";
On 2015/11/13 02:35:39, gsennton wrote:
> When does this happen?

When the client does loadUrl("about:blank"). Discovered via
ClientOnPageFinishedTest#testCalledAfterError.

https://codereview.chromium.org/1432083004/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java
(right):

https://codereview.chromium.org/1432083004/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:39:
null, mContentsClient);
On 2015/11/13 02:35:39, gsennton wrote:
> I guess this is to avoid the whole loadDataWithBaseUrl case, should there be a
> comment here about that?

That's correct. Added a comment.

[mnaganov (inactive), 2015-11-13 17:41:44]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-13 17:41:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsennton@chromium.org
Link to the patchset: https://codereview.chromium.org/1432083004/#ps80001
(title: "Comments addressed")

[commit-bot: I haz the power, 2015-11-13 17:42:00]

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

[mnaganov (inactive), 2015-11-13 17:43:34]

The CQ bit was unchecked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-13 17:46:49]

On 2015/11/13 17:42:00, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1432083004/80001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1432083004/80001

Oh, wait -- I actually realized there is a bit of a problem here --
if we now return 'historyUrl' for loadDataWithBaseURL, it may break
somebody's assumptions. Although, I don't actually see any tests
verifying that, not even in CTS :) But I guess we should preserve
the old behavior still. Let me fix that.

[mnaganov (inactive), 2015-11-13 19:23:07]

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

[mnaganov (inactive), 2015-11-13 19:23:07]

Hi Ted,

Please take a look at the WebContentsObserverProxy change. I have found out that
we used to extract "base url of data url" in didStopLoading before (see a year
old patch https://codereview.chromium.org/68763012/). Now we actually need to do
that both for didFinishLoad and didStopLoading. This will not affect Chrome, as
it doesn't use loadDataWithBaseUrl.

[Ted C, 2015-11-13 19:39:06]

lgtm

[mnaganov (inactive), 2015-11-13 20:45:56]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-13 20:45:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsennton@chromium.org
Link to the patchset: https://codereview.chromium.org/1432083004/#ps80002
(title: "Restore required AwWebContentsObserverTest changes")

[commit-bot: I haz the power, 2015-11-13 20:47:04]

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

[commit-bot: I haz the power, 2015-11-13 21:15:42]

Committed patchset #7 (id:80002)

[commit-bot: I haz the power, 2015-11-13 21:17:11]

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