Tim, can you give Chris a hand with this (or redirect him to someone else that knows the gesture pipeline here)?

Is this still flaky if the long press timer fires instantly?

What happens if you set the long press timeout to 0?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_detection/gesture_configuration_android.cc&l=46

The way long press is synthesized and recognized here, we don't really have any timing guarantees.

What causes InternalSwapCompositorFrame to be called in these tests?

I'm not seeing OnShowPastePopup, where is it defined? (https://code.google.com/p/chromium/codesearch#search/&q=onshowpastepopup&sq=package:chromium&type=cs)

+aelias

I took a quick look. 
The test testSelectActionBarPasswordPaste has three steps:
1. Long-press-select the test from password field.
2. Paste the text from the clipboard into the password field.
3. Long-press-select the password field and ensure the text has changed.

The editable property is set in RenderWidgetHostViewAndroid::OnFrameMetadataUpdated based on the frame metadata. The default value is false, so it's false for empty selections. The issue could be that there is a race condition between steps 2 and 3: On the browser side we have have already processed the long-press from step 3, so selection controller thinks there is an active selection (TouchSelectionController::WillHandleLongPressEvent was called). However the frame metadata that we are getting in RenderWidgetHostViewAndroid::OnFrameMetadataUpdated corresponds to the empty selection from step 2, and the editable property is set to the default value, i.e. false. So TouchSelectionController::OnSelectionEditable gets called, which deactivates the insertion.

This is not the first time we get this sort of race conditions. We might be able to patch this up by adding some more state/verification to TSC so that we can tell that the metadata we are getting is stale. A better way would be to add more selection metadata as suggested in issue 571897, so that we don't have to do as much guessing on the TSC side.

Just to clarify, all the tests currently disabled in that file seem to be caused by this bug (not just testSelectActionBarPasswordPaste).

I have been hunting by using testPastePopupNotShownOnLongPressingNonEmptyInput https://code.google.com/p/chromium/codesearch#chromium/src/content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java&q=testSelectActionBarPasswordPaste&sq=package:chromium&l=137

In that test it is a bit easier to see. I can comment out the second long-press. The issue happens between these lines:

DOMUtils.longPressNode(this, mContentViewCore, "empty_input_text");
waitForPastePopupStatus(true);

waitForPastePopupStatus(true); will poll until it times out hoping that the paste popup shows. However, the predicate it uses doesn't only return if the paste popup UI element is visible -- it also returns false if the element is null.

I confirmed that in all cases where it flakes (at least, in my several test runs) the null is the cause of the test flaking.

So then I investigated why the element was set to null.

RenderWidgetHostViewAndroid::InternalSwapCompositorFrame https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc&q=InternalSwapCompositorFrame&sq=package:chromium&type=cs&l=1107
calls RenderWidgetHostViewAndroid::OnFrameMetadataUpdated https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc&q=OnFrameMetadataUpdated&sq=package:chromium&type=cs&l=1284
which calls TouchSelectionController::OnSelectionEditable https://code.google.com/p/chromium/codesearch#chromium/src/ui/touch_selection/touch_selection_controller.cc&q=OnSelectionEditable&sq=package:chromium&type=cs&l=294

Normally, that last function exits early.
But because we call ShowPastePopup before the 20th InternalSwapCompositorFrame it doesn't exit early. And as that function runs, it will end up calling DeactivateInsertion(); which will obviously destroy the insertion handles and so the PastePopup ends up not being shown.

+aelias@ who wasn't added before

At first glance, it looks like the renderer is explicitly sending a notification that the selection is no longer editable, and hiding the paste popup is a correct reaction to such a notification.  So the root cause probably related to why is that notification being sent.

@aelias: Do you think what I described in comment #4 could be the cause?

I tried setting the long press timeout to 0 and it crashed. I could try to investigate why it crashed if we think this is a valuable path to explore.

