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

Issue 818897 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
(OOO slow)
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocked on:
issue 818374

Blocking:
issue 754862



Sign in to add a comment

[TTS] Mojo-version of Tap notification is flaky, based on automated tests

Project Member Reported by donnd@google.com, Mar 6 2018

Issue description

When we landed the mojo version of CS Tap notification on trunk on 3/2/18, one or two tests started to fail on the KitKat Release build (crbug.com/818374), so the CL was reverted.   Evidently something in the call chain for mojo notification is flaky (or causing the test to be flaky).

I can occasionally reproduce the failure locally on a Kitkat Release phone.  Hoping to figure out what's causing the flake, or limit the flakiness, so we can reland.

 

Comment 1 by donnd@google.com, Mar 6 2018

I have a theory that the mojo messaging is quick enough that it sometimes beats the regular messaging for selection-cleared when a tap occurs.  This would explain why it happens only on one specific platform (KitKat Rel) -- it's related to performance and a race condition that only is exposed when the race is not consistently won.

When I look at the state transitions during the automated test "testTapALot" that does repeated taps, we normally see:
I/System.out( 8203): ctxs trans: SELECTION_CLEARED_RECOGNIZED -> WAITING_FOR_POSSIBLE_TAP_NEAR_PREVIOUS
I/System.out( 8203): ctxs trans: TAP_RECOGNIZED -> WAITING_FOR_POSSIBLE_TAP_ON_TAP_SELECTION
I/System.out( 8203): ctxs trans: WAITING_FOR_POSSIBLE_TAP_ON_TAP_SELECTION -> TAP_GESTURE_COMMIT
I/System.out( 8203): ctxs trans: TAP_GESTURE_COMMIT -> GATHERING_SURROUNDINGS
etc...
but in the failing case we see
I/System.out( 8203): ctxs trans: TAP_RECOGNIZED -> WAITING_FOR_POSSIBLE_TAP_ON_TAP_SELECTION
I/System.out( 8203): ctxs trans: SELECTION_CLEARED_RECOGNIZED -> WAITING_FOR_POSSIBLE_TAP_NEAR_PREVIOUS
I/System.out( 8203): ctxs trans: WAITING_FOR_POSSIBLE_TAP_NEAR_PREVIOUS -> IDLE

Not sure how best to handle this, short of changing the selection-notification to Mojo too.  Maybe we should ignore Selection-cleared that happen right after a Tap, but that feels like a hack.

Comment 2 by donnd@google.com, Mar 7 2018

The following revision WAS SUPPOSED TO refer to this bug:
------------------------------
  https://chromium.googlesource.com/chromium/src.git/+/84b690feb3f2e20883f72a5bcbfd141c65372a15

commit 84b690feb3f2e20883f72a5bcbfd141c65372a15
Author: Donn Denman <donnd@google.com>
Date: Wed Mar 07 02:06:10 2018

[TTS] Minor test cleanup and improvements.

Poll the UI thread instead of the Intrumentation thread
in some cases -- likely there are others that should be
changed too.

Remove my initials from test names (done for local
debugging and accidently submitted).

BUG= 818374 

Change-Id: I5bb80c520b155be94a9c1ecfc0c9c1f3a0e4184e
Reviewed-on: https://chromium-review.googlesource.com/950319
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541282}
[modify] https://crrev.com/84b690feb3f2e20883f72a5bcbfd141c65372a15/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
------------------------------

The above change should allow us to land the mojo-ification without too much of an impact on flakiness.

I was unable to apply a good workaround to the code other than just limiting the failing test to fewer iterations and some other possibly helpful changes in this CL.

I plan to reland, watch the tree, and update this bug with observations.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a

commit fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a
Author: Donn Denman <donnd@google.com>
Date: Fri Mar 09 05:08:57 2018

[TTS] Restrict tests that are flaking on KitKat.

Changes some Contextual Search tests that are flaking on KitKat.
Now they only run on Android-L or later.

BUG=818897

Change-Id: I97b10e45b055ccc8b05dce569b44c8f987c13d90
Reviewed-on: https://chromium-review.googlesource.com/953563
Commit-Queue: Donn Denman <donnd@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542033}
[modify] https://crrev.com/fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bd2e471d9abc7413907974f56b8f694fb2180bd

commit 5bd2e471d9abc7413907974f56b8f694fb2180bd
Author: Donn Denman <donnd@google.com>
Date: Fri Mar 16 17:16:40 2018

[TTS] Test if tap flakiness is based on the thread

Adds another ContextualSearchManagerTest similar to an existing test
that is currently somewhat flaky to see if the flakiness is due to
polling on the UI thread vs the Instrumentation thread.

BUG=818897

