Avoid gpu watchdog crash on timeout if X is un-responsive.

content/gpu/gpu_watchdog_thread.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
 
 #include "content/gpu/gpu_watchdog_thread.h"
 
@@ skipping 21 matching lines @@
     : base::Thread("Watchdog"),
       watched_message_loop_(base::MessageLoop::current()),
       timeout_(base::TimeDelta::FromMilliseconds(timeout)),
       armed_(false),
 #if defined(OS_WIN)
       watched_thread_handle_(0),
       arm_cpu_time_(),
 #endif
       task_observer_(this),
       suspended_(false),
+#if defined(USE_X11)
+      display_(NULL),
+      window_(0),
+      atom_(None),
+#endif
       weak_factory_(this) {
   DCHECK(timeout >= 0);
 
 #if defined(OS_WIN)
   // GetCurrentThread returns a pseudo-handle that cannot be used by one thread
   // to identify another. DuplicateHandle creates a "real" handle that can be
   // used for this purpose.
   BOOL result = DuplicateHandle(GetCurrentProcess(),
                                 GetCurrentThread(),
                                 GetCurrentProcess(),
                                 &watched_thread_handle_,
                                 THREAD_QUERY_INFORMATION,
                                 FALSE,
                                 0);
   DCHECK(result);
 #endif
 
 #if defined(OS_CHROMEOS)
   tty_file_ = base::OpenFile(base::FilePath(kTtyFilePath), "r");
 #endif
+#if defined(USE_X11)
+  SetupXServer();
+#endif
   watched_message_loop_->AddTaskObserver(&task_observer_);
 }
 
 void GpuWatchdogThread::PostAcknowledge() {
   // Called on the monitored thread. Responds with OnAcknowledge. Cannot use
   // the method factory. Rely on reference counting instead.
   message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&GpuWatchdogThread::OnAcknowledge, this));
 }
@@ skipping 44 matching lines @@
 
   base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
   if (power_monitor)
     power_monitor->RemoveObserver(this);
 
 #if defined(OS_CHROMEOS)
   if (tty_file_)
     fclose(tty_file_);
 #endif
 
