[Android] Tweak ChromeTabUtils.waitForTabPageLoaded loading state logic.

[Android] Tweak ChromeTabUtils.waitForTabPageLoaded loading state logic.

ChromeTabUtils.waitForTabPageLoaded was previously using Tab.isLoading()
to determine whether a tab had previously finished loading, then using
TabObserver.onPageLoadFinished to determine when a tab that hadn't
previously finished loading did so. Unfortunately, Tab's loading state
as tracked by Tab.mIsLoading is modified by the Tab.onLoadStopped
(which, in turn, calls TabObserver.onLoadStopped), not by
Tab.didPageLoadFinish (which, in turn, calls
TabObserver.onPageLoadFinished). The disconnect here means that it's
possible for ChromeTabUtils.waitForTabPageLoaded to see a Tab that is
still loading according to mIsLoading but has already fired
onPageLoadFinished. In such a scenario, waitForTabPageLoaded will time
out without ever seeing a call to onPageLoadFinished even though the
page has loaded.

This CL changes the observer used by ChromeTabUtils. Instead of simply
waiting for onPageLoadFinished, it now will:
 - recognize that the Tab has successfully loaded upon receiving
   onPageLoadFinished (same as now)
 - recognize that the Tab has failed to load upon receiving onCrash
 - note whether onLoadStopped has been called. If a timeout is hit and
   onLoadStopped has been called, the Tab is on the correct page, and
   the Tab hasn't crashed, waitForTabPageLoaded will assume that the
   Tab has successfully loaded.

BUG=709004
Review-Url: https://codereview.chromium.org/2859053004
Cr-Commit-Position: refs/heads/master@{#470172}
Committed: https://chromium.googlesource.com/chromium/src/+/8e566f616c9ecf713ce7e1283405adfb756a6915

[jbudorick, 2017-05-04 19:24:25]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[jbudorick, 2017-05-04 19:25:08]

jbudorick@chromium.org changed reviewers:
+ tedchoc@chromium.org, yolandyan@chromium.org

[commit-bot: I haz the power, 2017-05-04 19:25:15]

Dry run: 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-05-04 20:16:47]

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

[commit-bot: I haz the power, 2017-05-04 20:16:48]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-05-04 21:25:13]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:71:
mCallback.notifyException(new Exception("Tab crashed :("));
any reason not to just do fail("Tab Crashed :("); instead of adding the
machinery to the callback?  Would that also cause the test to fail?

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
I'm wondering if onPageLoadFinished and here should just be

if (loadComplete(tab, mExpectedUrl)) {
    mCallback.notifyCalled();
    tab.removeObserver(this);
}

Then we wouldn't need the countdown latch?

[jbudorick, 2017-05-04 22:34:26]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:71:
mCallback.notifyException(new Exception("Tab crashed :("));
On 2017/05/04 21:25:13, Ted C wrote:
> any reason not to just do fail("Tab Crashed :("); instead of adding the
> machinery to the callback?  Would that also cause the test to fail?

My motivation was having the failure get raised on the correct thread. The
output is better. I'll send it to you offline.

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/04 21:25:13, Ted C wrote:
> I'm wondering if onPageLoadFinished and here should just be
> 
> if (loadComplete(tab, mExpectedUrl)) {
>     mCallback.notifyCalled();
>     tab.removeObserver(this);
> }
> 
> Then we wouldn't need the countdown latch?

onPageLoadFinished should definitely be that. This, however, should not be,
because when a renderer crashes, we get onLoadStopped and then onCrash.

[Ted C, 2017-05-05 00:23:22]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/04 22:34:25, jbudorick wrote:
> On 2017/05/04 21:25:13, Ted C wrote:
> > I'm wondering if onPageLoadFinished and here should just be
> > 
> > if (loadComplete(tab, mExpectedUrl)) {
> >     mCallback.notifyCalled();
> >     tab.removeObserver(this);
> > }
> > 
> > Then we wouldn't need the countdown latch?
> 
> onPageLoadFinished should definitely be that. This, however, should not be,
> because when a renderer crashes, we get onLoadStopped and then onCrash.

Hmm...that is sadness.  The problem that this relies on a timeout to succeed
will make our tests even longer.

I wonder if we should switch to didFinishNavigation for all of this.

Then the checks could be !isErrorPage, errorCode == 0, isMainFrame == true,
mExpectedUrl == null || TextUtils.equals(url, mExpectedUrl).

