Try setting DBUS_SESSION_BUS_ADDRESS when running tests in Chromedriver #5626

jgraham · 2017-04-20T17:23:58Z

This change is

w3c-bots · 2017-04-20T17:46:39Z

View the complete job log.

Lint

Passed

w3c-bots · 2017-04-20T17:46:40Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 2d52816
Using browser at version BuildID 20170420100256; SourceStamp 27311156637f9b5d4504373967e01c4241902ae7
Starting 10 test iterations
No tests run.

w3c-bots · 2017-04-20T17:46:41Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 2d52816
Using browser at version 59.0.3071.15 dev
Starting 10 test iterations
No tests run.

codecov-io · 2017-04-20T17:46:43Z

Codecov Report

Merging #5626 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5626   +/-   ##
=======================================
  Coverage   87.22%   87.22%           
=======================================
  Files          24       24           
  Lines        2418     2418           
  Branches      406      406           
=======================================
  Hits         2109     2109           
  Misses        239      239           
  Partials       70       70

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a741b1e...f7f7921. Read the comment docs.

jugglinmike · 2017-04-20T18:07:54Z

(This workaround is currently being discussed in gh-5406.)

jugglinmike · 2017-04-20T20:55:41Z

After (finally) reproducing the issue locally and verifying the efficacy of the workaround, and after reading the sordid history of the problem from ChromeDriver to Chromium to glib, I'm starting to come around to this.

From a purely technical perspective: it seems a little imprecise to incorporate the workaround into the "check stability" utility directly. We now know that the problem is limited to non-interactive environments. Did you consider implementing this as part of the TravisCI test script?

jugglinmike

On IRC, we discussed implementing this workaround in wptrunner. I'm not sure if that's still on your radar, but even if this is just an "interim" fix, I'd like to see a little more formal structure. Nothing major, though!

jugglinmike · 2017-04-21T15:13:33Z

check_stability.py

@@ -172,6 +172,9 @@ def version(self):
    def wptrunner_args(self):
        return NotImplemented

+    def extra_setup(self):
+        pass


The term "setup" is a bit ambiguous in this context (i.e. more installation
steps?) Could we rename the extra_setup method to prepare_environment? And
defer its invocation to just prior to run_tests?

Also, could you add a doc string?

jugglinmike · 2017-04-21T15:13:49Z

check_stability.py

+    def extra_setup(self):
+        # https://bugs.chromium.org/p/chromium/issues/detail?id=713947
+        logger.debug("DBUS_SESSION_BUS_ADDRESS %s" % os.environ.get("DBUS_SESSION_BUS_ADDRESS"))
+        if "DBUS_SESSION_BUS_ADDRESS" not in os.environ:


This is only necessary in Linux; could we bail in other environments?

Is that necessary given that there are no other environements at the moment and everything will certainly break in !linux, given that we are downloading linux-specific binaries?

Hm, I guess I forgot that context. Adding the condition now would make future efforts towards portability easier, but that would be a big job anyway (and one I don't know if we're even considering). I'm fine with leaving it as-is

jugglinmike · 2017-04-21T15:14:06Z

check_stability.py

+                logger.debug(subprocess.check_output(["dbus-launch"]))
+            else:
+                logger.debug("Setting DBUS_SESSION_BUS_ADDRESS to a placeholder value")
+                os.environ["DBUS_SESSION_BUS_ADDRESS"] = "/dev/null"


I don't currently understand the implications of using this value. I am going to take some time to learn what this actually means.

Since we believe that this works and dbus-launch also seems to work in the travis environment can we land this whilst you do that research? As I say at some point I will move this into wptrunner and we can be more careful then.

My research is complete. The short version: let's fail the build instead of setting the variable to /dev/null.

The manual page for dbus-launch reads:

If DBUS_SESSION_BUS_ADDRESS is not set for a process that tries to use
D-Bus, by default the process will attempt to invoke dbus-launch with the
--autolaunch option to start up a new session bus or find the existing bus
address on the X display or in a file in ~/.dbus/session-bus/

Which means that setting the variable to any value will have the intended result: stopping glib from attempting to start a session. However, setting it to /dev/null will effect the way Chrome interacts with the system. This is unlikely to be a problem today, but I'm concerned about future scenarios where tests are being written for the Web Bluetooth API (for example). I'd hate to trip anyone up just because they assumed that this low-level integration was valid.

The threat may seem kind of protracted, but I think it's fair to be conservative (call it paranoid if you like) here. That's because in reality, we have no use-case for this branch. The latest job log demonstrates that TravisCI (i.e. the only environment that requires this workaround) offers the dbus-launch executable:

DEBUG:check_stability:DBUS_SESSION_BUS_ADDRESS None DEBUG:check_stability:Attempting to start dbus DEBUG:check_stability:DBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-R7UYTMZ60G,guid=5b04beaf64b61ac2fe15c33858fa3141 DBUS_SESSION_BUS_PID=5839 DBUS_SESSION_BUS_WINDOWID=2097153

