1360493 - Write a test for bug 1358151, asserting that Firefox launches without hanging

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Carl Corcoran [:ccorcoran]]

Bug 1358151 describes a race condition at Firefox startup. This hang was originally accidentally exposed in appupdate tests because they launch the browser.

Write a test that targets this condition specifically.

Comment 1

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1360493 write a test asserting that Firefox launches without hanging;

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

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143188

::: toolkit/xre/test/test_launch_without_hang.js:12
(Diff revision 1)
> +
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, manager: Cm, results: Cr, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/FileUtils.jsm");

Is this actually used?

::: toolkit/xre/test/test_launch_without_hang.js:26
(Diff revision 1)
> +
> +// Sets a group of environment variables, returning the old values.
> +// newVals AND return value is an array of { key: "", value: "" }
> +function setEnvironmentVariables(newVals) {
> +  let oldVals = [];
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);

nit: s/Components.interfaces/Ci/

::: toolkit/xre/test/test_launch_without_hang.js:48
(Diff revision 1)
> +
> +function getFirefoxExecutableFilename() {
> +  let executableName = AppConstants.MOZ_APP_NAME;
> +
> +  if (AppConstants.platform === "macosx")
> +      executableName = "firefox";

This patch as is will run this test on other applications than Firefox. Not sure what the best way to deal with this is but it should be possible to make it so this test works with other apps.

::: toolkit/xre/test/test_launch_without_hang.js:70
(Diff revision 1)
> +
> +// Takes an executable and arguments, and wraps it in a call to the system shell.
> +// Technique adapted from \toolkit\mozapps\update\tests\unit_service_updater\xpcshellUtilsAUS.js
> +// to avoid child process console output polluting the xpcshell log.
> +// returns { file: (nsIFile), args: [] }
> +function wrapLaunchInShell(file, args) {

It has been a very long time since this was written. It might be possible to just launch the binary with nsIProcess and then you'd be able to take advantage of it to kill the process. I think it would  be worth investigating.

::: toolkit/xre/test/test_launch_without_hang.js:96
(Diff revision 1)
> +function terminateFirefox(completion) {
> +  let executableName = getFirefoxExecutableFilename();
> +  let file;
> +  let args;
> +
> +  if (AppConstants.platform === "win") {

see previous comment

::: toolkit/xre/test/test_launch_without_hang.js:223
(Diff revision 1)
> +  let triesStarted = 1;
> +
> +  let handler;
> +
> +  let testTry = function testTry() {
> +    let shell = wrapLaunchInShell(getFirefoxExecutableFile(), ["-no-remote", "-test-process-updates"]);

-test-process-updates isn't available when MOZ_UPDATER isn't defined so a different arg will need to be used.

::: toolkit/xre/test/xpcshell.ini:9
(Diff revision 1)
> +
> +[DEFAULT]
> +tags = native
> +
> +[test_launch_without_hang.js]
> +run-sequentially = Has to launch Firefox binary

Should be "Has to launch application binary" or the test needs to exclude non-Firefox.

Comment 3

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

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

Comment 4

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143188

> Is this actually used?

Nope, removed.

> nit: s/Components.interfaces/Ci/

fixed

> This patch as is will run this test on other applications than Firefox. Not sure what the best way to deal with this is but it should be possible to make it so this test works with other apps.

I am not sure how to test/verify it, but I changed to using AppConstants.MOZ_APP_NAME as the app name instead of "firefox". I am not sure if that's the right approach.

> It has been a very long time since this was written. It might be possible to just launch the binary with nsIProcess and then you'd be able to take advantage of it to kill the process. I think it would  be worth investigating.

I tried both ways; wrapping in cmd.exe really does clean up the log quite a bit.

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143666

::: toolkit/xre/nsAppRunner.cpp:3988
(Diff revision 2)
>      // Leak it with extreme prejudice!
>      PR_SetEnv(ToNewCString(desktopStartupEnv));
>    }
>  #endif
>  
> +  if (CheckArg("test-launch-without-hang")) {

Please add a comment for why this is necessary.

::: toolkit/xre/test/test_launch_without_hang.js:44
(Diff revision 2)
> +  return oldVals;
> +}
> +
> +
> +function getFirefoxExecutableFilename() {
> +  if (AppConstants.platform === "win")

nit: please use braces as you have done elsewhere
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

::: toolkit/xre/test/test_launch_without_hang.js:117
(Diff revision 2)
> +                   "Terminate firefox process exit value should be 0");
> +      Assert.equal(aTopic, "process-finished",
> +                   "Terminate firefox observer topic should be " +
> +                   "process-finished");
> +
> +      if (completion)

nit: please use braces as you have done elsewhere
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

::: toolkit/xre/test/test_launch_without_hang.js:218
(Diff revision 2)
> +    { key: "XPCOM_DEBUG_BREAK", value: "stack-and-abort" }
> +  ];
> +
> +  let triesStarted = 1;
> +
> +  let handler;

nit: why not just?
let handler = function launchFirefoxHandler(okToContinue) {

::: toolkit/xre/test/test_launch_without_hang.js:228
(Diff revision 2)
> +    launchProcess(shell.file, shell.args, env, APP_TIMER_TIMEOUT_MS, handler, triesStarted);
> +  };
> +
> +  handler = function launchFirefoxHandler(okToContinue) {
> +    triesStarted++;
> +    if ((triesStarted <= TRY_COUNT) && okToContinue)

nit: please use braces as you have done elsewhere
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143670

I was able to run this on c-c Thunderbird successfully. One issue I had was that printing to the log didn't print it to the console as it does with m-c so I called do_throw just before do_test_finished which did print everything to the console.

r=me with the nits I pointed out.

Comment 7

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143666

> nit: why not just?
> let handler = function launchFirefoxHandler(okToContinue) {

I wasn't sure the circular dependency between handler & testTry would work otherwise. My lack of Javascript understanding is showing. Fixed.

Comment 8

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

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

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143860

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/420cf7ba089f
write a test asserting that Firefox launches without hanging; r=rstrong

Comment 11

[Carsten Book [:Tomcat]]

sorry Robert had to back this out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=100025903&repo=autoland in the test

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d18aa0ba5171
Backed out changeset 420cf7ba089f for eslint failure in own test

Comment 13

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

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

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review144178

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea1bbc02a7ea
write a test asserting that Firefox launches without hanging; r=rstrong

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea1bbc02a7ea