Sets X11WholeScreenMoveLoop as OverrideDispatcher to get events even before grab is granted

Sets X11WholeScreenMoveLoop as OverrideDispatcher to get events even before grab is granted.

BUG=363623
TEST=Quickly drag tab from a browser with 2 tabs immediately releasing the mouse

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266382

[varkha, 2014-04-25 02:02:28]

sadrul@, I am posting this more as an illustration but I would like your input.
Do you think this (or a similar) approach would work to fix the bug in M35? This
certainly resolves the bug but introduces too many assumptions (e.g. single move
loop) which don't look pretty.
I think the right thing to do would be to create a dedicated capture window and
use it always when we need a pointer grab. That would then redirect the events
to appropriate hosts that we could swap synchronously and so while the initial
grab would still be asynchronous, shifting it from one host to another would be
synchronous. This will prevent the race conditions that we have now.
Thanks!

[sadrul, 2014-04-25 11:34:03]

On 2014/04/25 02:02:28, varkha wrote:
> sadrul@, I am posting this more as an illustration but I would like your
input.
> Do you think this (or a similar) approach would work to fix the bug in M35?
This
> certainly resolves the bug but introduces too many assumptions (e.g. single
move
> loop) which don't look pretty.
> I think the right thing to do would be to create a dedicated capture window
and
> use it always when we need a pointer grab. That would then redirect the events
> to appropriate hosts that we could swap synchronously and so while the initial
> grab would still be asynchronous, shifting it from one host to another would
be
> synchronous. This will prevent the race conditions that we have now.
> Thanks!

Can you explain more what is causing the bug? Is it the case that the
mouse-release is sent to the wrong window, and not the grab-window?

[varkha, 2014-04-25 11:40:30]

On 2014/04/25 11:34:03, sadrul wrote:
> On 2014/04/25 02:02:28, varkha wrote:
> > sadrul@, I am posting this more as an illustration but I would like your
> input.
> > Do you think this (or a similar) approach would work to fix the bug in M35?
> This
> > certainly resolves the bug but introduces too many assumptions (e.g. single
> move
> > loop) which don't look pretty.
> > I think the right thing to do would be to create a dedicated capture window
> and
> > use it always when we need a pointer grab. That would then redirect the
events
> > to appropriate hosts that we could swap synchronously and so while the
initial
> > grab would still be asynchronous, shifting it from one host to another would
> be
> > synchronous. This will prevent the race conditions that we have now.
> > Thanks!
> 
> Can you explain more what is causing the bug? Is it the case that the
> mouse-release is sent to the wrong window, and not the grab-window?

Yes, as well as a few mouse move events before and (less importantly) after the
release. The capture is first set to the torn off window, then switched to the
move loop, but it takes some time to switch during witch the events are
dispatched by a wrong window.

[sadrul, 2014-04-25 12:00:04]

On 2014/04/25 11:40:30, varkha wrote:
> On 2014/04/25 11:34:03, sadrul wrote:
> > On 2014/04/25 02:02:28, varkha wrote:
> > > sadrul@, I am posting this more as an illustration but I would like your
> > input.
> > > Do you think this (or a similar) approach would work to fix the bug in
M35?
> > This
> > > certainly resolves the bug but introduces too many assumptions (e.g.
single
> > move
> > > loop) which don't look pretty.
> > > I think the right thing to do would be to create a dedicated capture
window
> > and
> > > use it always when we need a pointer grab. That would then redirect the
> events
> > > to appropriate hosts that we could swap synchronously and so while the
> initial
> > > grab would still be asynchronous, shifting it from one host to another
would
> > be
> > > synchronous. This will prevent the race conditions that we have now.
> > > Thanks!
> > 
> > Can you explain more what is causing the bug? Is it the case that the
> > mouse-release is sent to the wrong window, and not the grab-window?
> 
> Yes, as well as a few mouse move events before and (less importantly) after
the
> release. The capture is first set to the torn off window, then switched to the
> move loop, but it takes some time to switch during witch the events are
> dispatched by a wrong window.

Is the capture switch from |source->GetHost()| to the |grab_input_window_| at
the root if the issue? Are the incorrectly dispatched events dispatched to
|source->GetHost()|? I am thinking a better solution would be for
X11WholeScreenMoveLoop::CanDispatchEvent() to always return true.
X11WholeScreenMoveLoop::DispatchEvent() would process all events targeted to
|grab_input_window_|, and all mouse/key events targeted to any window. Would
that work?

[varkha, 2014-04-25 13:04:34]

On 2014/04/25 12:00:04, sadrul wrote:
> On 2014/04/25 11:40:30, varkha wrote:
> > On 2014/04/25 11:34:03, sadrul wrote:
> > > On 2014/04/25 02:02:28, varkha wrote:
> > > > sadrul@, I am posting this more as an illustration but I would like your
> > > input.
> > > > Do you think this (or a similar) approach would work to fix the bug in
> M35?
> > > This
> > > > certainly resolves the bug but introduces too many assumptions (e.g.
> single
> > > move
> > > > loop) which don't look pretty.
> > > > I think the right thing to do would be to create a dedicated capture
> window
> > > and
> > > > use it always when we need a pointer grab. That would then redirect the
> > events
> > > > to appropriate hosts that we could swap synchronously and so while the
> > initial
> > > > grab would still be asynchronous, shifting it from one host to another
> would
> > > be
> > > > synchronous. This will prevent the race conditions that we have now.
> > > > Thanks!
> > > 
> > > Can you explain more what is causing the bug? Is it the case that the
> > > mouse-release is sent to the wrong window, and not the grab-window?
> > 
> > Yes, as well as a few mouse move events before and (less importantly) after
> the
> > release. The capture is first set to the torn off window, then switched to
the
> > move loop, but it takes some time to switch during witch the events are
> > dispatched by a wrong window.
> 
> Is the capture switch from |source->GetHost()| to the |grab_input_window_| at
> the root if the issue? Are the incorrectly dispatched events dispatched to
> |source->GetHost()|? I am thinking a better solution would be for
> X11WholeScreenMoveLoop::CanDispatchEvent() to always return true.
> X11WholeScreenMoveLoop::DispatchEvent() would process all events targeted to
> |grab_input_window_|, and all mouse/key events targeted to any window. Would
> that work?