So the branch in question effectively states: "Do something potentially invalid in a case that we don't currently need to support." We could just remove that branch, but I think it would be more responsible to report a meaningful error for this case, just in case this script winds up getting ported to some new environment (FreeBSD on Docker...?). Something like, "Due to a bug in glib, a valid DBUS session address must be defined before starting Chrome."

In effect, the current branch silently implements a workaround for that future case, but it's a workaround that might have deeper ramifications. Reporting an error would give the future caller the knowledge they need to implement a version of this workaround that is suited to their environment (even including DBUS_SESSION_BUS_ADDRESS=/dev/null).

jugglinmike

This looks good to me. I don't know how you want to merge this (or if you'd like me to do it), but note that the commit message should be updated as well ("set the relevant environment variable to a bogus value.")

Due to a bug in glibc Chrome may hang when dbus isn't started (c.f. https://crbug.com/713947). This is not usually the case on machines being used interactively but is the case on Travis and other CI environments. As a workaround try starting dbus, and error out if that fails.

wpt-pr-bot added infra test labels Apr 20, 2017

jgraham mentioned this pull request Apr 20, 2017

Google Chrome CI job occassionally does not run any tests #5406

Closed

jgraham force-pushed the chrome_dbus_environ branch from c86699c to 760c0b2 Compare April 20, 2017 20:51

jgraham force-pushed the chrome_dbus_environ branch from 760c0b2 to c2ba531 Compare April 21, 2017 08:18

jgraham requested a review from jugglinmike April 21, 2017 14:18

jugglinmike requested changes Apr 21, 2017

View reviewed changes

jgraham force-pushed the chrome_dbus_environ branch 2 times, most recently from 2e8864c to fe6eb9a Compare April 21, 2017 16:53

jugglinmike approved these changes Apr 21, 2017

View reviewed changes

jgraham added 2 commits April 21, 2017 18:35

Ensure we set a return code if an exception is thrown

f7f7921

jgraham force-pushed the chrome_dbus_environ branch from fe6eb9a to f7f7921 Compare April 21, 2017 17:36

jgraham merged commit cd80437 into master Apr 21, 2017

plehegar deleted the chrome_dbus_environ branch September 25, 2017 20:42

jugglinmike mentioned this pull request Apr 12, 2018

WPT CLI Feature proposal: --experimental #10452

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try setting DBUS_SESSION_BUS_ADDRESS when running tests in Chromedriver #5626

Try setting DBUS_SESSION_BUS_ADDRESS when running tests in Chromedriver #5626

jgraham commented Apr 20, 2017 •

edited by wpt-reviewable-bot

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

codecov-io commented Apr 20, 2017 •

edited

Loading

jugglinmike commented Apr 20, 2017

jugglinmike commented Apr 20, 2017

jugglinmike left a comment

jugglinmike Apr 21, 2017

jugglinmike Apr 21, 2017

jgraham Apr 21, 2017

jugglinmike Apr 21, 2017

jugglinmike Apr 21, 2017

jgraham Apr 21, 2017

jugglinmike Apr 21, 2017

jugglinmike left a comment

Try setting DBUS_SESSION_BUS_ADDRESS when running tests in Chromedriver #5626

Try setting DBUS_SESSION_BUS_ADDRESS when running tests in Chromedriver #5626

Conversation

jgraham commented Apr 20, 2017 • edited by wpt-reviewable-bot Loading

w3c-bots commented Apr 20, 2017 • edited Loading

Lint

w3c-bots commented Apr 20, 2017 • edited Loading

Firefox (nightly channel)

w3c-bots commented Apr 20, 2017 • edited Loading

Chrome (unstable channel)

codecov-io commented Apr 20, 2017 • edited Loading

Codecov Report

jugglinmike commented Apr 20, 2017

jugglinmike commented Apr 20, 2017

jugglinmike left a comment

Choose a reason for hiding this comment

jugglinmike Apr 21, 2017

Choose a reason for hiding this comment

jugglinmike Apr 21, 2017

Choose a reason for hiding this comment

jgraham Apr 21, 2017

Choose a reason for hiding this comment

jugglinmike Apr 21, 2017

Choose a reason for hiding this comment

jugglinmike Apr 21, 2017

Choose a reason for hiding this comment

jgraham Apr 21, 2017

Choose a reason for hiding this comment

jugglinmike Apr 21, 2017

Choose a reason for hiding this comment

jugglinmike left a comment

Choose a reason for hiding this comment

jgraham commented Apr 20, 2017 •

edited by wpt-reviewable-bot

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

w3c-bots commented Apr 20, 2017 •

edited

Loading

codecov-io commented Apr 20, 2017 •

edited

Loading