[jbudorick, 2017-05-05 00:26:07]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/05 00:23:22, Ted C wrote:
> On 2017/05/04 22:34:25, jbudorick wrote:
> > On 2017/05/04 21:25:13, Ted C wrote:
> > > I'm wondering if onPageLoadFinished and here should just be
> > > 
> > > if (loadComplete(tab, mExpectedUrl)) {
> > >     mCallback.notifyCalled();
> > >     tab.removeObserver(this);
> > > }
> > > 
> > > Then we wouldn't need the countdown latch?
> > 
> > onPageLoadFinished should definitely be that. This, however, should not be,
> > because when a renderer crashes, we get onLoadStopped and then onCrash.
> 
> Hmm...that is sadness.  The problem that this relies on a timeout to succeed
> will make our tests even longer.

In the only case where we hit this condition -- i.e., *only* onLoadStopped is
called -- we'd currently time out and then fail with a loaded page. In cases
where we'd currently succeed, this change shouldn't matter. In cases where the
tab crashes, this change should speed up the test by failing more quickly.

> 
> I wonder if we should switch to didFinishNavigation for all of this.
> 
> Then the checks could be !isErrorPage, errorCode == 0, isMainFrame == true,
> mExpectedUrl == null || TextUtils.equals(url, mExpectedUrl).

I can investigate this and get back to you.

[jbudorick, 2017-05-05 16:47:50]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/05 00:26:07, jbudorick wrote:
> On 2017/05/05 00:23:22, Ted C wrote:
> > On 2017/05/04 22:34:25, jbudorick wrote:
> > > On 2017/05/04 21:25:13, Ted C wrote:
> > > > I'm wondering if onPageLoadFinished and here should just be
> > > > 
> > > > if (loadComplete(tab, mExpectedUrl)) {
> > > >     mCallback.notifyCalled();
> > > >     tab.removeObserver(this);
> > > > }
> > > > 
> > > > Then we wouldn't need the countdown latch?
> > > 
> > > onPageLoadFinished should definitely be that. This, however, should not
be,
> > > because when a renderer crashes, we get onLoadStopped and then onCrash.
> > 
> > Hmm...that is sadness.  The problem that this relies on a timeout to succeed
> > will make our tests even longer.
> 
> In the only case where we hit this condition -- i.e., *only* onLoadStopped is
> called -- we'd currently time out and then fail with a loaded page. In cases
> where we'd currently succeed, this change shouldn't matter. In cases where the
> tab crashes, this change should speed up the test by failing more quickly.
> 
> > 
> > I wonder if we should switch to didFinishNavigation for all of this.
> > 
> > Then the checks could be !isErrorPage, errorCode == 0, isMainFrame == true,
> > mExpectedUrl == null || TextUtils.equals(url, mExpectedUrl).
> 
> I can investigate this and get back to you.

After looking at this a bit, it doesn't seem to be particularly different from
onPageLoadFinished -- they both fire before onLoadStopped (w/
onDidFinishNavigation preceding onPageLoadFinished). In looking through
TabWebContentsObserver.didFinishNavigation and Tab.handleDidFinishNavigation, I
don't see anything that precipitates some reliable, readily available state
change that we can use to check whether a Tab has finished loading when we call
waitForTabPageLoaded in place of the current loadComplete logic.

[Ted C, 2017-05-05 17:04:03]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/05 16:47:50, jbudorick wrote:
> On 2017/05/05 00:26:07, jbudorick wrote:
> > On 2017/05/05 00:23:22, Ted C wrote:
> > > On 2017/05/04 22:34:25, jbudorick wrote:
> > > > On 2017/05/04 21:25:13, Ted C wrote:
> > > > > I'm wondering if onPageLoadFinished and here should just be
> > > > > 
> > > > > if (loadComplete(tab, mExpectedUrl)) {
> > > > >     mCallback.notifyCalled();
> > > > >     tab.removeObserver(this);
> > > > > }
> > > > > 
> > > > > Then we wouldn't need the countdown latch?
> > > > 
> > > > onPageLoadFinished should definitely be that. This, however, should not
> be,
> > > > because when a renderer crashes, we get onLoadStopped and then onCrash.
> > > 
> > > Hmm...that is sadness.  The problem that this relies on a timeout to
succeed
> > > will make our tests even longer.
> > 
> > In the only case where we hit this condition -- i.e., *only* onLoadStopped
is
> > called -- we'd currently time out and then fail with a loaded page. In cases
> > where we'd currently succeed, this change shouldn't matter. In cases where
the
> > tab crashes, this change should speed up the test by failing more quickly.
> > 
> > > 
> > > I wonder if we should switch to didFinishNavigation for all of this.
> > > 
> > > Then the checks could be !isErrorPage, errorCode == 0, isMainFrame ==
true,
> > > mExpectedUrl == null || TextUtils.equals(url, mExpectedUrl).
> > 
> > I can investigate this and get back to you.
> 
> After looking at this a bit, it doesn't seem to be particularly different from
> onPageLoadFinished -- they both fire before onLoadStopped (w/
> onDidFinishNavigation preceding onPageLoadFinished). In looking through
> TabWebContentsObserver.didFinishNavigation and Tab.handleDidFinishNavigation,
I
> don't see anything that precipitates some reliable, readily available state
> change that we can use to check whether a Tab has finished loading when we
call
> waitForTabPageLoaded in place of the current loadComplete logic.

