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..da3cd1b93a647982de13750cea897c040c7514ce 100644
--- a/content/gpu/gpu_watchdog_thread.cc
+++ b/content/gpu/gpu_watchdog_thread.cc
@@ -28,6 +28,10 @@ const base::FilePath::CharType
 #endif
 }  // namespace
 
+#if defined(USE_X11)
+const unsigned char text[20] = "check";
+#endif

[piman, 2015/02/13 22:28:38]

nit: move into anonymous namespace above.

+
 GpuWatchdogThread::GpuWatchdogThread(int timeout)
     : base::Thread("Watchdog"),
       watched_message_loop_(base::MessageLoop::current()),
@@ -39,6 +43,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 +68,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 +135,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 +205,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 +240,61 @@ void GpuWatchdogThread::DeliberatelyTerminateToRecoverFromHang() {
     return;
   }
 
+#if defined(USE_X11)
+  XWindowAttributes attributes;
+  XGetWindowAttributes(display_, window_, &attributes);
+
+  XSelectInput(display_, window_, PropertyChangeMask);
+  SetupXChangeProp();
+
+  XFlush(display_);
+
+  XEvent event_return;
+  base::TimeTicks deadline = base::TimeTicks::Now() + timeout_;
+  while (true) {
+    base::TimeDelta delta = deadline - base::TimeTicks::Now();
+    if (delta < base::TimeDelta()) {
+      // timeout

[piman, 2015/02/13 22:28:38]

It would be more useful to have a comment describing the loop and the overall
logic, something along the lines of:
"We wait for the property change event with a timeout. If it arrives we know
that X is responsive and is not the cause of the watchdog trigger, so we should
terminate. If it times out, it may be due to X taking a long time, but
terminating won't help, so ignore the watchdog trigger."

[sohanjg, 2015/02/16 09:44:49]

Done.

+      return;
+    } else {
+      if (XCheckWindowEvent(display_, window_, PropertyChangeMask,
+                            &event_return)) {
+        // got property

[piman, 2015/02/13 22:28:38]

nit: remove comment

[sohanjg, 2015/02/16 09:44:49]

Done.

+        if (MatchXWindowProp())
+          break;
+      }
+      struct pollfd fds[1];
+      fds[0].fd = XConnectionNumber(display_);
+      fds[0].events = POLLIN;
+      int status = poll(fds, 1, delta.InMilliseconds());
+      if (status == -1) {
+        if (errno == EINTR)
+          // restart

[piman, 2015/02/13 22:28:38]

nit: remove comment

[sohanjg, 2015/02/16 09:44:49]

Done.

+          continue;
+        else
+          // error, abort

[piman, 2015/02/13 22:28:38]

nit: remove comment

[sohanjg, 2015/02/16 09:44:49]

Done.

+          break;

[piman, 2015/02/13 22:28:38]

In this case, we should LOG(FATAL) << "Lost X connection, aborting." to separate
the case from the watchdog trigger.

[sohanjg, 2015/02/16 09:44:49]

Done.

+      } else if (status == 0) {
+        // timeout

[piman, 2015/02/13 22:28:38]

nit: remove comment

+        return;
+      } else {
+        while (true) {

[piman, 2015/02/13 22:28:38]

I don't think this second block makes sense. Can we just continue, but maybe
replace the if on line 260 by a while?

[sohanjg, 2015/02/16 09:44:49]

Done.

+          base::TimeDelta delta2 = deadline - base::TimeTicks::Now();
+          if (delta2 < base::TimeDelta())
+            // timeout
+            return;
+          if (XCheckWindowEvent(display_, window_, PropertyChangeMask,
+                                &event_return)) {
+            if (MatchXWindowProp())
+              break;
+          } else
+            continue;
+        }
+      }
+    }
+  }
+#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 +330,36 @@ 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);
+  atom_ = XInternAtom(display_, "CHECK", False);
+}
+
+void GpuWatchdogThread::SetupXChangeProp() {
+  XChangeProperty(display_, window_, atom_, XA_STRING, 8, PropModeReplace, text,
+                  strlen((char*)text));

[piman, 2015/02/13 22:28:38]

nit: no c-style cast.
You should be able to use arraysize(text)-1.

[sohanjg, 2015/02/16 09:44:49]

Done.

+}
+
+bool GpuWatchdogThread::MatchXWindowProp() {
+  unsigned long nitems = 0;
+  unsigned long nbytes = 0;
+  XAtom prop_type = None;
+  int prop_format = 0;
+  unsigned char* property_data = NULL;
+  if (XGetWindowProperty(display_, window_, atom_, 0, 65535, False,

[piman, 2015/02/13 22:28:38]

I was more imagining that we'd check the X event, verifying that the atom is the
same as the one we set.
As is, this would check if the property is there at all, which would be if we
went through the loop once and then timed out.

[sohanjg, 2015/02/16 09:44:49]

Done.

+                         AnyPropertyType, &prop_type, &prop_format, &nitems,
+                         &nbytes, &property_data) == Success) {
+    if (prop_type == XA_STRING && prop_format == 8 &&
+        nitems == strlen((char*)text))
+      return true;
+  }
+  return false;
+}
+
+#endif
 void GpuWatchdogThread::AddPowerObserver() {
   message_loop()->PostTask(
       FROM_HERE,