Issue 892226  has been merged into this issue.

FindIt thinks https://chromium-review.googlesource.com/c/chromium/src/+/1371899 made this test more flaky.  That's possible, although I suspect this test like others I've fixed is relying on the interleaving of delayed and non-delayed tasks which is an error.

I'll be happy to try and fix this test when I get back in the office (Jan 3rd), in the mean time if it's causing problems I'd suggest disabling it because it was already flaky.

Looks like views' test framework assumes things about task ordering which aren't strictly guaranteed (but happened to be true under base::MessageLoop).

In particular, SequenceManager treats both DoWork and DoDelayedWork as just an opportunity to "do work" (refactoring to merge them incoming : bit.ly/merge-message-pump-do-work). This means that MessagePumpForUI's typical sequence of DoSystemWork/DoWork/DoDelayedWork can now process two undelayed application tasks for each system event.

I think this is making RunClosureAfterAllPendingUIEvents() (ui/views/test/ui_controls_factory_desktop_aurax11.cc) flaky.

Actually, my bad RunClosureAfterAllPendingUIEvents() is in the X11 version of the code.

I subsequently thought the flake was caused by its Windows equivalent : SendKeyPressImpl() (ui/base/test/ui_controls_internal_win.cc).

But digging some more I think the issue is with this specific test, the flaky scenario is:

 1) ...
 2) BookmarkBarViewTest24::Step3()
    => ui_controls::SendKeyPress(ui::VKEY_APPS)
      => send WM_KEYDOWN ui::VKEY_APPS
      => send WM_KEYUP ui::VKEY_APPS
 3) Process WM_KEYDOWN enqueues some tasks
 4) Enqueued tasks run and ultimately BookmarkContextMenuNotificationObserver triggers Step4()
 5) BookmarkBarViewTest24::Step4()
    => ui_controls::SendKeyPressNotifyWhenDone(ui::VKEY_ESCAPE, Bind(&Step5))
      => InputDispatcher::CreateForMessage (observes WM_KEYUP)
      => send WM_KEYDOWN ui::VKEY_ESCAPE
      => send WM_KEYUP ui::VKEY_ESCAPE
 6) Process WM_KEYUP from (2)!! (flake source)
    => Enqueue Step5()
 7) Process Step5() (too early, flake on ASSERT_FALSE(bb_view_->GetContextMenu());)
 8) Process WM_KEYDOWN ui::VKEY_ESCAPE -- too late.

r616280 doesn't introduce the race but it increases the likelihood of it occurring because ThreadControllerWithMessagePumpImpl/SequenceManagerImpl intentionally do not differentiate between DoWork/DoDelayedWork (intent is to merge them soon -- bit.ly/merge-message-pump-do-work). As such DoDelayedWork() can process non-delayed tasks and the typical MessagePumpForUI's ProcessNextWindowsMessage/DoWork/DoDelayedWork now runs 2 application tasks for each system task even in the absence of delayed work.
As such, in the above scenario it is more likely since r616280 that tasks enqueued to handle VKEY_APPS are processed fast enough to allow Step5() to run before handling WM_KEYDOWN (additional theory : Step5() could still flakily have been enqueued too early before but the backlog of application tasks would have allowed the WM_KEYDOWN to be handled before Step5() executed).

This means that all users of ui_controls::SendKeyPress() are currently potentially making any future calls to ui_controls::SendKeyPressNotifyWhenDone() flaky.

(and I think two ui_controls::SendKeyPressNotifyWhenDone() in a row can be flaky as well if the first one uses a modifier -- because AppendAcceleratorInputs() will cause key ups for the modifiers *after* the InputDispatcher fired for key's WM_KEYUP).

Update : working on a potential fix @ https://chromium-review.googlesource.com/c/chromium/src/+/1392178

From flakiness dashboard it looks like it's no longer flaky on linux-xenial-rel (OP) : https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=interactive_ui_tests&tests=BookmarkBarViewTest24.ContextMenusKeyboardEscape

All recent flakes are on Windows bots so this is now a Windows only bug. I will temporarily disable this test on Windows to relieve the CQ while working on fix.

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

commit 7a3681e2ea0b5d27426a0d2508f2fd5c9bd46817
Author: Gabriel Charette <gab@chromium.org>
Date: Sun Dec 30 15:36:58 2018

Temporarily disable flaky BookmarkBarViewTest24.ContextMenusKeyboardEscape on Windows

See  https://crbug.com/892228#c4  for flake diagnosis.

TBR=pkasting@chromium.org

Bug:  892228 
Change-Id: I000743d3b38a12171af09fc5aa8d7d0f07d89c91
Reviewed-on: https://chromium-review.googlesource.com/c/1392713
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619277}
[modify] https://crrev.com/7a3681e2ea0b5d27426a0d2508f2fd5c9bd46817/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc

