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

content/gpu/gpu_watchdog_thread.cc

Index: content/gpu/gpu_watchdog_thread.cc
diff --git a/content/gpu/gpu_watchdog_thread.cc b/content/gpu/gpu_watchdog_thread.cc
index 02b6bff8040e8f6c7c464f309c31761da0f508f9..1caacb76b739780cdda919bb366e70792c6b6ef1 100644
--- a/content/gpu/gpu_watchdog_thread.cc
+++ b/content/gpu/gpu_watchdog_thread.cc
@@ -39,6 +39,11 @@ GpuWatchdogThread::GpuWatchdogThread(int timeout)
 #endif
       task_observer_(this),
       suspended_(false),
+#if defined(USE_X11)
+      display_(NULL),
+      window_(0),
+      atom_(None),
+#endif
       weak_factory_(this) {
   DCHECK(timeout >= 0);
 
@@ -59,6 +64,9 @@ GpuWatchdogThread::GpuWatchdogThread(int timeout)
 #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_);
 }
 
@@ -123,6 +131,11 @@ GpuWatchdogThread::~GpuWatchdogThread() {
     fclose(tty_file_);
 #endif
 
+#if defined(USE_X11)
+  XDestroyWindow(display_, window_);
+  XCloseDisplay(display_);
+#endif
+
   watched_message_loop_->RemoveTaskObserver(&task_observer_);
 }
 
@@ -188,9 +201,8 @@ void GpuWatchdogThread::OnCheck(bool after_suspend) {
   // not respond in time.
   message_loop()->PostDelayedTask(
       FROM_HERE,
-      base::Bind(
-          &GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang,
-          weak_factory_.GetWeakPtr()),
+      base::Bind(&GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang,
+                 weak_factory_.GetWeakPtr()),
       timeout);
 }
 
@@ -224,6 +236,31 @@ void GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang() {
     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.
@@ -259,6 +296,20 @@ void GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang() {
   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,