While Tab#mIsLoading is set from onLoadStopped, it is actually backed by
the same thing as tab.getWebContents().isLoadingToDifferentDocument().

It uses getWebContents().isLoading(), but then checks that it is to a
different document before setting mIsLoading to true.

From what I can tell, we can just remove the tab.isLoading() check from
this call entirely and only rely on tab.isLoadingToDifferentDocument() and
then rely just on didFinishNavigation.

[jbudorick, 2017-05-05 17:51:21]

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java
(right):

https://codereview.chromium.org/2859053004/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeTabUtils.java:77:
mLoadStoppedLatch.countDown();
On 2017/05/05 17:04:03, Ted C wrote:
> On 2017/05/05 16:47:50, jbudorick wrote:
> > On 2017/05/05 00:26:07, jbudorick wrote:
> > > On 2017/05/05 00:23:22, Ted C wrote:
> > > > On 2017/05/04 22:34:25, jbudorick wrote:
> > > > > On 2017/05/04 21:25:13, Ted C wrote:
> > > > > > I'm wondering if onPageLoadFinished and here should just be
> > > > > > 
> > > > > > if (loadComplete(tab, mExpectedUrl)) {
> > > > > >     mCallback.notifyCalled();
> > > > > >     tab.removeObserver(this);
> > > > > > }
> > > > > > 
> > > > > > Then we wouldn't need the countdown latch?
> > > > > 
> > > > > onPageLoadFinished should definitely be that. This, however, should
not
> > be,
> > > > > because when a renderer crashes, we get onLoadStopped and then
onCrash.
> > > > 
> > > > Hmm...that is sadness.  The problem that this relies on a timeout to
> succeed
> > > > will make our tests even longer.
> > > 
> > > In the only case where we hit this condition -- i.e., *only* onLoadStopped
> is
> > > called -- we'd currently time out and then fail with a loaded page. In
cases
> > > where we'd currently succeed, this change shouldn't matter. In cases where
> the
> > > tab crashes, this change should speed up the test by failing more quickly.
> > > 
> > > > 
> > > > I wonder if we should switch to didFinishNavigation for all of this.
> > > > 
> > > > Then the checks could be !isErrorPage, errorCode == 0, isMainFrame ==
> true,
> > > > mExpectedUrl == null || TextUtils.equals(url, mExpectedUrl).
> > > 
> > > I can investigate this and get back to you.
> > 
> > After looking at this a bit, it doesn't seem to be particularly different
from
> > onPageLoadFinished -- they both fire before onLoadStopped (w/
> > onDidFinishNavigation preceding onPageLoadFinished). In looking through
> > TabWebContentsObserver.didFinishNavigation and
Tab.handleDidFinishNavigation,
> I
> > don't see anything that precipitates some reliable, readily available state
> > change that we can use to check whether a Tab has finished loading when we
> call
> > waitForTabPageLoaded in place of the current loadComplete logic.
> 
> While Tab#mIsLoading is set from onLoadStopped, it is actually backed by
> the same thing as tab.getWebContents().isLoadingToDifferentDocument().
> 
> It uses getWebContents().isLoading(), but then checks that it is to a
> different document before setting mIsLoading to true.
> 
> From what I can tell, we can just remove the tab.isLoading() check from
> this call entirely and only rely on tab.isLoadingToDifferentDocument() and
> then rely just on didFinishNavigation.

The backing state still seems to be set in WebContentsImpl::LoadingStateChanged
shortly before the call to the delegates' LoadingStateChanged that eventually
results in onLoadStopped being called. According to either Tab.mIsLoading or
WebContents.isLoadingToDifferentDocument(), we're still loading when we receive
either onDidFinishNavigation or onPageLoadFinished. We'd still have the same
racy check if we relied solely on
tab.getWebContents().isLoadingToDifferentDocument() and onDidFinishNavigation().

