xvfb: Wait for ReparentNotify before executing tests.

X11 tests can fail because start up of OpenBox is not
synchronous and it is unknown when exactly it starts up.

That results in a client that uses X11 missing events
and flakiness of tests.

Thus, to avoid that issue, use a small program that
creates a dummy X11 windows and waits for the
ReparentNotify event. That is, OpenBox decorates
all the windows once it is initialized that results
in that event. Once the event is received, we can
be sure that the WM is up and running, and tests
can be executed.

Bug: 1078771
Change-Id: Ic6fae30a706a6b9c32e4670da56457c833b4b7f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214526Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#775001}

PRESUBMIT.py

@@ -459,7 +459,8 @@ _BANNED_CPP_FUNCTIONS = (
         r"^ui[\\/]gl[\\/].*\.cc$",
         r"^media[\\/]gpu[\\/].*\.cc$",
         r"^gpu[\\/].*\.cc$",
-      ),
+        r"^ui[\\/]base[\\/]x[\\/]xwmstartupcheck[\\/]xwmstartupcheck\.cc$",
+        ),
     ),
     (
       r'/XInternAtom|xcb_intern_atom',
@@ -2505,7 +2506,9 @@ def _CheckSpamLogging(input_api, output_api):
                  r"^tools[\\/]",
                  r"^ui[\\/]base[\\/]resource[\\/]data_pack.cc$",
                  r"^ui[\\/]aura[\\/]bench[\\/]bench_main\.cc$",
-                 r"^ui[\\/]ozone[\\/]platform[\\/]cast[\\/]"))
+                 r"^ui[\\/]ozone[\\/]platform[\\/]cast[\\/]",
+                 r"^ui[\\/]base[\\/]x[\\/]xwmstartupcheck[\\/]"
+                     r"xwmstartupcheck\.cc$"))
   source_file_filter = lambda x: input_api.FilterSourceFile(
       x, white_list=file_inclusion_pattern, black_list=black_list)
 

testing/xvfb.py

@@ -142,10 +142,17 @@ def _run_with_xvfb(cmd, env, stdoutfile, use_openbox, use_xcompmgr):
   openbox_proc = None
   xcompmgr_proc = None
   xvfb_proc = None
+  xwmstartupcheck_proc = None
   xvfb_ready = MutableBoolean()
   def set_xvfb_ready(*_):
     xvfb_ready.setvalue(True)
 
+  # Allow to wait for openbox. See comment below near xwmstartupcheck_proc.
+  wait_for_openbox = False
+  if '--wait-for-openbox' in cmd:
+    wait_for_openbox = True
+    cmd.remove('--wait-for-openbox')
+
   try:
     signal.signal(signal.SIGTERM, raise_xvfb_error)
     signal.signal(signal.SIGINT, raise_xvfb_error)
@@ -181,9 +188,25 @@ def _run_with_xvfb(cmd, env, stdoutfile, use_openbox, use_xcompmgr):
     dbus_pid = launch_dbus(env)
 
     if use_openbox:
+      # Creates a dummy window that waits for a ReparentNotify event that is
+      # sent whenever Openbox WM starts. Must be started before the OpenBox WM
+      # so that it does not miss the event. This helper program is located in
+      # the current build directory. The program terminates automatically after
+      # 1 second of waiting for the event.
+      if wait_for_openbox:
+        xwmstartupcheck_proc = subprocess.Popen(
+            './xwmstartupcheck', stderr=subprocess.STDOUT, env=env)
+
       openbox_proc = subprocess.Popen(
           ['openbox', '--sm-disable'], stderr=subprocess.STDOUT, env=env)
 
+      # Wait until execution is done. Does not block if the process has already
+      # been terminated. In that case, it's safe to read the return value.
+      if wait_for_openbox:
+        xwmstartupcheck_proc.wait()
+        if xwmstartupcheck_proc.returncode is not 0:
+          raise _XvfbProcessError('Failed to get OpenBox up.')
+
     if use_xcompmgr:
       xcompmgr_proc = subprocess.Popen(
           'xcompmgr', stderr=subprocess.STDOUT, env=env)
@@ -357,7 +380,8 @@ def _set_xdg_runtime_dir(env):
 
 
 def main():
-  usage = 'Usage: xvfb.py [command [--no-xvfb or --use-weston] args...]'
+  usage = 'Usage: xvfb.py [command [--no-xvfb, or --use-weston, or ' \
+    '--wait-for-openbox] args...]'
   if len(sys.argv) < 2:
     print >> sys.stderr, usage
     return 2

testing/xvfb_unittest.py

@@ -24,6 +24,9 @@ XVFB_TEST_SCRIPT = TEST_FILE.replace('_unittest', '_test_script')
 
 def launch_process(args):
   """Launches a sub process to run through xvfb.py."""
+  # Disable openbox as long as xvfb requires xwmstartupcheck program to be
+  # compiled and run so that it can check when Openbox starts.
+  args.append("--no-openbox")
   return subprocess.Popen(
       [XVFB, XVFB_TEST_SCRIPT] + args, stdout=subprocess.PIPE,
       stderr=subprocess.STDOUT, env=os.environ.copy())

tools/mb/mb.py

@@ -1532,6 +1532,7 @@ class MetaBuildWrapper(object):
       ]
     elif use_xvfb and test_type == 'windowed_test_launcher':
       extra_files.append('../../testing/xvfb.py')