The root cause is that events are (correctly from X point of view) dispatched to
the window that they are sent to which is the window that X thinks has the
pointer grab. Since the drag_input_window in the move loop is created off screen
it would not be targeted unless it has a grab and the grab is established
asynchronously some time after it is requested. Mouse events are completely
asynchronous of the grab handshake and some arrive to the client before (and
some are even sent or are in the client queue before) the server even knows that
new grab is requested so they arrive at the client with target window set to the
host under the mouse or to the host that has capture (and then dispatched to
that host). The drag_input_window does not even has a chance until the server
receives and grants the grab request. The server then notifies the host with
NotifyGrab mode set in EnterNotify or in LeaveNotify and from that point on the
events arrive to the client targeting the drag_input_window.
So it seems that since all dispatchers are notified your approach should work.
CanDispatchEvent would return true after the drag_input_window gets created and
DispatchEvent will return POST_DISPATCH_STOP_PROPAGATION only for mouse events.
I'll try that.

[varkha, 2014-04-25 13:14:11]

sadrul@, I think I will also need to not return POST_DISPATCH_STOP_PROPAGATION
in DesktopWindowTreeHostX11::DispatchTouchEvent because that dispatcher will get
called first, no? And I think I can only do that if the DesktopWindowTreeHostX11
knows that there is an active move loop, possibly created by another instance of
the DesktopWindowTreeHostX11. Am I missing something?

[sadrul, 2014-04-25 13:28:04]

On 2014/04/25 13:14:11, varkha wrote:
> sadrul@, I think I will also need to not return POST_DISPATCH_STOP_PROPAGATION
> in DesktopWindowTreeHostX11::DispatchTouchEvent because that dispatcher will
get
> called first, no? And I think I can only do that if the
DesktopWindowTreeHostX11
> knows that there is an active move loop, possibly created by another instance
of
> the DesktopWindowTreeHostX11. Am I missing something?

I think it makes sense to have the move-loop install itself as the
override-dispatcher (using OverrideDispatcher() instead of
AddPlatformEventDispatcher()). That will force all events to go through the
move-loop first, and so we avoid the issue of other dispatchers processing the
event earlier than the move-loop. We use the override-dispatcher for other
similar cases where spin up a nested message-loop (e.g. in menus).

[varkha, 2014-04-25 17:10:38]

sadrul@, I still want to clean it up, but I think this follows your general idea
(and it works). OverrideDispatcher was introduced in
https://codereview.chromium.org/203483004 and landed in r259417. M35 branched
off in 260298 so it should have it. Not sure about the crash
http://crbug.com/360477 - was it M36-specific and so not merged? PTAL.

[sadrul, 2014-04-25 17:19:50]

On 2014/04/25 17:10:38, varkha wrote:
> sadrul@, I still want to clean it up, but I think this follows your general
idea
> (and it works). OverrideDispatcher was introduced in
> https://codereview.chromium.org/203483004 and landed in r259417.

Right. But the PlatformEventDispatcher mechanism was not yet used. The switch to
using that in views/aura happened in http://crrev.com/262008

> M35 branched
> off in 260298 so it should have it. Not sure about the crash
> http://crbug.com/360477 - was it M36-specific and so not merged? PTAL.

https://codereview.chromium.org/254573002/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/254573002/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:159:
DispatchEvent(&xevent);
It would be really great if we didn't have to recreate an XEvent here.

[varkha, 2014-04-25 18:00:35]

https://codereview.chromium.org/254573002/diff/40001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/254573002/diff/40001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:159:
DispatchEvent(&xevent);
On 2014/04/25 17:19:50, sadrul wrote:
> It would be really great if we didn't have to recreate an XEvent here.

We talked about this offline. I think it would be more complicated than it
already is because of the way we post a task to throttle the dispatch of the
motion events.

[varkha, 2014-04-25 18:22:31]

sadrul@, I've cleaned it up a bit. PTAL.

[sadrul, 2014-04-25 19:47:39]

LGTM

(We should prioritize adding tests for the x11 related code in desktop_aura/)

https://codereview.chromium.org/254573002/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/254573002/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:90: // the grab is
granted by X.
Update the comment explaining why we need to do this.

[varkha, 2014-04-25 19:56:41]

Thanks for the direction! Tab dragging tests are now getting enabled in Linux -
initially in https://codereview.chromium.org/259513003/.

https://codereview.chromium.org/254573002/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc (right):

https://codereview.chromium.org/254573002/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/x11_whole_screen_move_loop.cc:90: // the grab is
granted by X.
On 2014/04/25 19:47:40, sadrul wrote:
> Update the comment explaining why we need to do this.

Done.

[varkha, 2014-04-25 19:56:59]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/254573002/80001

[commit-bot: I haz the power, 2014-04-25 23:13:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:13:57]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg
  tryserver.chromium on linux_chromium_rel

[varkha, 2014-04-25 23:19:40]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:21:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/254573002/80001

[commit-bot: I haz the power, 2014-04-26 22:17:10]

Change committed as 266382