New OS X try failures with level 3 on task cluster tests

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: haik, Assigned: Alex_Gaynor)

Tracking

55 Branch
mozilla56
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: sbmc2)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1332190
See Also: → bug 1372229
Whiteboard: sbmc2
(Assignee)

Comment 1

a year ago
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).
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → agaynor
(Assignee)

Comment 3

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

a year ago
mozreview-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/#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.
Attachment #8879979 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

a year ago
mozreview-review
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.
Attachment #8879978 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8879980 - Attachment is obsolete: true

Comment 27

a year ago
mozreview-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/#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.
Attachment #8879979 - Flags: review?(gpascutto) → review+
(Assignee)

Comment 28

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Attachment #8879979 - Flags: review?(haftandilian) → review?(jmaher)
(Reporter)

Comment 29

a year ago
mozreview-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/#review158906

r+ from me as long as we get build peer review.
Attachment #8879979 - Flags: review+

Comment 30

a year ago
mozreview-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?
Attachment #8879979 - Flags: review?(jmaher) → review+
(Assignee)

Comment 31

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 32

a year ago
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
Keywords: checkin-needed

Comment 33

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74c02bb049fc
https://hg.mozilla.org/mozilla-central/rev/b57aac875b65
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → bug 1403325
You need to log in before you can comment on or make changes to this bug.