Don't refcount tracking id -> slot id mapping.

Previously we tried to refcount the tracking id to slot id mapping.
This broke in some circumstances where the number of press events was
not equal to the number of release events.

This patch switches to marking some touch events such that they don't
modify the mapping, simplifying logic, and fixing a nasty bug.

BUG=439051
TEST=EventsXTest.TouchEventNotRemovingFromNativeMapping

Review URL: https://codereview.chromium.org/785753002

Cr-Commit-Position: refs/heads/master@{#313520}

ui/events/cocoa/events_mac.mm

@@ -160,10 +160,6 @@ void ReleaseCopiedNativeEvent(const base::NativeEvent& event) {
   [event release];
 }
 
-void IncrementTouchIdRefCount(const base::NativeEvent& native_event) {
-  NOTIMPLEMENTED();
-}
-
 void ClearTouchIdIfReleased(const base::NativeEvent& native_event) {
   NOTIMPLEMENTED();
 }

ui/events/devices/x11/touch_factory_x11.cc

@@ -250,14 +250,8 @@ int TouchFactory::GetSlotForTrackingID(uint32 tracking_id) {
   return id_generator_.GetGeneratedID(tracking_id);
 }
 
-void TouchFactory::AcquireSlotForTrackingID(uint32 tracking_id) {
-  tracking_id_refcounts_[tracking_id]++;
-}
-
 void TouchFactory::ReleaseSlotForTrackingID(uint32 tracking_id) {
-  tracking_id_refcounts_[tracking_id]--;
-  if (tracking_id_refcounts_[tracking_id] == 0)
-    id_generator_.ReleaseNumber(tracking_id);
+  id_generator_.ReleaseNumber(tracking_id);
 }
 
 bool TouchFactory::IsTouchDevicePresent() {
@@ -270,7 +264,6 @@ void TouchFactory::ResetForTest() {
   touch_events_disabled_ = false;
   touch_device_list_.clear();
   touchscreen_ids_.clear();
-  tracking_id_refcounts_.clear();
   id_generator_.ResetForTest();
 }
 

ui/events/devices/x11/touch_factory_x11.h

@@ -67,10 +67,6 @@ class EVENTS_DEVICES_EXPORT TouchFactory {
   // isn't one already, allocates a new slot ID and sets up the mapping.
   int GetSlotForTrackingID(uint32 tracking_id);
 
-  // Increases the number of times |ReleaseSlotForTrackingID| needs to be called
-  // on a given tracking id before it will actually be released.
-  void AcquireSlotForTrackingID(uint32 tracking_id);
-
   // Releases the slot ID mapping to tracking ID.
   void ReleaseSlotForTrackingID(uint32 tracking_id);
 
@@ -130,10 +126,6 @@ class EVENTS_DEVICES_EXPORT TouchFactory {
   // Touch screen <vid, pid>s.
   std::set<std::pair<int, int> > touchscreen_ids_;
 
-  // Maps from a tracking id to the number of times |ReleaseSlotForTrackingID|
-  // must be called before the tracking id is released.
-  std::map<uint32, int> tracking_id_refcounts_;
-
   // Device ID of the virtual core keyboard.
   int virtual_core_keyboard_device_;
 

ui/events/event.cc

@@ -526,19 +526,17 @@ TouchEvent::TouchEvent(const base::NativeEvent& native_event)
       radius_y_(GetTouchRadiusY(native_event)),
       rotation_angle_(GetTouchAngle(native_event)),
       force_(GetTouchForce(native_event)),
-      may_cause_scrolling_(false) {
-  latency()->AddLatencyNumberWithTimestamp(
-      INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT,
-      0,
-      0,
-      base::TimeTicks::FromInternalValue(time_stamp().ToInternalValue()),
-      1);
+      should_remove_native_touch_id_mapping_(false) {
+  if (type() == ET_TOUCH_RELEASED || type() == ET_TOUCH_CANCELLED)
+    should_remove_native_touch_id_mapping_ = true;
 
   latency()->AddLatencyNumber(INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
-  fixRotationAngle();
+  FixRotationAngle();
 
-  if (type() == ET_TOUCH_PRESSED)
-    IncrementTouchIdRefCount(native_event);
+  latency()->AddLatencyNumberWithTimestamp(
+      INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT, 0, 0,
+      base::TimeTicks::FromInternalValue(time_stamp().ToInternalValue()), 1);
+  latency()->AddLatencyNumber(INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
 }
 
 TouchEvent::TouchEvent(EventType type,
@@ -552,7 +550,7 @@ TouchEvent::TouchEvent(EventType type,
       radius_y_(0.0f),
       rotation_angle_(0.0f),
       force_(0.0f),
-      may_cause_scrolling_(false) {
+      should_remove_native_touch_id_mapping_(false) {
   latency()->AddLatencyNumber(INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
 }
 
@@ -572,17 +570,20 @@ TouchEvent::TouchEvent(EventType type,
       radius_y_(radius_y),
       rotation_angle_(angle),
       force_(force),
-      may_cause_scrolling_(false) {
+      should_remove_native_touch_id_mapping_(false) {
   latency()->AddLatencyNumber(INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
-  fixRotationAngle();
+  FixRotationAngle();
 }
 
 TouchEvent::~TouchEvent() {
   // In ctor TouchEvent(native_event) we call GetTouchId() which in X11
   // platform setups the tracking_id to slot mapping. So in dtor here,
   // if this touch event is a release event, we clear the mapping accordingly.
-  if (HasNativeEvent())
-    ClearTouchIdIfReleased(native_event());
+  if (should_remove_native_touch_id_mapping_) {
+    DCHECK(type() == ET_TOUCH_RELEASED || type() == ET_TOUCH_CANCELLED);
+    if (type() == ET_TOUCH_RELEASED || type() == ET_TOUCH_CANCELLED)
+      ClearTouchIdIfReleased(native_event());
+  }
 }
 
 void TouchEvent::UpdateForRootTransform(
@@ -603,7 +604,7 @@ void TouchEvent::DisableSynchronousHandling() {
       static_cast<EventResult>(result() | ER_DISABLE_SYNC_HANDLING));
 }
 
-void TouchEvent::fixRotationAngle() {
+void TouchEvent::FixRotationAngle() {
   while (rotation_angle_ < 0)
     rotation_angle_ += 180;
   while (rotation_angle_ >= 180)

ui/events/event.h

@@ -212,7 +212,7 @@ class EVENTS_EXPORT Event {
   void StopPropagation();
   bool stopped_propagation() const { return !!(result_ & ER_CONSUMED); }
 
-
+  // Marks the event as having been handled. A handled event does not reach the
   // next event phase. For example, if an event is handled during the pre-target
   // phase, then the event is dispatched to all pre-target handlers, but not to
   // the target or post-target handlers.
@@ -225,9 +225,6 @@ class EVENTS_EXPORT Event {
   Event(const base::NativeEvent& native_event, EventType type, int flags);
   Event(const Event& copy);
   void SetType(EventType type);
-  void set_delete_native_event(bool delete_native_event) {
-    delete_native_event_ = delete_native_event;
-  }
   void set_cancelable(bool cancelable) { cancelable_ = cancelable; }
 
   void set_time_stamp(const base::TimeDelta& time_stamp) {
@@ -495,7 +492,7 @@ class EVENTS_EXPORT TouchEvent : public LocatedEvent {
         radius_y_(model.radius_y_),
         rotation_angle_(model.rotation_angle_),
         force_(model.force_),
-        may_cause_scrolling_(model.may_cause_scrolling_) {}
+        should_remove_native_touch_id_mapping_(false) {}
 
   TouchEvent(EventType type,
              const gfx::PointF& location,
@@ -532,6 +529,12 @@ class EVENTS_EXPORT TouchEvent : public LocatedEvent {
   void set_radius_x(const float r) { radius_x_ = r; }
   void set_radius_y(const float r) { radius_y_ = r; }
 
+  void set_should_remove_native_touch_id_mapping(
+      bool should_remove_native_touch_id_mapping) {
+    should_remove_native_touch_id_mapping_ =
+        should_remove_native_touch_id_mapping;
+  }
+
   // Overridden from LocatedEvent.
   void UpdateForRootTransform(
       const gfx::Transform& inverted_root_transform) override;
@@ -544,7 +547,7 @@ class EVENTS_EXPORT TouchEvent : public LocatedEvent {
 
  private:
   // Adjusts rotation_angle_ to within the acceptable range.
-  void fixRotationAngle();
+  void FixRotationAngle();
 
   // The identity (typically finger) of the touch starting at 0 and incrementing
   // for each separable additional touch that the hardware can detect.
@@ -570,6 +573,12 @@ class EVENTS_EXPORT TouchEvent : public LocatedEvent {
   // touchmove that exceeds the platform slop region, or a touchend that
   // causes a fling). Defaults to false.
   bool may_cause_scrolling_;
+
+  // True if this event should remove the mapping between the native
+  // event id and the touch_id_. This should only be the case for
+  // release and cancel events where the associated touch press event
+  // created a mapping between the native id and the touch_id_.
+  bool should_remove_native_touch_id_mapping_;
 };
 
 // An interface that individual platforms can use to store additional data on

ui/events/event_utils.h

@@ -116,11 +116,6 @@ void ReleaseCopiedNativeEvent(
 // Gets the touch id from a native event.
 EVENTS_EXPORT int GetTouchId(const base::NativeEvent& native_event);
 
-// Increases the number of times |ClearTouchIdIfReleased| needs to be called on
-// an event with a given touch id before it will actually be cleared.
-EVENTS_EXPORT void IncrementTouchIdRefCount(
-    const base::NativeEvent& native_event);
-
 // Clear the touch id from bookkeeping if it is a release/cancel event.
 EVENTS_EXPORT void ClearTouchIdIfReleased(
     const base::NativeEvent& native_event);

ui/events/events_stub.cc

@@ -70,10 +70,6 @@ base::NativeEvent CopyNativeEvent(const base::NativeEvent& event) {
 void ReleaseCopiedNativeEvent(const base::NativeEvent& event) {
 }
 
-void IncrementTouchIdRefCount(const base::NativeEvent& native_event) {
-  NOTIMPLEMENTED();
-}
-
 void ClearTouchIdIfReleased(const base::NativeEvent& native_event) {
   NOTIMPLEMENTED();
 }

ui/events/ozone/events_ozone.cc

@@ -86,9 +86,6 @@ base::NativeEvent CopyNativeEvent(const base::NativeEvent& event) {
 void ReleaseCopiedNativeEvent(const base::NativeEvent& event) {
 }
 
-void IncrementTouchIdRefCount(const base::NativeEvent& event) {
-}
-
 void ClearTouchIdIfReleased(const base::NativeEvent& xev) {
 }
 

ui/events/win/events_win.cc

@@ -299,10 +299,6 @@ base::NativeEvent CopyNativeEvent(const base::NativeEvent& event) {
 void ReleaseCopiedNativeEvent(const base::NativeEvent& event) {
 }
 
-void IncrementTouchIdRefCount(const base::NativeEvent& event) {
-  NOTIMPLEMENTED();
-}
-
 void ClearTouchIdIfReleased(const base::NativeEvent& xev) {
   NOTIMPLEMENTED();
 }

ui/events/x/events_x.cc

@@ -739,18 +739,6 @@ void ReleaseCopiedNativeEvent(const base::NativeEvent& event) {
   delete event;
 }
 
-void IncrementTouchIdRefCount(const base::NativeEvent& xev) {
-  ui::DeviceDataManagerX11* manager = ui::DeviceDataManagerX11::GetInstance();
-  double tracking_id;
-  if (!manager->GetEventData(
-          *xev, ui::DeviceDataManagerX11::DT_TOUCH_TRACKING_ID, &tracking_id)) {
-    return;
-  }
-
-  ui::TouchFactory* factory = ui::TouchFactory::GetInstance();
-  factory->AcquireSlotForTrackingID(tracking_id);
-}
-
 void ClearTouchIdIfReleased(const base::NativeEvent& xev) {
   ui::EventType type = ui::EventTypeFromNative(xev);
   if (type == ui::ET_TOUCH_CANCELLED ||

ui/events/x/events_x_unittest.cc

@@ -300,54 +300,47 @@ int GetTouchIdForTrackingId(uint32 tracking_id) {
   return -1;
 }
 
-TEST_F(EventsXTest, TouchEventIdRefcounting) {
+TEST_F(EventsXTest, TouchEventNotRemovingFromNativeMapping) {
   std::vector<unsigned int> devices;
   devices.push_back(0);
   ui::SetUpTouchDevicesForTest(devices);
   std::vector<Valuator> valuators;
 
-  const int kTrackingId0 = 5;
-  const int kTrackingId1 = 7;
+  const int kTrackingId = 5;
 
-  // Increment ref count once for first touch.
+  // Two touch presses with the same tracking id.
   ui::ScopedXI2Event xpress0;
   xpress0.InitTouchEvent(
-      0, XI_TouchBegin, kTrackingId0, gfx::Point(10, 10), valuators);
+      0, XI_TouchBegin, kTrackingId, gfx::Point(10, 10), valuators);
   scoped_ptr<ui::TouchEvent> upress0(new ui::TouchEvent(xpress0));
-  EXPECT_EQ(0, GetTouchIdForTrackingId(kTrackingId0));
+  EXPECT_EQ(0, GetTouchIdForTrackingId(kTrackingId));
 
-  // Increment ref count 4 times for second touch.
   ui::ScopedXI2Event xpress1;
   xpress1.InitTouchEvent(
-      0, XI_TouchBegin, kTrackingId1, gfx::Point(20, 20), valuators);
+      0, XI_TouchBegin, kTrackingId, gfx::Point(20, 20), valuators);
+  ui::TouchEvent upress1(xpress1);
+  EXPECT_EQ(0, GetTouchIdForTrackingId(kTrackingId));
 
-  for (int i = 0; i < 4; ++i) {
-    ui::TouchEvent upress1(xpress1);
-    EXPECT_EQ(1, GetTouchIdForTrackingId(kTrackingId1));
+  // The first touch release shouldn't clear the mapping from the
+  // tracking id.
+  ui::ScopedXI2Event xrelease0;
+  xrelease0.InitTouchEvent(
+      0, XI_TouchEnd, kTrackingId, gfx::Point(10, 10), valuators);
+  {
+    ui::TouchEvent urelease0(xrelease0);
+    urelease0.set_should_remove_native_touch_id_mapping(false);
   }
+  EXPECT_EQ(0, GetTouchIdForTrackingId(kTrackingId));
 
+  // The second touch release should clear the mapping from the
+  // tracking id.
   ui::ScopedXI2Event xrelease1;
   xrelease1.InitTouchEvent(
-      0, XI_TouchEnd, kTrackingId1, gfx::Point(10, 10), valuators);
-
-  // Decrement ref count 3 times for second touch.
-  for (int i = 0; i < 3; ++i) {
+      0, XI_TouchEnd, kTrackingId, gfx::Point(10, 10), valuators);
+  {
     ui::TouchEvent urelease1(xrelease1);
-    EXPECT_EQ(1, GetTouchIdForTrackingId(kTrackingId1));
   }
-
-  // This should clear the touch id of the second touch.
-  scoped_ptr<ui::TouchEvent> urelease1(new ui::TouchEvent(xrelease1));
-  urelease1.reset();
-  EXPECT_EQ(-1, GetTouchIdForTrackingId(kTrackingId1));
-
-  // This should clear the touch id of the first touch.
-  ui::ScopedXI2Event xrelease0;
-  xrelease0.InitTouchEvent(
-      0, XI_TouchEnd, kTrackingId0, gfx::Point(10, 10), valuators);
-  scoped_ptr<ui::TouchEvent> urelease0(new ui::TouchEvent(xrelease0));
-  urelease0.reset();
-  EXPECT_EQ(-1, GetTouchIdForTrackingId(kTrackingId0));
+  EXPECT_EQ(-1, GetTouchIdForTrackingId(kTrackingId));
 }
 
 TEST_F(EventsXTest, NumpadKeyEvents) {

ui/views/controls/menu/menu_controller_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/run_loop.h"
 #include "ui/aura/scoped_window_targeter.h"
 #include "ui/aura/window.h"
+#include "ui/events/event_handler.h"
 #include "ui/events/event_targeter.h"
 #include "ui/events/platform/platform_event_source.h"
 #include "ui/views/controls/menu/menu_item_view.h"
@@ -186,6 +187,14 @@ class MenuControllerTest : public ViewsTestBase {
     controller_->exit_type_ = MenuController::EXIT_ALL;
     DispatchEvent();
   }
+
+  void DispatchTouch(int evtype, int id) {
+    ui::ScopedXI2Event touch_event;
+    std::vector<ui::Valuator> valuators;
+    touch_event.InitTouchEvent(1, evtype, id, gfx::Point(10, 10), valuators);
+    event_source_.Dispatch(touch_event);
+    DispatchEvent();
+  }
 #endif
 
   void DispatchEvent() {
@@ -266,4 +275,64 @@ TEST_F(MenuControllerTest, EventTargeter) {
 }
 #endif
 
+#if defined(USE_X11)
+
+class TestEventHandler : public ui::EventHandler {
+ public:
+  TestEventHandler() : outstanding_touches_(0) {}
+
+  void OnTouchEvent(ui::TouchEvent* event) override {
+    switch(event->type()) {
+      case ui::ET_TOUCH_PRESSED:
+        outstanding_touches_++;
+        break;
+      case ui::ET_TOUCH_RELEASED:
+      case ui::ET_TOUCH_CANCELLED:
+        outstanding_touches_--;
+        break;
+      default:
+        break;
+    }
+  }
+
+  int outstanding_touches() const { return outstanding_touches_; }
+
+ private:
+  int outstanding_touches_;
+};
+
+// Tests that touch event ids are released correctly. See
+// crbug.com/439051 for details. When the ids aren't managed
+// correctly, we get stuck down touches.
+TEST_F(MenuControllerTest, TouchIdsReleasedCorrectly) {
+  scoped_ptr<Widget> owner(CreateOwnerWidget());
+  TestEventHandler test_event_handler;
+  owner->GetNativeWindow()->GetRootWindow()->AddPreTargetHandler(
+      &test_event_handler);
+
+  std::vector<unsigned int> devices;
+  devices.push_back(1);
+  ui::SetUpTouchDevicesForTest(devices);
+
+  DispatchTouch(XI_TouchBegin, 0);
+  DispatchTouch(XI_TouchBegin, 1);
+  DispatchTouch(XI_TouchEnd, 0);
+
+  message_loop()->PostTask(FROM_HERE,
+                           base::Bind(&MenuControllerTest::DispatchTouch,
+                                      base::Unretained(this), XI_TouchEnd, 1));
+
+  message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&MenuControllerTest::DispatchEscapeAndExpect,
+                 base::Unretained(this), MenuController::EXIT_OUTERMOST));
+
+  RunMenu(owner.get());
+  EXPECT_EQ(0, test_event_handler.outstanding_touches());
+
+  owner->GetNativeWindow()->GetRootWindow()->RemovePreTargetHandler(
+      &test_event_handler);
+}
+#endif // defined(USE_X11)
+
 }  // namespace views

ui/views/controls/menu/menu_event_dispatcher_linux.cc

@@ -75,6 +75,14 @@ uint32_t MenuEventDispatcher::DispatchEvent(const ui::PlatformEvent& event) {
         should_quit = false;
         should_perform_default = false;
         break;
+      case ui::ET_TOUCH_RELEASED:
+      case ui::ET_TOUCH_CANCELLED:
+        // Don't allow the event copy to clear the native touch id
+        // mapping, or we'll lose the mapping before the initial event
+        // has finished being dispatched.
+        static_cast<ui::TouchEvent*>(ui_event.get())
+            ->set_should_remove_native_touch_id_mapping(false);
+        break;
       default:
         break;
     }