Do not mutate ui::Event properties during nested event processing

Do not mutate ui::Event properties during nested event processing

If an EventProcessor starts to process a ui::Event while
it is already being processed by another EventProcessor,
then the properties of the event (e.g., target, phase)
should not be mutated by the inner processor before
control is returned to the outer processor. The exception
being whether or not the event has been handled.

To guarantee this, the inner processor should dispatch
a copy of the event rather than the original event
itself. This CL also introduces the static
ui::Event::Clone() method, which is used to create a
copy of a ui::Event.

BUG=404232
TEST=EventProcessorTest.NestedEventProcessing,
     updated WindowManagerTest.Focus

Committed: https://crrev.com/88d8e4e3f6ced79844bf7a296bb5b9d6ac0ef4f5
Cr-Commit-Position: refs/heads/master@{#293410}

[tdanderson, 2014-08-29 20:18:48]

tdanderson@chromium.org changed reviewers:
+ kpschoedel@chromium.org, sadrul@chromium.org

[tdanderson, 2014-08-29 20:18:49]

Sadrul and Kevin, can you please take a look? As-is, I am making the assumption
that the default copy constructors for the event types are sufficient (with the
exception of KeyEvent which actually defines its own).

[kpschoedel, 2014-08-29 20:49:17]

On 2014/08/29 20:18:49, tdanderson wrote:
> Sadrul and Kevin, can you please take a look? As-is, I am making the
assumption
> that the default copy constructors for the event types are sufficient (with
the
> exception of KeyEvent which actually defines its own).

The KeyEvent copy constructor was just added a few days ago for mojo and hasn't
been exercised much. It doesn't copy the native event, so your patch might
expose some dependencies on that.

[kpschoedel, 2014-08-29 20:51:29]

On 2014/08/29 20:49:17, kpschoedel wrote:
> It doesn't copy the native event, so your patch might
> expose some dependencies on that.

Disregard that; it DOES copy the native event in the Event copy constructor.

[sadrul, 2014-08-29 21:12:17]

https://codereview.chromium.org/519113002/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/519113002/diff/1/ui/events/event.h#newcode219
ui/events/event.h:219: virtual scoped_ptr<Event> Clone() const = 0;
Can we have a static Event::Clone(const Event& event); method that does the
right thing, e.g.:
  scoped_ptr<Event> Event::Clone(const Event& event) {
    if (event.IsMouseEvent())
      return scoped_ptr<Event>(new MouseEvent(cast<>(event)));
    ....

  }
etc.?

https://codereview.chromium.org/519113002/diff/1/ui/events/event_processor.cc
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/1/ui/events/event_processor.cc...
ui/events/event_processor.cc:30: details = DispatchEvent(target,
event_copy.get());
I think we want to clone the event before going into this loop?

[tdanderson, 2014-09-02 22:57:09]

Can you please take another look?

Note that I updated WindowManagerTest.Focus to not dispatch the same event more
than once, otherwise the event's phase information is incorrect for subsequent
dispatches and the nested-processing code path is hit when it shouldn't be.

https://codereview.chromium.org/519113002/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/519113002/diff/1/ui/events/event.h#newcode219
ui/events/event.h:219: virtual scoped_ptr<Event> Clone() const = 0;
On 2014/08/29 21:12:17, sadrul wrote:
> Can we have a static Event::Clone(const Event& event); method that does the
> right thing, e.g.:
>   scoped_ptr<Event> Event::Clone(const Event& event) {
>     if (event.IsMouseEvent())
>       return scoped_ptr<Event>(new MouseEvent(cast<>(event)));
>     ....
> 
>   }
> etc.?

Done.

https://codereview.chromium.org/519113002/diff/1/ui/events/event_processor.cc
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/1/ui/events/event_processor.cc...
ui/events/event_processor.cc:30: details = DispatchEvent(target,
event_copy.get());
On 2014/08/29 21:12:17, sadrul wrote:
> I think we want to clone the event before going into this loop?

Done.

[sadrul, 2014-09-03 16:23:53]

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
ui/events/event_processor.cc:20: scoped_ptr<Event>
event_copy(Event::Clone(*event));
I was thinking of something more like this:
  scoped_ptr<Event> event_copy;
  if (event->phase() != EP_PREDISPATCH)
    event_copy = Event::Clone(*event);
  while (target) {
    if (event_copy) {
      ...
    } else {
      ...
    }
  }

i.e. do the clone only if needed.

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
ui/events/event_processor.cc:39: details = DispatchEvent(target, event);
DispatchEvent() would set the event-phase to POSTDISPATCH. So you would end up
dispatching event_copy in the next iteration, and send event_copy with phase ==
POSTDISPATCH in the iteration after that.

With this change, let's add a CHECK in EventDispatcherDelegate::DispatchEvent
that event->phase() == PREDISPATCH

[tdanderson, 2014-09-03 20:20:02]

Sadrul, can you please take a look at the next patch set and my inline comments?

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
ui/events/event_processor.cc:20: scoped_ptr<Event>
event_copy(Event::Clone(*event));
On 2014/09/03 16:23:53, sadrul wrote:
> I was thinking of something more like this:
>   scoped_ptr<Event> event_copy;
>   if (event->phase() != EP_PREDISPATCH)
>     event_copy = Event::Clone(*event);
>   while (target) {
>     if (event_copy) {
>       ...
>     } else {
>       ...
>     }
>   }
> 
> i.e. do the clone only if needed.

Done.

https://codereview.chromium.org/519113002/diff/60001/ui/events/event_processo...
ui/events/event_processor.cc:39: details = DispatchEvent(target, event);
On 2014/09/03 16:23:53, sadrul wrote:
> DispatchEvent() would set the event-phase to POSTDISPATCH. So you would end up
> dispatching event_copy in the next iteration, and send event_copy with phase
==
> POSTDISPATCH in the iteration after that.

Thanks for catching this! In the next patch set I believe we will always do the
right thing: for any invocation of OnEventFromSource(), we'll either always
enter the if at each loop iteration (if this invocation is doing nested
processing) or always enter the else at each loop iteration (if this invocation
is not doing nested processing).

> 
> With this change, let's add a CHECK in EventDispatcherDelegate::DispatchEvent
> that event->phase() == PREDISPATCH

This check will fail on ToT, and it will also fail if I were to add it in the
next patch set. DispatchEvent() is called with an event having a phase of
POSTDISPATCH at each iteration of the while loop except for the first. But since
the first thing EDD::DispatchEvent() does is reset the phase of the event to
PREDISPATCH, I don't think this will present a problem.

Are you concerned that attributes of the event other than the phase will be
incorrect before re-dispatching when we allow bubbling?

[sadrul, 2014-09-04 15:45:06]

LGTM

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
ui/events/event_processor.cc:22: // EventProcessor.
If the event is in the process of being dispatched, or has already been
dispatched, then dispatch a copy of the event instead.

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
ui/events/event_processor.cc:33: // |event_copy| instead of |event|.
You don't need this comment. The comment above in lines 21:22 is sufficient.

[tdanderson, 2014-09-04 17:03:12]

tdanderson@chromium.org changed reviewers:
+ flackr@chromium.org

[tdanderson, 2014-09-04 17:03:12]

Rob, can you please review the change in ash/ ?

[tdanderson, 2014-09-04 17:19:03]

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
ui/events/event_processor.cc:22: // EventProcessor.
On 2014/09/04 15:45:06, sadrul wrote:
> If the event is in the process of being dispatched, or has already been
> dispatched, then dispatch a copy of the event instead.

Done.

https://codereview.chromium.org/519113002/diff/80001/ui/events/event_processo...
ui/events/event_processor.cc:33: // |event_copy| instead of |event|.
On 2014/09/04 15:45:06, sadrul wrote:
> You don't need this comment. The comment above in lines 21:22 is sufficient.

Done.

[flackr, 2014-09-04 17:42:38]

ash/ LGTM

[tdanderson, 2014-09-04 18:19:27]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:21:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/519113002/100001

[commit-bot: I haz the power, 2014-09-04 23:18:46]

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

[commit-bot: I haz the power, 2014-09-04 23:18:47]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdanderson, 2014-09-05 01:11:40]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-09-05 01:15:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/519113002/100001

[commit-bot: I haz the power, 2014-09-05 02:45:29]

Committed patchset #6 (id:100001) as 5e4ae076275c91a23d69eabaaed3876f3a8a7626

[commit-bot: I haz the power, 2014-09-10 03:35:53]

Patchset 6 (id:??) landed as
https://crrev.com/88d8e4e3f6ced79844bf7a296bb5b9d6ac0ef4f5
Cr-Commit-Position: refs/heads/master@{#293410}