[Ted C, 2017-05-08 20:52:31]

lgtm

[jbudorick, 2017-05-08 22:01:38]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-05-08 22:01:38]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2859053004/#ps40001
(title: "expanded comment")

[jbudorick, 2017-05-08 22:02:07]

The CQ bit was unchecked by jbudorick@chromium.org

[commit-bot: I haz the power, 2017-05-08 22:02:26]

CQ is trying da patch.

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

[jbudorick, 2017-05-08 22:04:51]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2017-05-08 22:05:55]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-05-08 22:05:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2859053004/#ps60001
(title: "typos & copy edits")

[commit-bot: I haz the power, 2017-05-08 22:06:55]

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-05-08 22:52:49]

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

[commit-bot: I haz the power, 2017-05-08 22:52:50]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbudorick, 2017-05-08 23:09:04]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2017-05-08 23:17:20]

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-05-09 03:53:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494284944936180,
"parent_rev": "d980e96a774c7e8d82039e5ef371b6ac0cb33b5d", "commit_rev":
"8e566f616c9ecf713ce7e1283405adfb756a6915"}

[commit-bot: I haz the power, 2017-05-09 03:53:54]

Description was changed from

==========
[Android] Tweak ChromeTabUtils.waitForTabPageLoaded loading state logic.

ChromeTabUtils.waitForTabPageLoaded was previously using Tab.isLoading()
to determine whether a tab had previously finished loading, then using
TabObserver.onPageLoadFinished to determine when a tab that hadn't
previously finished loading did so. Unfortunately, Tab's loading state
as tracked by Tab.mIsLoading is modified by the Tab.onLoadStopped
(which, in turn, calls TabObserver.onLoadStopped), not by
Tab.didPageLoadFinish (which, in turn, calls
TabObserver.onPageLoadFinished). The disconnect here means that it's
possible for ChromeTabUtils.waitForTabPageLoaded to see a Tab that is
still loading according to mIsLoading but has already fired
onPageLoadFinished. In such a scenario, waitForTabPageLoaded will time
out without ever seeing a call to onPageLoadFinished even though the
page has loaded.

This CL changes the observer used by ChromeTabUtils. Instead of simply
waiting for onPageLoadFinished, it now will:
 - recognize that the Tab has successfully loaded upon receiving
   onPageLoadFinished (same as now)
 - recognize that the Tab has failed to load upon receiving onCrash
 - note whether onLoadStopped has been called. If a timeout is hit and
   onLoadStopped has been called, the Tab is on the correct page, and
   the Tab hasn't crashed, waitForTabPageLoaded will assume that the
   Tab has successfully loaded.

BUG=709004
==========

to

==========
[Android] Tweak ChromeTabUtils.waitForTabPageLoaded loading state logic.

ChromeTabUtils.waitForTabPageLoaded was previously using Tab.isLoading()
to determine whether a tab had previously finished loading, then using
TabObserver.onPageLoadFinished to determine when a tab that hadn't
previously finished loading did so. Unfortunately, Tab's loading state
as tracked by Tab.mIsLoading is modified by the Tab.onLoadStopped
(which, in turn, calls TabObserver.onLoadStopped), not by
Tab.didPageLoadFinish (which, in turn, calls
TabObserver.onPageLoadFinished). The disconnect here means that it's
possible for ChromeTabUtils.waitForTabPageLoaded to see a Tab that is
still loading according to mIsLoading but has already fired
onPageLoadFinished. In such a scenario, waitForTabPageLoaded will time
out without ever seeing a call to onPageLoadFinished even though the
page has loaded.

This CL changes the observer used by ChromeTabUtils. Instead of simply
waiting for onPageLoadFinished, it now will:
 - recognize that the Tab has successfully loaded upon receiving
   onPageLoadFinished (same as now)
 - recognize that the Tab has failed to load upon receiving onCrash
 - note whether onLoadStopped has been called. If a timeout is hit and
   onLoadStopped has been called, the Tab is on the correct page, and
   the Tab hasn't crashed, waitForTabPageLoaded will assume that the
   Tab has successfully loaded.

BUG=709004

Review-Url: https://codereview.chromium.org/2859053004
Cr-Commit-Position: refs/heads/master@{#470172}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e566f616c9ecf713ce7e1283405...
==========

[commit-bot: I haz the power, 2017-05-09 03:53:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8e566f616c9ecf713ce7e1283405...