[WebView 56] Post onPageFinished for committed Url if nav to error page.

[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG=656919
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2622283002
Cr-Commit-Position: refs/branch-heads/2924@{#747}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed: https://chromium.googlesource.com/chromium/src/+/00b73e7d682c407bf15fc79e667c75fc3112ebd3

[gsennton, 2017-01-11 14:39:47]

gsennton@chromium.org changed reviewers:
+ japhet@chromium.org, sgurun@chromium.org

[gsennton, 2017-01-11 14:39:48]

Hey guys, Selim has the actual commit power here (so ptal :)).

Btw, Selim, should I add the tests related to these changes in this CL as well?
(will they be run on this branch?)

Nate, do you know whether there are any cases where we can fail (receive
didFailLoad) a committed load except for the case in which we cancel the load?
When we cancel a load we don't navigate to an error-page so in that case we post
onPageFinished only from didFailLoad (and the logic I'm adding here won't
trigger).
I just want to make sure there aren't any cases where we both trigger this logic
and the one in didFailLoad/onReceivedError (thus posting onPageFinished several
times) :).

[Nate Chapin, 2017-01-11 18:53:51]

On 2017/01/11 14:39:48, gsennton wrote:
> Hey guys, Selim has the actual commit power here (so ptal :)).
> 
> Btw, Selim, should I add the tests related to these changes in this CL as
well?
> (will they be run on this branch?)
> 
> Nate, do you know whether there are any cases where we can fail (receive
> didFailLoad) a committed load except for the case in which we cancel the load?
> When we cancel a load we don't navigate to an error-page so in that case we
post
> onPageFinished only from didFailLoad (and the logic I'm adding here won't
> trigger).
> I just want to make sure there aren't any cases where we both trigger this
logic
> and the one in didFailLoad/onReceivedError (thus posting onPageFinished
several
> times) :).

I think it's possible but rare for a load to commit then terminate early due to
a problem with the network. I think that's the only possible didFailLoad besides
cancellation.

[gsennton, 2017-01-11 19:36:34]

On 2017/01/11 18:53:51, Nate Chapin wrote:
> On 2017/01/11 14:39:48, gsennton wrote:
> > Hey guys, Selim has the actual commit power here (so ptal :)).
> > 
> > Btw, Selim, should I add the tests related to these changes in this CL as
> well?
> > (will they be run on this branch?)
> > 
> > Nate, do you know whether there are any cases where we can fail (receive
> > didFailLoad) a committed load except for the case in which we cancel the
load?
> > When we cancel a load we don't navigate to an error-page so in that case we
> post
> > onPageFinished only from didFailLoad (and the logic I'm adding here won't
> > trigger).
> > I just want to make sure there aren't any cases where we both trigger this
> logic
> > and the one in didFailLoad/onReceivedError (thus posting onPageFinished
> several
> > times) :).
> 
> I think it's possible but rare for a load to commit then terminate early due
to
> a problem with the network. I think that's the only possible didFailLoad
besides
> cancellation.

Ok, I guess we could avoid posting several onPageFinished (one from
didStopLoading and one from didFailLoad/onResourceError) by setting
mLastCommittedUrlWhenNavigatingToErrorPage in didFailLoad? (that would make the
logic more complex though :/).

[sgurun-gerrit only, 2017-01-11 23:06:45]

On 2017/01/11 19:36:34, gsennton wrote:
> On 2017/01/11 18:53:51, Nate Chapin wrote:
> > On 2017/01/11 14:39:48, gsennton wrote:
> > > Hey guys, Selim has the actual commit power here (so ptal :)).
> > > 
> > > Btw, Selim, should I add the tests related to these changes in this CL as
> > well?
> > > (will they be run on this branch?)
> > > 
> > > Nate, do you know whether there are any cases where we can fail (receive
> > > didFailLoad) a committed load except for the case in which we cancel the
> load?
> > > When we cancel a load we don't navigate to an error-page so in that case
we
> > post
> > > onPageFinished only from didFailLoad (and the logic I'm adding here won't
> > > trigger).
> > > I just want to make sure there aren't any cases where we both trigger this
> > logic
> > > and the one in didFailLoad/onReceivedError (thus posting onPageFinished
> > several
> > > times) :).
> > 
> > I think it's possible but rare for a load to commit then terminate early due
> to
> > a problem with the network. I think that's the only possible didFailLoad
> besides
> > cancellation.
> 
> Ok, I guess we could avoid posting several onPageFinished (one from
> didStopLoading and one from didFailLoad/onResourceError) by setting
> mLastCommittedUrlWhenNavigatingToErrorPage in didFailLoad? (that would make
the
> logic more complex though :/).


