Closed Bug 1380141 Opened 7 years ago Closed 7 years ago

Running Talos locally on OS X results in crashed tabs

Categories

(Core :: Security: Process Sandboxing, enhancement)

Product:
Core
Component:
Security: Process Sandboxing
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: haik)

References

Details

(Whiteboard: sbmc2)

Attachments

(1 file)

Bug 1380141 - Running Talos locally on OS X results in crashed tabs.
59 bytes, text/x-review-board-request
jmaher
: review+
Alex_Gaynor
: review+
Details 
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.
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.
Assignee: nobody → haftandilian
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 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.
Attachment #8885467 - Flags: review?(agaynor) → review?(jmaher)
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.
See Also: → 1380416
The latest patch includes setting MOZ_DEVELOPER_OBJ_DIR which I plan to introduce in Bug 1380132.
See Also: → 1380560
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.
Attachment #8885467 - Flags: review?(agaynor) → review+
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[...
Attachment #8885467 - Flags: review?(jmaher) → review-
Whiteboard: sbmc2
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 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 on attachment 8885467 [details]
Bug 1380141 - Running Talos locally on OS X results in crashed tabs.

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

thanks!
Attachment #8885467 - Flags: review?(jmaher) → review+
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ffd794d30d1
Running Talos locally on OS X results in crashed tabs. r=Alex_Gaynor,jmaher
https://hg.mozilla.org/mozilla-central/rev/6ffd794d30d1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: