With the changes to OS X automated tests to use task cluster, several tests are failing. Here's an example run enabling level3 including the fix for Bug 1334550. https://treeherder.mozilla.org/#/jobs?repo=try&revision=779a73db4cc282b66c829c330a07c892e539a0a4 The failures appear to be due to the tests being run out of the home directory. Before they were run out of /builds.

Spent a bit of time looking at it, moving the build to |/builds| leads to effectively the same problems as we see in bug 1357758. To solve this, we'll need to do essentially the same things as bug 1308400. Specifically the pieces we'll need are: - |read_path_whitelist| from https://reviewboard.mozilla.org/r/133516/diff/2#index_header - Setting |read_path_whitelist| with the test paths from https://reviewboard.mozilla.org/r/147690/diff/1#index_header One thing that's going to be a little bit frustrating is that the macOS sandbox policy language does not have a way of passing arrays as a parameters or splitting a string (as far as google can tell at least, it's not like any of this is documented :-)). This means we'll probably have to resort to concatenating strings for each path we want to whitelist. Not the end of the world, just ugly. Long term, I think we can remove all this manual whitelisting with bug 1136836 (as I mentioned on IRC a few weeks ago, I'm starting to wonder if we'd be better off remoting all of necko).

Hah, actually I lied, there is way to split a string |(split-string var ",")| so that reduces the ugliness of this a little bit. You end up with: (allow file-read* (apply subpath (split-string read-whitelist-paths ","))) Not too horrible.

Will hopefully have a patch up for this tomorrow, will be a few grafts from :gcp's patch, so we'll have to figure out how to deconflict those.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review157880 This looks good, but if Mac will only require at most two directories to whitelist, I'd prefer to add two prefs specifically for Mac. That way we can avoid using a comma delimeter which will break for directories that include commas and we'll want to fix it eventually. Another way of encoding the pref that didn't depend on the comma-delimetering would also work. ::: testing/mozbase/mozprofile/mozprofile/profile.py:56 (Diff revision 3) > :param addons: String of one or list of addons to install > :param addon_manifests: Manifest for addons (see http://bit.ly/17jQ7i6) > :param preferences: Dictionary or class of preferences > :param locations: ServerLocations object > :param proxy: Setup a proxy > :param restore: Flag for removing all custom settings during cleanup Document the whitelistpaths param. ::: testing/mozharness/configs/unittests/linux_unittest.py:120 (Diff revision 3) > "--log-errorsummary=%(error_summary_file)s", > "--use-test-media-devices", > "--screenshot-on-fail", > "--cleanup-crashes", > "--marionette-startup-timeout=180", > + "--work-path=" + ABS_WORK_DIR, We shouldn't include the Linux changes with this fix since we won't be testing it yet.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review157880 Sounds good -- I'll do this once I've confirmed that tests are all green!

Comment on attachment 8879978 [details] Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox; https://reviewboard.mozilla.org/r/151328/#review158506 ::: dom/ipc/ContentChild.cpp:1446 (Diff revision 5) > - } else { > - info.appDir.assign(appDir.get()); > + info.appDir.assign(appDir.get()); > - } > info.appTempDir.assign(tempDirPath.get()); > > + nsAdoptingCString testingReadPath1 = Please add a comment explaining what these are for. How we only expect those prefs to appear in profiles used for automated tests.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review158890 ::: testing/mozbase/mozprofile/mozprofile/profile.py:118 (Diff revision 7) > prefs_js, user_js = self.permissions.network_prefs(self._proxy) > + > + if self._whitelistpaths: > + # On macOS we don't want to support a generalized read whitelist, > + # and the macOS sandbox policy language doesn't have support for > + # lists, so we handle these specially. I'm not sure that warrants a separate pref and/or the (fairly ugly) handling that follows here. You can split them up in the thing that reads this pref, right? Also you need a proper peer to review changes to test scripts.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review158890 > I'm not sure that warrants a separate pref and/or the (fairly ugly) handling that follows here. You can split them up in the thing that reads this pref, right? > > Also you need a proper peer to review changes to test scripts. Splitting on `,` is technically ambigious (if you have a `,` in the path), which :haik wanted to avoid.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review158906 r+ from me as long as we get build peer review.

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review158940 ::: layout/tools/reftest/runreftest.py:306 (Diff revision 7) > '5.1' in platform.version() and options.e10s: > prefs['layers.acceleration.disabled'] = True > > + sandbox_whitelist_paths = [SCRIPT_DIRECTORY] > + try: > + if options.workPath: do we need topsrcdir like we have for mochitest? ::: testing/mozbase/mozprofile/mozprofile/profile.py:131 (Diff revision 7) > + prefs_js.append(( > + "security.sandbox.content.mac.testing_read_path1", > + self._whitelistpaths[0] > + )) > + else: > + prefs_js.append(("security.sandbox.content.read_path_whitelist", do we need to do this for talos as well?

Comment on attachment 8879979 [details] Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; https://reviewboard.mozilla.org/r/151330/#review158940 > do we need topsrcdir like we have for mochitest? Nope! (Tests would have failed horribly if it was :-)) > do we need to do this for talos as well? Great question. I don't _think_ so, unless talos is relying on being able to load `chrome://` URIs from various places on disk, it shouldn't be necessary.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/74c02bb049fc Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox; r=haik https://hg.mozilla.org/integration/autoland/rev/b57aac875b65 Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; r=gcp,haik,jmaher

https://hg.mozilla.org/mozilla-central/rev/74c02bb049fc https://hg.mozilla.org/mozilla-central/rev/b57aac875b65