"OnShowPastePopup" was supposed to be -On. It is:
content/browser/android/content_view_core_impl.cc ContentViewCoreImpl::ShowPastePopup


>> What causes InternalSwapCompositorFrame to be called in these tests?
That is what I am curious about.
If I can make sure all 20 of those calls happen before the ShowPastePopup happens then we'll fix this flaky test.
The calls are coming in here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc&q=RenderWidgetHostView%20OnSwapCompositorFrame&sq=package:chromium&type=cs&l=1110

That is being called from here, where the IPC message comes in: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_impl.cc&q=OnSwapCompositorFrame&sq=package:chromium&l=1568&ct=rc&cd=18&dr=CSs

>> However the frame metadata that we are getting in RenderWidgetHostViewAndroid::OnFrameMetadataUpdated corresponds to the empty selection from step 2, and the editable property is set to the default value, i.e. false. So TouchSelectionController::OnSelectionEditable gets called, which deactivates the insertion.

That is exactly what is happening.

However, it isn't happening between steps 2 and 3.


public void testPastePopupNotShownOnLongPressingNonEmptyInput() throws Throwable {
  copyStringToClipboard("SampleTextToCopy");
  // Select the empty text field.
  DOMUtils.longPressNode(this, mContentViewCore, "empty_input_text");
  waitForPastePopupStatus(true);
  //DOMUtils.longPressNode(this, mContentViewCore, "input_text");
  //waitForSelectActionBarVisible(true);
  //waitForPastePopupStatus(false);
}

The test still failed in the same flaky way.

But yeah, that 20th call to InternalSwapCompositorFrame has the same contents in both the non-flaky and flaky cases. It's just in the non-flaky case it takes the early exit. And in the flaky case it has editable set to false, which deactivates the insertion like you said.

Could you check if RenderWidgetCompositor::clearSelection() is being called (or perhaps RenderWidgetCompositor::registerSelection())?  Then track down the source of that.

Adding logging to RenderWidgetCompositor and the functions that call into it has made the test less likely to flake.

It looks like during the 20x calls to InternalSwapCompositorFrame are several calls to:

FrameView::updateLifecycleToCompositingCleanPlusScrolling
which calls FrameView::updateLifecyclePhasesInternal

Although, it doesn't seem to be a 1:1 mapping. I am curious about updateLifecyclePhasesInternal...it looks like it might be calling some recursive functions.

When the insertion handles are added there is this list of calls:

FrameView::updateAllLifecyclePhases
calls FrameView::updateLifecyclePhasesInternal
calls FrameView updateCompositedSelectionIfNeeded
calls RenderWidgetCompositor::registerSelection

RenderWidgetCompositor::clearSelection is only called a few times on startup.
It does not seem to be called when the extra InternalSwapCompositorFrame is called after ShowPastePopup.

SLB

To clarify,
#5 and #10 are responses to #4
#9 and is a response to #3

However, I want to get this back on track. My original goal was to figure out where the messages were posted from so I could continue to investigate. I didn't make that clear enough.

I spoke with mfomitchev@ and mohsen@ and we are now all on the same page.

mohsen@ suggested that it is possible that the frame update messages are being posted from cc/trees/layer_tree_host_impl.cc MakeCompositorFrameMEtaData on the compositor side. And that the show context menu messages may be posted from third_party/WebKit/Source/core/input/EventHandler.cpp EventHandler::handleGestureLongPress on the blink side.

I will investigate to confirm this and see if there is a way that the compositor's messages, which become interleaved with blink's messages, could not use outdated or soon-to-be-outdated data.

An alternative to fixing the info in compositor messages could be to make the paste popup appear based on compositor frame metadata messages instead of the "show context menu" message. I.e. have it controlled by the TouchSelectionController. The selection "floating menu" (containing Copy) is already controlled by TouchSelectionController, and so are the selection handles. It seems logical to have the paste popup controlled by TouchSelectionController as well. It already works this way on Aura, and the reason it's not done this way on Android is probably purely historic.