Use accurate X11 event timestamp computation.

X events have a timestamp which is only well defined relative to the X11 Server
time. The previous computation for timestamp for X11 events was making the
assumption that Server time and Chrome time were the same. This assumption is
not always true -- this is likely the root cause of "bad" timestamps observed in
https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1

This CL changes event timestamp computation to make a roundtrip to the X11
Server to get an accurate base::TimeTicks. This logic was lifted out of the
responsiveness calculator, which was already doing this computation. The latter
will subsequently be changed to use the computation in this CL.

Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82
Bug: 859155
Reviewed-on: https://chromium-review.googlesource.com/1249383Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594844}

ui/events/event_unittest.cc

@@ -25,6 +25,7 @@
 
 #if defined(USE_X11)
 #include "ui/events/test/events_test_utils_x11.h"
+#include "ui/events/x/events_x_utils.h"  // nogncheck
 #include "ui/gfx/x/x11.h"        // nogncheck
 #include "ui/gfx/x/x11_types.h"  // nogncheck
 #endif
@@ -430,6 +431,15 @@ TEST(EventTest, KeyEventCode) {
 #if defined(USE_X11)
 namespace {
 
+class MockTimestampServer : public ui::TimestampServer {
+ public:
+  Time GetCurrentServerTime() override { return base_time_; }
+  void SetBaseTime(Time time) { base_time_ = time; }
+
+ private:
+  Time base_time_ = 0;
+};
+
 void SetKeyEventTimestamp(XEvent* event, int64_t time) {
   event->xkey.time = time & UINT32_MAX;
 }
@@ -440,7 +450,23 @@ void AdvanceKeyEventTimestamp(XEvent* event) {
 
 }  // namespace
 
-TEST(EventTest, AutoRepeat) {
+class X11EventTest : public testing::Test {
+ public:
+  X11EventTest() {}
+  ~X11EventTest() override {}
+
+  void SetUp() override { SetTimestampServer(&server_); }
+
+  void TearDown() override { SetTimestampServer(nullptr); }
+
+ protected:
+  MockTimestampServer server_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(X11EventTest);
+};
+
+TEST_F(X11EventTest, AutoRepeat) {
   const uint16_t kNativeCodeA =
       ui::KeycodeConverter::DomCodeToNativeKeycode(DomCode::US_A);
   const uint16_t kNativeCodeB =
@@ -468,6 +494,7 @@ TEST(EventTest, AutoRepeat) {
 
   int64_t ticks_base =
       (base::TimeTicks::Now() - base::TimeTicks()).InMilliseconds() - 5000;
+  server_.SetBaseTime(static_cast<Time>(ticks_base));
   SetKeyEventTimestamp(native_event_a_pressed, ticks_base);
   SetKeyEventTimestamp(native_event_a_pressed_1500, ticks_base + 1500);
   SetKeyEventTimestamp(native_event_a_pressed_3000, ticks_base + 3000);
@@ -538,12 +565,14 @@ TEST(EventTest, AutoRepeat) {
     EXPECT_FALSE(key_a4_pressed_nonstandard_state.is_repeat());
   }
 
+  // The nonstandard event from above was ignored, so the last event pressed was
+  // |native_event_a_pressed|. These are both repeats.
   {
     KeyEvent key_a1(native_event_a_pressed);
-    EXPECT_FALSE(key_a1.is_repeat());
+    EXPECT_TRUE(key_a1.is_repeat());
 
     KeyEvent key_a1_with_same_event(native_event_a_pressed);
-    EXPECT_FALSE(key_a1_with_same_event.is_repeat());
+    EXPECT_TRUE(key_a1_with_same_event.is_repeat());
   }
 }
 #endif  // USE_X11

ui/events/platform/x11/x11_event_source.cc

@@ -100,6 +100,7 @@ X11EventSource::X11EventSource(X11EventSourceDelegate* delegate,
       distribution_(0, 999) {
   DCHECK(!instance_);
   instance_ = this;
+  SetTimestampServer(this);
 
   DCHECK(delegate_);
   DCHECK(display_);
@@ -110,6 +111,7 @@ X11EventSource::X11EventSource(X11EventSourceDelegate* delegate,
 X11EventSource::~X11EventSource() {
   DCHECK_EQ(this, instance_);
   instance_ = nullptr;
+  SetTimestampServer(nullptr);
   if (dummy_initialized_)
     XDestroyWindow(display_, dummy_window_);
 }

ui/events/platform/x11/x11_event_source.h

@@ -14,6 +14,7 @@
 #include "base/optional.h"
 #include "build/build_config.h"
 #include "ui/events/events_export.h"
+#include "ui/events/x/events_x_utils.h"
 #include "ui/gfx/x/x11_types.h"
 
 using Time = unsigned long;
@@ -46,7 +47,7 @@ class X11EventSourceDelegate {
 
 // Receives X11 events and sends them to X11EventSourceDelegate. Handles
 // receiving, pre-process and post-processing XEvents.
-class EVENTS_EXPORT X11EventSource {
+class EVENTS_EXPORT X11EventSource : TimestampServer {
  public:
   X11EventSource(X11EventSourceDelegate* delegate, XDisplay* display);
   ~X11EventSource();
@@ -82,7 +83,7 @@ class EVENTS_EXPORT X11EventSource {
 
   // Explicitly asks the X11 server for the current timestamp, and updates
   // |last_seen_server_time_| with this value.
-  Time GetCurrentServerTime();
+  Time GetCurrentServerTime() override;
 
  protected:
   // Extracts cookie data from |xevent| if it's of GenericType, and dispatches

ui/events/x/events_x.cc

@@ -81,9 +81,7 @@ int EventFlagsFromNative(const PlatformEvent& native_event) {
 }
 
 base::TimeTicks EventTimeFromNative(const PlatformEvent& native_event) {
-  base::TimeTicks timestamp = EventTimeFromXEvent(*native_event);
-  ValidateEventTimeClock(&timestamp);
-  return timestamp;
+  return EventTimeFromXEvent(*native_event);
 }
 
 gfx::PointF EventLocationFromNative(const PlatformEvent& native_event) {

ui/events/x/events_x_unittest.cc

@@ -76,6 +76,15 @@ float ComputeRotationAngle(float twist) {
   return rotation_angle;
 }
 
+class MockTimestampServer : public ui::TimestampServer {
+ public:
+  Time GetCurrentServerTime() override { return base_time_; }
+  void SetBaseTime(Time time) { base_time_ = time; }
+
+ private:
+  Time base_time_ = 0;
+};
+
 }  // namespace
 
 class EventsXTest : public testing::Test {
@@ -84,13 +93,15 @@ class EventsXTest : public testing::Test {
   ~EventsXTest() override {}
 
   void SetUp() override {
+    SetTimestampServer(&server_);
     DeviceDataManagerX11::CreateInstance();
     ui::TouchFactory::GetInstance()->ResetForTest();
-    ResetTimestampRolloverCountersForTesting();
   }
-  void TearDown() override { ResetTimestampRolloverCountersForTesting(); }
+
+  void TearDown() override { SetTimestampServer(nullptr); }
 
  private:
+  MockTimestampServer server_;
   DISALLOW_COPY_AND_ASSIGN(EventsXTest);
 };
 
@@ -542,52 +553,4 @@ TEST_F(EventsXTest, IgnoresMotionEventForMouseWheelScroll) {
   EXPECT_EQ(ui::ET_UNKNOWN, ui::EventTypeFromNative(xev));
 }
 
-namespace {
-
-// Returns a fake TimeTicks based on the given millisecond offset.
-base::TimeTicks TimeTicksFromMillis(int64_t millis) {
-  return base::TimeTicks() + base::TimeDelta::FromMilliseconds(millis);
-}
-
-}  // namespace
-
-TEST_F(EventsXTest, TimestampRolloverAndAdjustWhenDecreasing) {
-  XEvent event;
-  InitButtonEvent(&event, true, gfx::Point(5, 10), 1, 0);
-
-  test::ScopedEventTestTickClock clock;
-  clock.SetNowTicks(TimeTicksFromMillis(0x100000001));
-  ResetTimestampRolloverCountersForTesting();
-
-  event.xbutton.time = 0xFFFFFFFF;
-  EXPECT_EQ(TimeTicksFromMillis(0xFFFFFFFF), ui::EventTimeFromNative(&event));
-
-  clock.SetNowTicks(TimeTicksFromMillis(0x100000007));
-  ResetTimestampRolloverCountersForTesting();
-
-  event.xbutton.time = 3;
-  EXPECT_EQ(TimeTicksFromMillis(0x100000000 + 3),
-            ui::EventTimeFromNative(&event));
-}
-
-TEST_F(EventsXTest, NoTimestampRolloverWhenMonotonicIncreasing) {
-  XEvent event;
-  InitButtonEvent(&event, true, gfx::Point(5, 10), 1, 0);
-
-  test::ScopedEventTestTickClock clock;
-  clock.SetNowTicks(TimeTicksFromMillis(10));
-  ResetTimestampRolloverCountersForTesting();
-
-  event.xbutton.time = 6;
-  EXPECT_EQ(TimeTicksFromMillis(6), ui::EventTimeFromNative(&event));
-  event.xbutton.time = 7;
-  EXPECT_EQ(TimeTicksFromMillis(7), ui::EventTimeFromNative(&event));
-
-  clock.SetNowTicks(TimeTicksFromMillis(0x100000005));
-  ResetTimestampRolloverCountersForTesting();
-
-  event.xbutton.time = 0xFFFFFFFF;
-  EXPECT_EQ(TimeTicksFromMillis(0xFFFFFFFF), ui::EventTimeFromNative(&event));
-}
-
 }  // namespace ui

ui/events/x/events_x_utils.cc

@@ -27,6 +27,24 @@
 
 namespace {
 
+ui::TimestampServer* g_timestamp_server = nullptr;
+
+// Clamps a TimeDelta to be within [0 seconds, 30 seconds].
+base::TimeDelta ClampDeltaFromExternalSource(const base::TimeDelta& delta) {
+  // Ignore pathologically long deltas. External source is probably having
+  // issues.
+  constexpr base::TimeDelta pathologically_long_duration =
+      base::TimeDelta::FromSeconds(30);
+  if (delta > pathologically_long_duration)
+    return base::TimeDelta();
+
+  // Ignore negative deltas. External source is probably having issues.
+  if (delta < base::TimeDelta())
+    return base::TimeDelta();
+
+  return delta;
+}
+
 // Scroll amount for each wheelscroll event. 53 is also the value used for GTK+.
 const int kWheelScrollAmount = 53;
 
@@ -308,40 +326,29 @@ bool GetGestureTimes(const XEvent& xev, double* start_time, double* end_time) {
   return true;
 }
 
-int64_t g_last_seen_timestamp_ms = 0;
-int64_t g_rollover_ms = 0;
-
-// Takes Xlib Time and returns a time delta that is immune to timer rollover.
-// This function is not thread safe as we do not use a lock.
 base::TimeTicks TimeTicksFromXEventTime(Time timestamp) {
-  int64_t timestamp64 = timestamp;
-
-  if (!timestamp)
-    return ui::EventTimeForNow();
-
-  // If this is the first event that we get, assume the time stamp roll-over
-  // might have happened before the process was started.
-  // Register a rollover if the distance between last timestamp and current one
-  // is larger than half the width. This avoids false rollovers even in a case
-  // where X server delivers reasonably close events out-of-order.
-  bool had_recent_rollover =
-      !g_last_seen_timestamp_ms ||
-      g_last_seen_timestamp_ms - timestamp64 > (UINT32_MAX >> 1);
-
-  g_last_seen_timestamp_ms = timestamp64;
-  if (!had_recent_rollover)
-    return base::TimeTicks() +
-        base::TimeDelta::FromMilliseconds(g_rollover_ms + timestamp);
-
-  DCHECK(timestamp64 <= UINT32_MAX)
-      << "X11 Time does not roll over 32 bit, the below logic is likely wrong";
-
-  base::TimeTicks now_ticks = ui::EventTimeForNow();
-  int64_t now_ms = (now_ticks - base::TimeTicks()).InMilliseconds();
-
-  g_rollover_ms = now_ms & ~static_cast<int64_t>(UINT32_MAX);
-  uint32_t delta = static_cast<uint32_t>(now_ms - timestamp);
-  return base::TimeTicks() + base::TimeDelta::FromMilliseconds(now_ms - delta);
+  // There's no way to convert from an X time to a base::TimeTicks without
+  // knowing the current X server time.
+  if (!g_timestamp_server)
+    return base::TimeTicks();
+
+  // X11 uses a uint32_t on the wire protocol. Xlib casts this to an unsigned
+  // long by prepending with 0s. We cast back to a uint32_t so that subtraction
+  // works properly when the timestamp overflows back to 0.
+  uint32_t event_server_time_ms = static_cast<uint32_t>(timestamp);
+  uint32_t current_server_time_ms =
+      static_cast<uint32_t>(g_timestamp_server->GetCurrentServerTime());
+
+  // On X11, event times are in X11 Server time. To convert to base::TimeTicks,
+  // we perform a round-trip to the X11 Server, subtract the two times to get a
+  // TimeDelta, and then subtract that from base::TimeTicks::Now(). Since we're
+  // working with units of time from an external source, we clamp the TimeDelta
+  // to reasonable values.
+  uint32_t delta_ms = current_server_time_ms - event_server_time_ms;
+  base::TimeDelta delta = base::TimeDelta::FromMilliseconds(delta_ms);
+  base::TimeDelta sanitized = ClampDeltaFromExternalSource(delta);
+
+  return base::TimeTicks::Now() - sanitized;
 }
 
 }  // namespace
@@ -831,9 +838,12 @@ bool IsAltPressed() {
   return XModifierStateWatcher::GetInstance()->state() & Mod1Mask;
 }
 
-void ResetTimestampRolloverCountersForTesting() {
-  g_last_seen_timestamp_ms = 0;
-  g_rollover_ms = 0;
+void SetTimestampServer(TimestampServer* server) {
+  // This method must be setting or unsetting a timestamp server. It should
+  // never replace an existing timestamp server, nor change from
+  // nullptr->nullptr.
+  CHECK(!!g_timestamp_server ^ !!server);
+  g_timestamp_server = server;
 }
 
 }  // namespace ui

ui/events/x/events_x_utils.h

@@ -16,6 +16,8 @@
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/x/x11_types.h"
 
+using Time = unsigned long;
+
 namespace ui {
 
 // Gets the EventType from a XEvent.
@@ -91,6 +93,15 @@ EVENTS_X_EXPORT bool IsAltPressed();
 
 EVENTS_X_EXPORT void ResetTimestampRolloverCountersForTesting();
 
+// Conversion from X Time to base::TimeTicks requires checking the current X
+// Server Time. This functionality is provided by X11EventSource, but due to odd
+// layering that cannot be referenced directly.
+class TimestampServer {
+ public:
+  virtual Time GetCurrentServerTime() = 0;
+};
+EVENTS_X_EXPORT void SetTimestampServer(TimestampServer* server);
+
 }  // namespace ui
 
 #endif  // UI_EVENTS_X_EVENTS_X_UTILS_H_