+#if defined(USE_X11)
+  XDestroyWindow(display_, window_);
+  XCloseDisplay(display_);
+#endif
+
   watched_message_loop_->RemoveTaskObserver(&task_observer_);
 }
 
 void GpuWatchdogThread::OnAcknowledge() {
   CHECK(base::PlatformThread::CurrentId() == thread_id());
 
   // The check has already been acknowledged and another has already been
   // scheduled by a previous call to OnAcknowledge. It is normal for a
   // watched thread to see armed_ being true multiple times before
   // the OnAcknowledge task is run on the watchdog thread.
@@ skipping 45 matching lines @@
   // TaskObserver. Any other tasks that are pending on the watched thread will
   // also wake up the observer. This simply ensures there is at least one.
   watched_message_loop_->PostTask(
       FROM_HERE,
       base::Bind(&base::DoNothing));
 
   // Post a task to the watchdog thread to exit if the monitored thread does
   // not respond in time.
   message_loop()->PostDelayedTask(
       FROM_HERE,
-      base::Bind(
-          &GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang,
-          weak_factory_.GetWeakPtr()),
+      base::Bind(&GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang,
+                 weak_factory_.GetWeakPtr()),
       timeout);
 }
 
 // Use the --disable-gpu-watchdog command line switch to disable this.
 void GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang() {
   // Should not get here while the system is suspended.
   DCHECK(!suspended_);
 
 #if defined(OS_WIN)
   // Defer termination until a certain amount of CPU time has elapsed on the
@@ skipping 13 matching lines @@
   // If the watchdog woke up significantly behind schedule, disarm and reset
   // the watchdog check. This is to prevent the watchdog thread from terminating
   // when a machine wakes up from sleep or hibernation, which would otherwise
   // appear to be a hang.
   if (base::Time::Now() > suspension_timeout_) {
     armed_ = false;
     OnCheck(true);
     return;
   }
 
+#if defined(USE_X11)
+  XWindowAttributes attributes;
+  XGetWindowAttributes(display_, window_, &attributes);
+
+  XSelectInput(display_, window_, PropertyChangeMask);
+  SetupXChangeProp();
+
+  XFlush(display_);
+
+  XEvent event_return;
+  while (1) {
+    struct pollfd fds[1];
+    fds[0].fd = XConnectionNumber(display_);
+    fds[0].events = POLLIN;
+    int status = poll(fds, 1, timeout_.InMilliseconds());

[piman, 2015/02/06 22:49:53]

1- we need to handle error values. In particular EINTR would mean we need to
restart the poll, but other errors would mean we need to abort (e.g. X died).
2- we need to treat the timeout globally across the loop. E.g. if we received a
signal every 5 seconds but the timeout is 10 seconds we would never terminate.
Something along the lines of getting the time before the loop, computing a
deadline, and then comparing the current time to the deadline in the while loop,
akin to:
base::TimeTicks deadline = base::TimeTicks::Now() + timeout_;
while (true) {
  base::TimeDelta delta = deadline - base::TimeTicks::Now();
  if (delta < base::TimeDelta()) {
     // timeout
  } else {
    int status = poll(..., delta.InMilliseconds());
    if (status == -1) {
      if (errno == EINTR) {
        // restart
        continue;
      } else {
        // error, abort
      }
    } else if (status == 0) {
      // timeout
    } else {
      if (XCheckWindowEvent(...)) {
        // got property
      } else {
        // other event, restart.
        continue;
      }
    }
  }
}

3- I think we need to check for the event even before the first poll. It's
possible in theory.
4- We could get more than 1 event (the window manager may want to tag properties
on our window), so the XCheckWindowEvent may need to be in a loop (to make sure
we consume all events), and check for the exact event we expect (the right
Atom).

[sohanjg, 2015/02/13 05:18:00]

Done.

+    if (status == 0) {
+      break;
+    }
+    if (XCheckWindowEvent(display_, window_, PropertyChangeMask,
+                          &event_return)) {
+      return;

[piman, 2015/02/06 22:49:54]

So, if I follow the overall logic, you're testing if X is alive, if it is you
cancel the watchdog trigger, but if it is not you keep going and crash.
I think we want the opposite. If X is alive, but the watchdog triggers, it means
we're not blocked waiting on X, so we're blocked waiting on the GPU to execute
GL commands. This is the typical case where we want to abort, because that means
that chrome is not responsive, possibly because of webgl content or something
else. If X is not responsive, it could be due to several things, among others
the VT switch as suggested in the bug in which case we do not want to crash, but
either way chrome and the desktop will not be responsive, but crashing the GPU
process is unlikely to fix it. I think that's the case where we want to avoid
the trigger.

[sohanjg, 2015/02/13 05:18:00]

Acknowledged.

+    }
+  }
+#endif
+
   // For minimal developer annoyance, don't keep terminating. You need to skip
   // the call to base::Process::Terminate below in a debugger for this to be
   // useful.
   static bool terminated = false;
   if (terminated)
     return;
 
 #if defined(OS_WIN)
   if (IsDebuggerPresent())
     return;
@@ skipping 15 matching lines @@
 
   LOG(ERROR) << "The GPU process hung. Terminating after "
              << timeout_.InMilliseconds() << " ms.";
 
   // Deliberately crash the process to create a crash dump.
   *((volatile int*)0) = 0x1337;
 
   terminated = true;
 }
 
+#if defined(USE_X11)
+void GpuWatchdogThread::SetupXServer() {
+  display_ = XOpenDisplay(NULL);
+  window_ = XCreateWindow(display_, DefaultRootWindow(display_), 0, 0, 1, 1, 0,
+                          CopyFromParent, InputOutput, CopyFromParent, 0, NULL);
+}
+
+void GpuWatchdogThread::SetupXChangeProp() {
+  unsigned char text[20] = "check";

[piman, 2015/02/06 22:49:53]

const unsigned char text[] = "check";

[sohanjg, 2015/02/13 05:18:00]

Done.

+  atom_ = XInternAtom(display_, "CHK", False);

[piman, 2015/02/06 22:49:54]

We shouldn't need to do this every time. Do you want to do this in SetupXServer?

[sohanjg, 2015/02/13 05:18:00]

Done.

+  XChangeProperty(display_, window_, atom_, XA_STRING, 8, PropModeReplace, text,
+                  5);

[piman, 2015/02/06 22:49:53]

strlen(text) instead of 5?
Do we need to change the property every time, or will we get an XPropertyEvent
even if it doesn't change?

[sohanjg, 2015/02/13 05:18:00]

We are getting the event even if we dont change.

+}
+#endif
 void GpuWatchdogThread::AddPowerObserver() {
   message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&GpuWatchdogThread::OnAddPowerObserver, this));
 }
 
 void GpuWatchdogThread::OnAddPowerObserver() {
   base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
   DCHECK(power_monitor);
   power_monitor->AddObserver(this);
@@ skipping 42 matching lines @@
   // not increasing. The other is where either the kernel hangs and never
   // returns to user level or where user level code
   // calls into kernel level repeatedly, giving up its quanta before it is
   // tracked, for example a loop that repeatedly Sleeps.
   return base::TimeDelta::FromMilliseconds(static_cast<int64>(
       (user_time64.QuadPart + kernel_time64.QuadPart) / 10000));
 }
 #endif
 
 }  // namespace content