It is a trade-off between fixing the existing case where we don't post a
onPageFinished on certain circumstances (already have two bugs about it) vs. the
minimal possibility of sending multiple onPageFinisheds on certain network
errors. 

I think it is a calculated risk and worths landing. LGTM.

You don't need to write tests/fix tests for branches.

However please coordinate landing with amineer@.

[gsennton, 2017-01-12 19:20:07]

On 2017/01/11 23:06:45, sgurun wrote:
> On 2017/01/11 19:36:34, gsennton wrote:
> > On 2017/01/11 18:53:51, Nate Chapin wrote:
> > > On 2017/01/11 14:39:48, gsennton wrote:
> > > > Hey guys, Selim has the actual commit power here (so ptal :)).
> > > > 
> > > > Btw, Selim, should I add the tests related to these changes in this CL
as
> > > well?
> > > > (will they be run on this branch?)
> > > > 
> > > > Nate, do you know whether there are any cases where we can fail (receive
> > > > didFailLoad) a committed load except for the case in which we cancel the
> > load?
> > > > When we cancel a load we don't navigate to an error-page so in that case
> we
> > > post
> > > > onPageFinished only from didFailLoad (and the logic I'm adding here
won't
> > > > trigger).
> > > > I just want to make sure there aren't any cases where we both trigger
this
> > > logic
> > > > and the one in didFailLoad/onReceivedError (thus posting onPageFinished
> > > several
> > > > times) :).
> > > 
> > > I think it's possible but rare for a load to commit then terminate early
due
> > to
> > > a problem with the network. I think that's the only possible didFailLoad
> > besides
> > > cancellation.
> > 
> > Ok, I guess we could avoid posting several onPageFinished (one from
> > didStopLoading and one from didFailLoad/onResourceError) by setting
> > mLastCommittedUrlWhenNavigatingToErrorPage in didFailLoad? (that would make
> the
> > logic more complex though :/).
> 
> 
> It is a trade-off between fixing the existing case where we don't post a
> onPageFinished on certain circumstances (already have two bugs about it) vs.
the
> minimal possibility of sending multiple onPageFinisheds on certain network
> errors. 

Just to clarify: unless I'm missing something, the hotmail-bug is the only one
where we are missing an onPageFinished - the Drive-bug happens because of the
extra onPageFinished posted from shouldInterceptRequest (I don't think this
changes your logic / risk calculation here but it's worth pointing out to avoid
misunderstandings for anyone reading these comments ;)).

> 
> I think it is a calculated risk and worths landing. LGTM.
> 
> You don't need to write tests/fix tests for branches.
> 
> However please coordinate landing with amineer@.

Alright, thanks Selim.

[gsennton, 2017-01-12 21:31:48]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:33:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:33:02]

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

[commit-bot: I haz the power, 2017-01-12 21:33:03]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
and NOPRESUBMIT=true in order for the CQ to process them

[gsennton, 2017-01-12 21:33:50]

Description was changed from

==========
[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't
finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG=656919
==========

to

==========
[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't
finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG=656919

NOTRY=true
NOPRESUBMIT=true
==========

[gsennton, 2017-01-12 22:16:43]

Adding notry and nopresubmit and committing.

[gsennton, 2017-01-12 22:16:59]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:18:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 22:19:30]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484259419142090, "parent_rev":
"e22cbf90e88a90259866b48a09f255cee687ed57", "commit_rev":
"00b73e7d682c407bf15fc79e667c75fc3112ebd3"}

[commit-bot: I haz the power, 2017-01-12 22:20:04]

Description was changed from

==========
[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't
finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG=656919

NOTRY=true
NOPRESUBMIT=true
==========

to

==========
[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't
finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG=656919

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2622283002
Cr-Commit-Position: refs/branch-heads/2924@{#747}
Cr-Branched-From:
3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed:
https://chromium.googlesource.com/chromium/src/+/00b73e7d682c407bf15fc79e667c...
==========

[commit-bot: I haz the power, 2017-01-12 22:20:05]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/00b73e7d682c407bf15fc79e667c...