Reland "xvfb: Wait for ReparentNotify before executing tests."

This is a reland of 2fb9ab54:

FIX: change test type from windowed to console for
midi_unittests

Original change's description:
> 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/+/2214526
> Reviewed-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}

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

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',
@@ -2508,7 +2509,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/buildbot/gn_isolate_map.pyl

@@ -1187,7 +1187,7 @@
   },
   "midi_unittests": {
     "label": "//media/midi:midi_unittests",
-    "type": "windowed_test_launcher",
+    "type": "console_test_launcher",
   },
   "mini_installer": {
     "label": "//chrome/installer/mini_installer:mini_installer",

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

@@ -1445,6 +1445,7 @@ class MetaBuildWrapper(object):
                       or vals.get('cros_passthrough', False))
     is_mac = self.platform == 'darwin'
     is_win = self.platform == 'win32' or 'target_os="win"' in vals['gn_args']
+    is_cast_audio_only = 'is_cast_audio_only=true' in vals['gn_args']
 
     # This should be true if tests with type='windowed_test_launcher' are
     # expected to run using xvfb. For example, Linux Desktop, X11 CrOS and
@@ -1455,7 +1456,7 @@ class MetaBuildWrapper(object):
     # TODO(tonikitoo,msisov,fwang): Find a way to run tests for the Wayland
     # backend.
     use_xvfb = (self.platform == 'linux2' and not is_android and not is_fuchsia
-                and not is_cros_device)
+                and not is_cros_device and not is_cast_audio_only)
 
     asan = 'is_asan=true' in vals['gn_args']
     msan = 'is_msan=true' in vals['gn_args']
@@ -1532,6 +1533,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 +1547,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