1380141 - Running Talos locally on OS X results in crashed tabs

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

I hit this today and burned some time debugging it. Apparently, we're runtime-aborting here:

http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/style/nsLayoutStylesheetCache.cpp#776

because resource://gre-resources/counterstyles.css isn't loading properly:

http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/style/nsLayoutStylesheetCache.cpp#345-346

I'm getting a NS_ERROR_FILE_ACCESS_DENIED there, which makes me think this is similar to bug 1379784, and is fallout from the sandboxing stuff that recently got turned on in bug 1332190.

My current workaround is to disable the content sandbox when doing Talos runs locally via MOZ_DISABLE_CONTENT_SANDBOX.

Comment 1

[Haik Aftandilian [:haik]]

It looks like we're not setting MOZ_DEVELOPER_REPO_DIR for talos tests.

This is reproducible for me with

  $ ./mach talos-test -a ts_paint

And setting MOZ_DEVELOPER_REPO_DIR to the path of my repo allows the test to run to completion without crashing:

  $ MOZ_DEVELOPER_REPO_DIR=/path/to/my/mozilla-central/ ./mach talos-test -a ts_paint

As a workaround, set the MOZ_DEVELOPER_REPO_DIR to the path of your repo.

Comment 2

[Haik Aftandilian [:haik]]

Attachment: Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Like we do for mochitests, set the MOZ_DEVELOPER_REPO_DIR environment
variable when running talos tests so that the content sandbox permits
access to test files that are symlinks from the object dir to the repo.

Review commit: https://reviewboard.mozilla.org/r/156320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156320/

Comment 3

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/1-2/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review161606

::: testing/talos/mach_commands.py:94
(Diff revision 2)
>              err_str = "Error writing to Talos Mozharness config file {0}:{1}"
>              print(err_str.format(self.config_file_path, str(e)))
>              raise e
>  
>      def run_mozharness(self):
> +        os.environ['MOZ_DEVELOPER_REPO_DIR'] = self.topsrcdir

I think it'd be better to pass the directory to `Talos`, and then use it in `Talos.run_tests` - `Talos.run_tests` already builds its own `env` `dict`, and if we do that we can avoid relying on `os.environ` being inherited by the child process.

Comment 5

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/2-3/

Comment 6

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review161606

> I think it'd be better to pass the directory to `Talos`, and then use it in `Talos.run_tests` - `Talos.run_tests` already builds its own `env` `dict`, and if we do that we can avoid relying on `os.environ` being inherited by the child process.

That's better. Thanks.

Comment 7

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/3-4/

Comment 8

[Haik Aftandilian [:haik]]

To add some context here, on Mac and Linux local (unpackaged) builds, the object directory contains symlinks to files in the repo directory so that they don't have to be copied to the object dir on every build. For Mac, the generated Nightly.app/ contains these symlinks. For content sandboxing, the the content process is allowed to access files within Nightly.app/, but to allow content to read files symlinked from Nightly.app/ to the source dir, we have to allow read access to the source dir. To do that, we have mach set MOZ_DEVELOPER_REPO_DIR. This is done for "./mach run" and "./mach mochitest". We need to also do that for talos tests.

And we're looking into ways to have this automatic so that MOZ_DEVELOPER_REPO_DIR doesn't have to be set by mach or manually when running outside of mach. This all only applies to unpackaged builds.

Comment 9

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/4-5/

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/5-6/

Comment 11

[Haik Aftandilian [:haik]]

The latest patch includes setting MOZ_DEVELOPER_OBJ_DIR which I plan to introduce in Bug 1380132.

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review162042

The commit message is a bit wonky, since we don't actually have `MOZ_DEVELOPER_OBJ_DIR` yet, but the change itself LGTM.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review162054

overall this is close, I just want to make sure we are not changing the environment and having unintended consequences for automation.  In general I like to keep automation and locally as close as possible.

::: testing/mozharness/mozharness/mozilla/testing/talos.py:178
(Diff revision 6)
>          self.installer_url = self.config.get("installer_url")
>          self.talos_json_url = self.config.get("talos_json_url")
>          self.talos_json = self.config.get("talos_json")
>          self.talos_json_config = self.config.get("talos_json_config")
> +        self.repo_path = self.config.get("repo_path")
> +        self.obj_path = self.config.get("obj_path")

these are set when running via mach, but how about in automation?  Is this something that has a default value and we know what it is?

::: testing/mozharness/mozharness/mozilla/testing/talos.py:555
(Diff revision 6)
>              env['PYTHONPATH'] = self.talos_path
>  
>          # mitmproxy needs path to mozharness when installing the cert
>          env['SCRIPTSPATH'] = scripts_path
>  
> +        env['MOZ_DEVELOPER_REPO_DIR'] = self.repo_path

what is the side effect of doing this in automation?  should this be inside an if statement:
if self.repo_path != None:
   env[...

Comment 14

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review162160

::: testing/mozharness/mozharness/mozilla/testing/talos.py:178
(Diff revision 6)
>          self.installer_url = self.config.get("installer_url")
>          self.talos_json_url = self.config.get("talos_json_url")
>          self.talos_json = self.config.get("talos_json")
>          self.talos_json_config = self.config.get("talos_json_config")
> +        self.repo_path = self.config.get("repo_path")
> +        self.obj_path = self.config.get("obj_path")

Discussed this on IRC with jmaher.

In short, mach talos tests are always run on developer builds where the presence of an object dir and repo dir are required for the browser to function. And code in testing/talos/mach_commands.py TalosRunner:init_variables() already depends on topsrcdir and topobjdir being set.

On automation, the talos tests use a packaged build and don't execute through mach. That's OK because we don't need these environment variables when running packaged builds. (We haven't see any issues with talos on automation caused by a missing REPO environment variable which supports this.)

::: testing/mozharness/mozharness/mozilla/testing/talos.py:555
(Diff revision 6)
>              env['PYTHONPATH'] = self.talos_path
>  
>          # mitmproxy needs path to mozharness when installing the cert
>          env['SCRIPTSPATH'] = scripts_path
>  
> +        env['MOZ_DEVELOPER_REPO_DIR'] = self.repo_path

Automation talos tests aren't executed through mach so these changes aren't relevant there.

I think it makes sense to only set the environment variables if the python strings are not "None" and I'll make that change. We have bug 1380560 filed to get the browser to warn if the environment variables are missing on a dev build.

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review162042

Thanks. I'll land this after MOZ_DEVELOPER_OBJ_DIR goes in.

Comment 16

[Haik Aftandilian [:haik]]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156320/diff/6-7/

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

https://reviewboard.mozilla.org/r/156320/#review162242

thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ffd794d30d1
Running Talos locally on OS X results in crashed tabs. r=Alex_Gaynor,jmaher

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6ffd794d30d1