[Aura] Ignore selection changes when a menu is closing.

[Aura] Ignore selection changes when a menu is closing.

The synthesized mouse move events when a menu is closing could cause the selection to change, which would notify the RenderWidgetHostViewBase twice, triggering a DCHECK.

BUG=118872, 237819
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198811

[Bernhard Bauer, 2013-05-06 13:24:15]

Please review.

[sky, 2013-05-06 16:14:26]

I much prefer the synchronous dispatch here. What is the specific scenario that
is happening?

[Bernhard Bauer, 2013-05-06 17:02:46]

On 2013/05/06 16:14:26, sky wrote:
> I much prefer the synchronous dispatch here. What is the specific
scenario that
> is happening?

These browser tests show a context menu (running a nested message loop
inside MenuController::Run()) and very quickly close it again. When closing
the context menu, MenuController::Cancel() sets the selection to NULL,
which causes capture to change. If we synchronously synthesize a mouse
event then, in some cases[*] that event causes a selection change again,
which makes the selection non-NULL. Then when it returns from the nested
message loop, MenuController::Run() sets the selection to NULL again. Both
of these transitions to NULL call
RenderWidgetHostViewBase::SetShowingContextMenu(false), but the second call
finds that the menu is already closed, and DCHECKS. The stack trace for the
reentrant SetSelection() looks like this:

views::MenuController::SetSelection()
views::MenuController::HandleMouseLocation()
views::MenuController::OnMouseMoved()
views::Widget::OnMouseEvent()
views::NativeWidgetAura::OnMouseEvent()
ui::EventTarget::OnEvent()
ui::EventDispatcher::DispatchEvent()
ui::EventDispatcher::ProcessEvent()
ui::EventDispatcherDelegate::DispatchEvent()
aura::RootWindow::ProcessEvent()
aura::RootWindow::DispatchMouseEventToTarget()
aura::RootWindow::DispatchMouseEventImpl()
aura::RootWindow::SynthesizeMouseMoveEvent()
aura::RootWindow::UpdateCapture()
ash::internal::CaptureController::SetCapture()
views::MenuHost::HideMenuHost()
views::SubmenuView::Hide()
views::MenuController::CommitPendingSelection()
views::MenuController::SetSelection()
views::MenuController::Cancel()
RenderViewContextMenuViews::PlatformCancel()
[...]
views::MenuController::RunMessageLoop()
views::MenuController::Run()

[*] If the |synthesize_mouse_move_| flag in RootWindow is set to false,
SynthesizeMouseMoveEvent() immediately returns again. Now, the only way
that flag gets set to true is in PostMouseMoveEventAfterWindowChange(),
*which also posts a task to SynthesizeMouseMoveEvent()*. That means that we
only synthesize an event if there's already a task scheduled that would do
that. It also explains why the tests are flaky: If for some reason the flag
is reset before SynthesizeMouseMoveEvent() runs (or if it's never set in
the first place, but I don't think that happens in a test), we don't
synthesize an event, which means we don't run into the reentrant
SetSelection() call.

[sky, 2013-05-06 19:41:35]

I like synchronously updating things. Less possibility of getting into a bad
state and easier to reason about. Can we make the menu ignore these events? Menu
knows it's shutting things down.

[Bernhard Bauer, 2013-05-06 22:14:00]

On 2013/05/06 19:41:35, sky wrote:
> I like synchronously updating things. Less possibility of getting into a bad
> state and easier to reason about. Can we make the menu ignore these events?
Menu
> knows it's shutting things down.

Yeah, that could work (and I'll look into it tomorrow), but it still seems wrong
to me to call SynthesizeMouseMoveEvent() if that method is guarded by
|synthesize_mouse_move_|. I don't know how severe that is though.

[Bernhard Bauer, 2013-05-07 13:51:24]

On 2013/05/06 19:41:35, sky wrote:
> I like synchronously updating things. Less possibility of getting into a bad
> state and easier to reason about. Can we make the menu ignore these events?
Menu
> knows it's shutting things down.

Done.

[sky, 2013-05-07 14:47:26]

LGTM

[commit-bot: I haz the power, 2013-05-07 14:53:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/14818007/46001

[commit-bot: I haz the power, 2013-05-07 15:08:45]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-07 15:09:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/14818007/46001

[commit-bot: I haz the power, 2013-05-07 22:13:41]

Change committed as 198811