New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 671201 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 673282



Sign in to add a comment

org.chromium.chrome.browser.externalnav.UrlOverridingTest#testOpenWindowFromUserGesture fails consistently on tablets

Project Member Reported by jbudorick@chromium.org, Dec 5 2016

Issue description

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk&tests=org.chromium.chrome.browser.externalnav.UrlOverridingTest%23testOpenWindowFromUserGesture

failure mode appears to be:

	
junit.framework.AssertionFailedError: Criteria not met in allotted time.
	at org.chromium.content.browser.test.util.CriteriaHelper.pollInstrumentationThread(CriteriaHelper.java:74)
	at org.chromium.content.browser.test.util.CriteriaHelper.pollUiThread(CriteriaHelper.java:112)
	at org.chromium.content.browser.test.util.CriteriaHelper.pollUiThread(CriteriaHelper.java:133)
	at org.chromium.chrome.browser.externalnav.UrlOverridingTest.loadUrlAndWaitForIntentUrl(UrlOverridingTest.java:197)
	at org.chromium.chrome.browser.externalnav.UrlOverridingTest.testOpenWindowFromUserGesture(UrlOverridingTest.java:322)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
	at org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:726)
	at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161)
	at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853)

https://chromium.googlesource.com/chromium/src/+/318ecffe833701cfd6db2ac43491ace0a68e18af appears to be the most likely culprit from the blamelist. bmcquade, can you take a look?
 
test still failing, friendly ping
will revert

Comment 3 by dgn@chromium.org, Dec 8 2016

friendly ping :)
Status: Started (was: Assigned)
as noted yesterday, i'm working on a revert: https://codereview.chromium.org/2557233002

the revert did not apply cleanly so i'm having it re-reviewed before committing.
Blocking: 673282
Labels: -Pri-2 Pri-1
Hopefully the revert can be applied soon. The test has been broken for over a week now.
Sure, should land today.

For future reference, how should developers find these test failures before landing? I don't recall seeing these tests run as part of the try jobs before submitting my original patch. Any reason these tests don't run with the tryjobs by default? If these had been included in the try jobs, this patch never would have landed, you'd save time pinging this bug, and I'd not have had to spend my time landing a revert.
This test doesn't run on the CQ because it's restricted to tablets (https://codesearch.chromium.org/chromium/src/chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java?rcl=0&l=320) and we don't have sufficient hardware to support tablets on the CQ.
I'm sorry, some tests are now failing consistently in the rollback patch, so this will need to wait another day as I debug. Will try to land this tomorrow.
This didn't land today, but bmcquade@ is still working on a fix. The a revert is apparently causing issues with other tests.

He is hoping to get the fix landed 2016-12-13 (Tuesday).
For the record, I did a git bisect, and it is quite clear that this is the culprit (if that ever was a question :-):

###
318ecffe833701cfd6db2ac43491ace0a68e18af is the first bad commit
commit 318ecffe833701cfd6db2ac43491ace0a68e18af
Author: bmcquade <bmcquade@chromium.org>
Date:   Wed Nov 30 10:44:25 2016 -0800

    Set user_gesture bit at NavigationHandle creation time.
    
    Previously, the user_gesture bit was set at different places and could be
    updated during the lifetime of a navigation, such as in WillStartNavigation
    and at commit time. In practice, whether a navigation was initiated by
    user gesture is known at the time a NavigationHandle is created.
    This change makes has_user_gesture_ a const member of NavigationHandleImpl,
    and sets it once at construction time.
    
    clamy suggested that we could set the user gesture bit at NavHandle
    construction time in the following places:
    - PlzNavigate: we have it when we create the handle.
    - Regular navs: we add the parameter to DidStartProvisionalLoad.
    - same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
      where we have the information.
    
    This patch makes the following changes:
    - adds a gesture param to NavigationHandleImpl::Create
    - adds a gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
      which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
    - removes the has_user_gesture boolean param from
      NavigationHandleImpl::WillStartRequest
    
    For the time being, we continue to update the gesture_ value at commit time.
    Once crbug.com/667572 is addressed, gesture_ can be made a const member and
    we can stop updating it at commit time.
    
    BUG= 665952 
    CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
    
    Review-Url: https://codereview.chromium.org/2499313003
    Cr-Commit-Position: refs/heads/master@{#435354}

###



For future reference, this is what I did:
Setup:
$ git bisect start
$ git bisect bad  # HEAD was at a28b757f7cb3cd93b0be18e7b3de5eaede46d41a
$ git bisect good 448b3411da25817b84202bff7c37bbc816c4b2b2

Then iteratively:
Run test: $ gclient sync -r src@HEAD && time CHROMIUM_OUT_DIR="out-sheriff" ninja -j10000 -l30 -C out-sheriff/Debug chrome_public_test_apk && ./out-sheriff/Debug/bin/run_chrome_public_test_apk -vv -f "org.chromium.chrome.browser.externalnav.UrlOverridingTest#testOpenWindowFromUserGesture"
Mark with 'git bisect good' or 'git bisect bad'.

My device cable was flaky, so I couldn't easily run the automated 'git bisect run'.
Status: Fixed (was: Started)
The revert has landed, so the test should go green soon. Apologies for the delay here.

Thanks Tommy for the instructions on how to bisect, and for investigating the cause here. I'll use those instructions next time around.

Sign in to add a comment