+      extra_files.append('xwmstartupcheck')
       cmdline += [
           '../../testing/xvfb.py',
           './' + str(executable) + executable_suffix,
@@ -1545,6 +1546,8 @@ class MetaBuildWrapper(object):
           '--msan=%d' % msan,
           '--tsan=%d' % tsan,
           '--cfi-diag=%d' % cfi_diag,
+          # Bringing up openbox is racy. See xvfb.py
+          '--wait-for-openbox',
       ]
     elif test_type in ('windowed_test_launcher', 'console_test_launcher'):
       cmdline += [

ui/base/x/BUILD.gn

@@ -83,6 +83,10 @@ jumbo_component("x") {
     "//ui/gfx/x",
     "//ui/platform_window/common",
   ]
+
+  # Needed for tests.
+  # TODO(dpranke): move that to appropriate place after test() template is reworked.
+  deps += [ "//ui/base/x/xwmstartupcheck" ]
 }
 
 source_set("gl") {

ui/base/x/xwmstartupcheck/BUILD.gn

+# Copyright 2020 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.
+
+executable("xwmstartupcheck") {
+  # TODO(dpranke): this should be testonly.
+  # testonly = true
+
+  sources = [ "xwmstartupcheck.cc" ]
+
+  configs += [ "//build/config/linux:x11" ]
+}

ui/base/x/xwmstartupcheck/xwmstartupcheck.cc

+// Copyright 2020 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.
+//
+// Checks for the reparent notify events that is a signal that a WM has been
+// started. Returns 0 on success and 1 on failure. This program must be started
+// BEFORE the Wm starts.
+//
+
+#include <cerrno>
+#include <cstdio>
+
+#include <time.h>
+
+#include <X11/Xlib.h>
+
+void CalculateTimeout(const timespec& now,
+                      const timespec& deadline,
+                      timeval* timeout) {
+  // 1s == 1e+6 us.
+  // 1nsec == 1e-3 us
+  timeout->tv_usec = (deadline.tv_sec - now.tv_sec) * 1000000 +
+                     (deadline.tv_nsec - now.tv_nsec) / 1000;
+  timeout->tv_sec = 0;
+}
+
+class XScopedDisplay {
+ public:
+  explicit XScopedDisplay(Display* display) : display_(display) {}
+  ~XScopedDisplay() {
+    if (display_)
+      XCloseDisplay(display_);
+  }
+
+  Display* display() const { return display_; }
+
+ private:
+  Display* const display_;
+};
+
+int main(int argc, char* argv[]) {
+  // Connects to a display specified in the current process' env value DISPLAY.
+  XScopedDisplay scoped_display(XOpenDisplay(nullptr));
+
+  // No display found - fail early.
+  if (!scoped_display.display()) {
+    fprintf(stderr, "Couldn't connect to a display.\n");
+    return 1;
+  }
+
+  auto* xdisplay = scoped_display.display();
+
+  auto root_window = DefaultRootWindow(xdisplay);
+  if (!root_window) {
+    fprintf(stderr, "Couldn't find root window.\n");
+    return 1;
+  }
+
+  auto dummmy_window = XCreateSimpleWindow(
+      xdisplay, root_window, 0 /*x*/, 0 /*y*/, 1 /*width*/, 1 /*height*/,
+      0 /*border width*/, 0 /*border*/, 0 /*background*/);
+  if (!dummmy_window) {
+    fprintf(stderr, "Couldn't create a dummy window.");
+    return 1;
+  }
+
+  XMapWindow(xdisplay, dummmy_window);
+  // We are only interested in the ReparentNotify events that are sent whenever
+  // our dummy window is reparented because of a wm start.
+  XSelectInput(xdisplay, dummmy_window, StructureNotifyMask);
+  XFlush(xdisplay);
+
+  int display_fd = ConnectionNumber(xdisplay);
+
+  // Set deadline as 30s.
+  struct timespec now, deadline;
+  clock_gettime(CLOCK_REALTIME, &now);
+  deadline = now;
+  deadline.tv_sec += 30;
+
+  // Calculate first timeout.
+  struct timeval tv;
+  CalculateTimeout(now, deadline, &tv);
+
+  XEvent ev;
+  do {
+    fd_set in_fds;
+    FD_ZERO(&in_fds);
+    FD_SET(display_fd, &in_fds);
+
+    int ret = select(display_fd + 1, &in_fds, nullptr, nullptr, &tv);
+    if (ret == -1) {
+      if (errno != EINTR) {
+        perror("Error occured while polling the display fd");
+        break;
+      }
+    } else if (ret > 0) {
+      while (XPending(xdisplay)) {
+        XNextEvent(xdisplay, &ev);
+        // If we got ReparentNotify, a wm has started up and we can stop
+        // execution.
+        if (ev.type == ReparentNotify) {
+          return 0;
+        }
+      }
+    }
+    // Calculate next timeout. If it's less or equal to 0, give up.
+    clock_gettime(CLOCK_REALTIME, &now);
+    CalculateTimeout(now, deadline, &tv);
+  } while (tv.tv_usec >= 0);
+
+  return 1;
+}
+
+#if defined(LEAK_SANITIZER)
+// XOpenDisplay leaks memory if it takes more than one try to connect. This
+// causes LSan bots to fail. We don't care about memory leaks in xdisplaycheck
+// anyway, so just disable LSan completely.
+// This function isn't referenced from the executable itself. Make sure it isn't
+// stripped by the linker.
+__attribute__((used)) __attribute__((visibility("default"))) extern "C" int
+__lsan_is_turned_off() {
+  return 1;
+}
+#endif