Revert "Use accurate X11 event timestamp computation."

This reverts commit 3225e1d5.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 594844 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vMzIyNWUxZDVlYTQ2MjZhNzcwZDgzNWRkMDNhMjViNmMwOTViNThiNQw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests/72634

Sample Failed Step: events_unittests

Sample Flaky Test: X11EventTest.AutoRepeat

Original change's description:
> 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/1249383
> Reviewed-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}

Change-Id: I911f8bd268739b5e91c550e3f6a6186ae4dfbecb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 859155, 890121
Reviewed-on: https://chromium-review.googlesource.com/1250210
Cr-Commit-Position: refs/heads/master@{#594958}

ui/events/event_unittest.cc

@@ -25,7 +25,6 @@
 
 #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
@@ -431,15 +430,6 @@ 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;
 }
@@ -450,23 +440,7 @@ void AdvanceKeyEventTimestamp(XEvent* event) {
 
 }  // namespace
 
-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) {
+TEST(EventTest, AutoRepeat) {
   const uint16_t kNativeCodeA =
       ui::KeycodeConverter::DomCodeToNativeKeycode(DomCode::US_A);
   const uint16_t kNativeCodeB =
@@ -494,7 +468,6 @@ TEST_F(X11EventTest, 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);
@@ -565,14 +538,12 @@ TEST_F(X11EventTest, 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_TRUE(key_a1.is_repeat());
+    EXPECT_FALSE(key_a1.is_repeat());
 
     KeyEvent key_a1_with_same_event(native_event_a_pressed);
-    EXPECT_TRUE(key_a1_with_same_event.is_repeat());
+    EXPECT_FALSE(key_a1_with_same_event.is_repeat());
   }
 }
 #endif  // USE_X11

ui/events/platform/x11/x11_event_source.cc

@@ -100,7 +100,6 @@ X11EventSource::X11EventSource(X11EventSourceDelegate* delegate,
       distribution_(0, 999) {
   DCHECK(!instance_);
   instance_ = this;
-  SetTimestampServer(this);
 
   DCHECK(delegate_);
   DCHECK(display_);
@@ -111,7 +110,6 @@ 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,7 +14,6 @@
 #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;
@@ -47,7 +46,7 @@ class X11EventSourceDelegate {
 
 // Receives X11 events and sends them to X11EventSourceDelegate. Handles
 // receiving, pre-process and post-processing XEvents.
-class EVENTS_EXPORT X11EventSource : TimestampServer {
+class EVENTS_EXPORT X11EventSource {
  public:
   X11EventSource(X11EventSourceDelegate* delegate, XDisplay* display);
   ~X11EventSource();
@@ -83,7 +82,7 @@ class EVENTS_EXPORT X11EventSource : TimestampServer {
 
   // Explicitly asks the X11 server for the current timestamp, and updates
   // |last_seen_server_time_| with this value.
-  Time GetCurrentServerTime() override;
+  Time GetCurrentServerTime();
 
  protected:
   // Extracts cookie data from |xevent| if it's of GenericType, and dispatches

ui/events/x/events_x.cc

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

ui/events/x/events_x_unittest.cc

@@ -76,15 +76,6 @@ 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 {
@@ -93,15 +84,13 @@ class EventsXTest : public testing::Test {
   ~EventsXTest() override {}
 
   void SetUp() override {
-    SetTimestampServer(&server_);
     DeviceDataManagerX11::CreateInstance();
     ui::TouchFactory::GetInstance()->ResetForTest();
+    ResetTimestampRolloverCountersForTesting();
   }
-
-  void TearDown() override { SetTimestampServer(nullptr); }
+  void TearDown() override { ResetTimestampRolloverCountersForTesting(); }
 
  private:
-  MockTimestampServer server_;
   DISALLOW_COPY_AND_ASSIGN(EventsXTest);
 };
 
@@ -553,4 +542,52 @@ 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,24 +27,6 @@
 
 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;
 
@@ -326,29 +308,40 @@ 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) {
-  // 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;
+  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);
 }
 
 }  // namespace
@@ -838,12 +831,9 @@ bool IsAltPressed() {
   return XModifierStateWatcher::GetInstance()->state() & Mod1Mask;
 }
 
-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;
+void ResetTimestampRolloverCountersForTesting() {
+  g_last_seen_timestamp_ms = 0;
+  g_rollover_ms = 0;
 }
 
 }  // namespace ui

ui/events/x/events_x_utils.h

@@ -16,8 +16,6 @@
 #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.
@@ -93,15 +91,6 @@ 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_