|
|
Description[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
Patch Set 1 #
Total comments: 9
Patch Set 2 : partial tedchoc comments #Patch Set 3 : expanded comment #Patch Set 4 : typos & copy edits #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run
jbudorick@chromium.org changed reviewers: + tedchoc@chromium.org, yolandyan@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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.
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).
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.
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.
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.
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().
lgtm
The CQ bit was checked by jbudorick@chromium.org
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")
The CQ bit was unchecked by jbudorick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jbudorick@chromium.org
The CQ bit was checked by jbudorick@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494284944936180, "parent_rev": "d980e96a774c7e8d82039e5ef371b6ac0cb33b5d", "commit_rev": "8e566f616c9ecf713ce7e1283405adfb756a6915"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8e566f616c9ecf713ce7e1283405... |