Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

As it looks right now in case of content crashes the browser just hangs around and tests won't be able to continue in the same tab due to the disconnected message channel. 

In Marionette we make use of the MOZ_CRASHREPORTER_SHUTDOWN=1 environment variable (bug 1299216) similar to other test harnesses, which forces Firefox to quit when a content crash happened. With this behavior we would at least be able to indicate that something went wrong.

Not sure yet if the following env variables would also be useful given that I cannot find those yet in the code:

MOZ_CRASHREPORTER=1,
MOZ_CRASHREPORTER_NO_REPORT=1,

I'm personally in favor of a more robust crash handling via geckodriver! And this would be a good start.

Andreas and James what do you both thin of that?

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 7 years ago

Your reasoning here seems sound, however, what do these two
variables do?

(In reply to Henrik Skupin (:whimboo) from comment #0)

> MOZ_CRASHREPORTER=1,
> MOZ_CRASHREPORTER_NO_REPORT=1,

And with regards to:

> I'm personally in favor of a more robust crash handling via
> geckodriver! And this would be a good start.

Is this something you have to set before Firefox starts, e.g. in
geckodriver, or something we can (also) enable at runtime?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> Your reasoning here seems sound, however, what do these two
variables do?

You can find details here: https://developer.mozilla.org/docs/Environment_variables_affecting_crash_reporting

> > MOZ_CRASHREPORTER=1,

> The opposite of MOZ_CRASHREPORTER_DISABLE, force crash reporting on even if disabled in application.ini. You must use this to enable crash reporting on Linux debug builds.

> > MOZ_CRASHREPORTER_NO_REPORT=1,

> Save the minidump file but don't launch the crash reporting UI or send the report to the server. Minidumps will be stored in the user's profile directory, in a subdirectory named "minidumps".

Maybe we should get a mozcrash equivalent coded in Rust first to handle those reports.

> > I'm personally in favor of a more robust crash handling via
> > geckodriver! And this would be a good start.
> 
> Is this something you have to set before Firefox starts, e.g. in
> geckodriver, or something we can (also) enable at runtime?

Those env variables have to be set before you start the process, or pass them directly into the process when creating it.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago

Thanks for the detailed feedback.

(In reply to Henrik Skupin (:whimboo) from comment #2)

> (In reply to Andreas Tolfsen ‹:ato› from comment #1)
> 
> > Is this something you have to set before Firefox starts, e.g. in
> > geckodriver, or something we can (also) enable at runtime?
> 
> Those env variables have to be set before you start the process,
> or pass them directly into the process when creating it.

Let’s set it up in geckodriver then.  I’m not sure if this
should live in the Rust crate mozrunner or in geckodriver.  You’ll
probably have to look at the code and see where it fits best.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

mozrunner has the `build_command()` method which adds already two environment variables which are necessary at this level. But it doesn't allow in `start()` to pass in custom environment variables:

https://github.com/jgraham/rust_mozrunner/blob/master/src/runner.rs#L114-L125
https://github.com/jgraham/rust_mozrunner/blob/master/src/runner.rs#L96

So I would say that we add the env argument to mozrunner and then lets geckodriver pass those variables over. The crashreporter ones should not be part of the runner.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 7 years ago

PR for mozrunner is here: https://github.com/jgraham/rust_mozrunner/pull/11

I will continue on this bug once the patch got landed and a new release of mozrunner is out.

	James Graham [:jgraham]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8900228 [details]
Bug 1388249 - Upgrade to mozrunner 0.4.2.

https://reviewboard.mozilla.org/r/171600/#review176746

	James Graham [:jgraham]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8900229 [details]
Bug 1388249 - Vendor in mozrunner 0.4.2 crate.

https://reviewboard.mozilla.org/r/171602/#review176748

	James Graham [:jgraham]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8900230 [details]
Bug 1388249 - Add crashreporter environment variables to geckodriver.

https://reviewboard.mozilla.org/r/171604/#review176750

::: testing/geckodriver/src/marionette.rs:455
(Diff revision 1)
>  
>          // double-dashed flags are not accepted on Windows systems
>          runner.args().push("-marionette".to_owned());
>  
> +        // https://developer.mozilla.org/docs/Environment_variables_affecting_crash_reporting
> +        runner.envs().insert("MOZ_CRASHREPORTER".to_string(), "1".to_string());

This is fine, but I wonder if there should be a moz:firefoxOptions capability to prevent this behaviour. Please file a followup for that.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8900230 [details]
Bug 1388249 - Add crashreporter environment variables to geckodriver.

https://reviewboard.mozilla.org/r/171604/#review176802

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8900230 [details]
Bug 1388249 - Add crashreporter environment variables to geckodriver.

https://reviewboard.mozilla.org/r/171604/#review176750

> This is fine, but I wonder if there should be a moz:firefoxOptions capability to prevent this behaviour. Please file a followup for that.

I filed bug 1393036 for discussion if this is necessary.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 7 years ago

Given that push to try currently fails via mozreview here a push from my local checkout:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6405e90a72fbd440148b82f2ac60c19aa1cbfd43

	Pulsebot
	Comment 15 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab2ea163058f
Upgrade to mozrunner 0.4.2. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/690620d3758f
Vendor in mozrunner 0.4.2 crate. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/4d0664f38990
Add crashreporter environment variables to geckodriver. r=jgraham

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ab2ea163058f
https://hg.mozilla.org/mozilla-central/rev/690620d3758f
https://hg.mozilla.org/mozilla-central/rev/4d0664f38990