[Ozone/x11] events can be dispatched to the wrong window

[Ozone/x11] events can be dispatched to the wrong window

When multiple X11Window/X11WindowOzone instances are created, each
registers itself in PlatformEventSource as a "platform event dispatcher".

Before dispatching an event to a specific window, PlatformEventSource
iterates over the list of registered dispatchers, "asking" each dispatcher
whether the event can be dispatch to it.

In case of non-Ozone X11 windows, X11Window::CanDispatchEvent is called
receiving a XEvent* parameter, and checks whether the xwindow targeted
by the event is the same as the xwindow it wraps (see ::IsEventForXWindow).
If so it dispatches the event to this X11Window instance.

In case of Ozone X11 windows, X11WindowOzone::CanDispatchEvent is called
receiving a ui::Event* parameter, and it is not possible to access its
associated NativeEvent*, because event was manually constructed in
EventSourceLibevent::ProcessXEvent.
Hence, ::CanDispatchEvent does not check if it is the event's target
window, but instead whether the event location is contained in its bounds.

This is fragile: if there are two overlapping X11WindowOzone instances,
the first one that registered itself in PlatformEventSource will be
the one receiving mouse events in the overlapping area, even if it is
not the foreground window.

CL fixes it by making it possible to pass the original ui::PlatformEvent
event alongside with the ui::Event instance created off of it, which allows
X11WindowOzone::CanDispatchEvent to minic the check that both
X11WindowOzone and DesktopWindowTreeHostX11 do.

BUG=707406

[tonikitoo, 2017-03-31 21:58:43]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 21:59:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 22:38:20]

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

[commit-bot: I haz the power, 2017-03-31 22:38:21]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-04-03 04:26:40]

tonikitoo@igalia.com changed reviewers:
+ kylechar@chromium.org, sadrul@chromium.org

[tonikitoo, 2017-04-03 04:26:41]

Sadrul/Kylechar: This CL is a first attempt to fix the issue.
I would like to know from you guys what you think or whether you have a better
approach to tackle it.

[fwang, 2017-04-03 09:36:57]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[fwang, 2017-04-03 09:36:57]

This approach l g t m.

(I guess the only not-so-nice thing is that a PlatformEvent is now cast to a
EventWithPlatformEvent which makes the variable names a bit confusing.)

https://codereview.chromium.org/2791693003/diff/1/ui/platform_window/x11/x11_...
File ui/platform_window/x11/x11_window_ozone.cc (right):

https://codereview.chromium.org/2791693003/diff/1/ui/platform_window/x11/x11_...
ui/platform_window/x11/x11_window_ozone.cc:95: auto* event =
static_cast<ui::Event*>(ewpe->event);
I guess you want DCHECK(event) here.

[kylechar, 2017-04-03 13:13:45]

We need sadrul@ to chime in here but I'm not totally sure this approach will
work. I think there PlatformEventObservers that assume the PlatformEvent is of
type ui::Event.

[sadrul, 2017-04-04 04:02:07]

https://codereview.chromium.org/2791693003/diff/1/ui/events/platform/x11/x11_...
File ui/events/platform/x11/x11_event_source_libevent.cc (right):

https://codereview.chromium.org/2791693003/diff/1/ui/events/platform/x11/x11_...
ui/events/platform/x11/x11_event_source_libevent.cc:162: DispatchEvent(&ewxe);
This is fairly hacky.

Maybe this should always go through the XEventDispatcher, or negotiate with the
XEventDispatchers to find the real target first?

[tonikitoo, 2017-04-04 04:07:31]

Thanks for looking kyle/sadrul/fred.

On 2017/04/04 04:02:07, sadrul wrote:
>
https://codereview.chromium.org/2791693003/diff/1/ui/events/platform/x11/x11_...
> File ui/events/platform/x11/x11_event_source_libevent.cc (right):
> 
>
https://codereview.chromium.org/2791693003/diff/1/ui/events/platform/x11/x11_...
> ui/events/platform/x11/x11_event_source_libevent.cc:162: DispatchEvent(&ewxe);
> This is fairly hacky.

Right, this ain't to be a final solution. The CL was an attempt to make it
explicit what the problem was.

> Maybe this should always go through the XEventDispatcher, or negotiate with
the
> XEventDispatchers to find the real target first?

Alternatively, we could to the "XEvent -> ui::Event" conversion in
x11_window_ozone.cc instead of in x11_event_source_libevent.cc?