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

ui/events/event_processor.cc

Index: ui/events/event_processor.cc
diff --git a/ui/events/event_processor.cc b/ui/events/event_processor.cc
index 3077cc85b168aea483a9e506021af80b5e64375e..589973dc560f89acc099b72a74e5b14d22988ea8 100644
--- a/ui/events/event_processor.cc
+++ b/ui/events/event_processor.cc
@@ -17,9 +17,28 @@ EventDispatchDetails EventProcessor::OnEventFromSource(Event* event) {
 
   PrepareEventForDispatch(event);
   EventTarget* target = targeter->FindTargetForEvent(root, event);
+  scoped_ptr<Event> event_copy(Event::Clone(*event));

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

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.

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

Done.

 
   while (target) {
-    EventDispatchDetails details = DispatchEvent(target, event);
+    EventDispatchDetails details;
+    if (event->phase() != EP_PREDISPATCH) {
+      // |event| is currently being processed by another EventProcessor, which
+      // expects the event's state (e.g., target, phase, etc.) to remain
+      // unchanged once |this| has finished its nested processing (with the
+      // exception of whether or not the event was handled). Thus dispatch
+      // a copy of the event instead.
+      details = DispatchEvent(target, event_copy.get());
+
+      if (event_copy->stopped_propagation())
+        event->StopPropagation();
+      else if (event_copy->handled())
+        event->SetHandled();
+    } else {
+      // |this| is the only EventProcessor currently processing |event|, so
+      // copying the event prior to dispatch is unnecessary.
+      details = DispatchEvent(target, event);

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

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]

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).
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?

+    }
+
     if (details.dispatcher_destroyed ||
         details.target_destroyed ||
         event->handled()) {