Change-Id: Icb40f34db4de4b1d26a4dc555a3bbf0d01b1090d
Reviewed-on: https://chromium-review.googlesource.com/963288
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543739}
[modify] https://crrev.com/5bd2e471d9abc7413907974f56b8f694fb2180bd/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/721b860552205c51f38809c900558258ff61ff63

commit 721b860552205c51f38809c900558258ff61ff63
Author: Donn Denman <donnd@google.com>
Date: Fri Apr 20 19:55:58 2018

[TTS] Fix Tap mojo race condition and update tests.

This CL implements a workaround for the race condition between the mojo
tap notification and the selection-changed notification.  Now we never enter
the SELECTION_CLEARED internal state unless there was a previous
selection that got cleared.

Before this CL every tap caused a transition to the SELECTION_CLEARED
internal state even when there was no previous selection.  This would usually
happen before the Tap notification, but now that's faster due to mojo so it
sometimes arrives beforehand.  With this change we ignore selection changes
from empty to empty so there's only one notification for most taps. Now we only
enter the SELECTION_CLEARED state when it's really needed (in the tap
near previous selection use-case).

Also does some minor test cleanup and re-enabling disabled tests that
were flaky.

BUG=818897

Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e
Reviewed-on: https://chromium-review.googlesource.com/1018300
Commit-Queue: Donn Denman <donnd@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552439}
[modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java
[modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
[modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java
[modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93

commit a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93
Author: Donn Denman <donnd@chromium.org>
Date: Fri Apr 20 20:34:52 2018

Revert "[TTS] Fix Tap mojo race condition and update tests."

This reverts commit 721b860552205c51f38809c900558258ff61ff63.

Reason for revert: Caused a few test failures: testTap and testImageControl.

Original change's description:
> [TTS] Fix Tap mojo race condition and update tests.
> 
> This CL implements a workaround for the race condition between the mojo
> tap notification and the selection-changed notification.  Now we never enter
> the SELECTION_CLEARED internal state unless there was a previous
> selection that got cleared.
> 
> Before this CL every tap caused a transition to the SELECTION_CLEARED
> internal state even when there was no previous selection.  This would usually
> happen before the Tap notification, but now that's faster due to mojo so it
> sometimes arrives beforehand.  With this change we ignore selection changes
> from empty to empty so there's only one notification for most taps. Now we only
> enter the SELECTION_CLEARED state when it's really needed (in the tap
> near previous selection use-case).
> 
> Also does some minor test cleanup and re-enabling disabled tests that
> were flaky.
> 
> BUG=818897
> 
> Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e
> Reviewed-on: https://chromium-review.googlesource.com/1018300
> Commit-Queue: Donn Denman <donnd@chromium.org>
> Reviewed-by: Theresa <twellington@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#552439}

TBR=donnd@chromium.org,twellington@chromium.org

Change-Id: Ic84b636996d182fce0cf630d1255b142f8f7619b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 818897
Reviewed-on: https://chromium-review.googlesource.com/1022670
Reviewed-by: Donn Denman <donnd@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552456}
[modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java
[modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
[modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java
[modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1d3ae38a77103597152d30487d19ddd519dfd3a

commit f1d3ae38a77103597152d30487d19ddd519dfd3a
Author: Donn Denman <donnd@google.com>
Date: Fri Apr 27 15:57:38 2018

Reland "[TTS] Fix Tap mojo race condition and update tests."

This is a reland of 721b860552205c51f38809c900558258ff61ff63

I've run the tests that flaked locally for several iterations without failure,
so I'm thinking the failures were due to associated flakes.

Original change's description:
> [TTS] Fix Tap mojo race condition and update tests.
>
> This CL implements a workaround for the race condition between the mojo
> tap notification and the selection-changed notification.  Now we never enter
> the SELECTION_CLEARED internal state unless there was a previous
> selection that got cleared.
>
> Before this CL every tap caused a transition to the SELECTION_CLEARED
> internal state even when there was no previous selection.  This would usually
> happen before the Tap notification, but now that's faster due to mojo so it
> sometimes arrives beforehand.  With this change we ignore selection changes
> from empty to empty so there's only one notification for most taps. Now we only
> enter the SELECTION_CLEARED state when it's really needed (in the tap
> near previous selection use-case).
>
> Also does some minor test cleanup and re-enabling disabled tests that
> were flaky.
>
> BUG=818897
>
> Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e
> Reviewed-on: https://chromium-review.googlesource.com/1018300
> Commit-Queue: Donn Denman <donnd@chromium.org>
> Reviewed-by: Theresa <twellington@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#552439}

Bug: 818897
Change-Id: Ibe3034c2fd2dee8bdfb5e69aacbef6e4836ba4ad
Reviewed-on: https://chromium-review.googlesource.com/1030770
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554398}
[modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java
[modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
[modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java
[modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Sign in to add a comment