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 10 matching lines @@
 
 namespace content {
 namespace {
 const int64 kCheckPeriodMs = 2000;
 #if defined(OS_CHROMEOS)
 const base::FilePath::CharType
     kTtyFilePath[] = FILE_PATH_LITERAL("/sys/class/tty/tty0/active");
 #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()),
       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;
+  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.
   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);
+  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,
       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