Thanks for looking into this gab.  It seems it's more complex than I thought, my initial plan was to try MOCK_TIME to see if that fixed it.

Right, I don't think MOCK_TIME would help. See tentative fix under review @ https://chromium-review.googlesource.com/c/chromium/src/+/1392178

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

commit 05ff85c02778369dd6ac93c7797eb001a3c49b9c
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Jan 10 18:11:13 2019

[ui_controls] Refactor InputDispatcher's memory model.

Extracted from https://chromium-review.googlesource.com/c/chromium/src/+/1392178

Removes manual ref-counting in favor of just-in-time creation + unique
self ownership. Required for WeakPtr scheme in follow-up CL.

Note: Patch set 1 tried to add checks that confirm the pre-existing assumptions
made by InputDispatcher (i.e. that the input was successfully sent) but some
scenarios fail those tests. To be investigated as a follow-up.

R=grt@chromium.org, sadrul@chromium.org

Bug:  892228 , 640996
Change-Id: Ia1df67e809845a228c4a9a6b77da08de037cf621
Reviewed-on: https://chromium-review.googlesource.com/c/1400950
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621639}
[modify] https://crrev.com/05ff85c02778369dd6ac93c7797eb001a3c49b9c/ui/base/test/ui_controls_internal_win.cc

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

commit 86c995c3718c4c14997a8a7b6422e56bad747685
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Jan 10 18:25:42 2019

[ui_controls] Fix InputDispatcher's handling of KeyboardProc

Extracted from
https://chromium-review.googlesource.com/c/chromium/src/+/1392178

While this CL doesn't fix all the flakiness encountered (i.e. doesn't
deal with pending WM_KEYUPs flakily being interpreted as the just-sent
key up events) : it's a strict improvement by addressing two fundamental
flaws of the existing InputDispatcher.

 1) Use bit 31, not 30, to determine key up events.
 2) Also expect key ups for modifiers.

While (2) isn't a complete fix, it might actually help reduce the
incidence of flakes caused by :
  A) SendInput and don't wait for them to complete
  B) SendInput and DO wait for them to complete
  (can flake when wait (B) unintentionally observes incomplete (A))
Without (2) (A) could also be a  SendInput WITH an InputDispatcher since
modifiers weren't waited upon and hence could leak into (B).

R=grt@chromium.org, sadrul@chromium.org

Bug:  892228 , 640996
Change-Id: I8848193ee4d53a7c60c845a9b02562662715ae3c
Reviewed-on: https://chromium-review.googlesource.com/c/1401223
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621649}
[modify] https://crrev.com/86c995c3718c4c14997a8a7b6422e56bad747685/ui/base/test/ui_controls_internal_win.cc

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

commit ec52747a5abc25a862843edca761104b3c319764
Author: Gabriel Charette <gab@chromium.org>
Date: Mon Jan 14 16:16:49 2019

[ui_controls] Unflake Send*NotifyWhenDone() on Windows

ui_controls::Send*NotifyWhenDone() can be flaky when invoked after
ui_controls::Send*() as the former can decide to notify based on
observing a yet-to-be-processed event from the latter (or even a
yet-to-be-processed event emitted by unrelated code) and thus notify too
early, resuming and testing conditions that have yet to be met.

Solution: defer the notification if the system queue has pending events
of the same type awaiting dispatch.

Note: mouse move can be repeated indefinitely during a drag, as such we
consider a mouse move complete when it hits the target regardless of
remaining mouse move messages in the queue.

@ BUG OWNERS : This might unflake many currently disabled tests. I've
CC'ed interactive_ui_tests + Windows bugs, please try to re-enable your
test after this CL if you think it might be related.

Bug:  892228 , 640996, 897801,893078,876224,875443,873110,852786,850343,848049,846695,840369,798492,756338,751031,665296,651906,499858,468660,419468,238347,131612,106489,97777,92467
Change-Id: I548856a3948ff71a145435799b4ba3e689561f14
Reviewed-on: https://chromium-review.googlesource.com/c/1392178
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622470}
[modify] https://crrev.com/ec52747a5abc25a862843edca761104b3c319764/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
[modify] https://crrev.com/ec52747a5abc25a862843edca761104b3c319764/ui/base/test/ui_controls_internal_win.cc

Test was re-enabled and I think it should no longer be flaky; I expect FindIt will let us know if not.

\o/