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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ccorcoran, Assigned: ccorcoran)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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 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.
Attachment #8868257 - Flags: review?(robert.strong.bugs) → review-
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 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
Attachment #8868257 - Flags: review?(robert.strong.bugs) → review+
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 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.
Keywords: checkin-needed
Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review143860
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/420cf7ba089f
write a test asserting that Firefox launches without hanging; r=rstrong
Keywords: checkin-needed
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
Flags: needinfo?(ccorcoran)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d18aa0ba5171
Backed out changeset 420cf7ba089f for eslint failure in own test
Comment on attachment 8868257 [details]
Bug 1360493 write a test asserting that Firefox launches without hanging;

https://reviewboard.mozilla.org/r/139842/#review144178
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea1bbc02a7ea
write a test asserting that Firefox launches without hanging; r=rstrong
https://hg.mozilla.org/mozilla-central/rev/ea1bbc02a7ea
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(ccorcoran)
You need to log in before you can comment on or make changes to this bug.