Closed Bug 1740619 Opened 3 years ago Closed 3 years ago

Include the launcher process in a job if "--wait-for-browser" is set

Categories

(Firefox :: Launcher Process, enhancement)

Unspecified
Windows
enhancement

Tracking

()

VERIFIED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: toshi, Assigned: toshi)

References

Details

Attachments

(1 file)

Puppeteer cannot terminate Firefox because it has a handle to the launcher process.

When the launcher process is enabled, Puppeteer, or any other automation tools, cannot
have control of the lifetime of the browser process even though the --wait-for-browser
option is used.

This patch is to include the launcher process and the browser process to a job to enable
a launcher of the launcher process like Puppeteer to terminate the application by terminating
the launcher process if --wait-for-browser is set.

Pushed by tkikuchi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b5ae0c3e9c56 Include the launcher process in a Job object when "--wait-for-browser" is set. r=mhowell

Backed out 2 changesets (Bug 1740619, Bug 1740805) for causing multiple test failures.
Backout link
Push with failures
Failure Log - c
Failure Log - X2
Failure Log - Mn

Flags: needinfo?(tkikuchi)

Even with the patch backed out I'll use the following try build to test if it works now with Puppeteer:
https://treeherder.mozilla.org/jobs?repo=try&revision=209877c741e432b135f7867881150ad191c7783a

With this change included it actually works like a charm! Thanks a ton! None of the known failures when the Firefox process is killed are visible anymore. The only Puppeteer unit test that is failing here is removing default arguments which includes --wait-for-browser, and as such fails without the patch from 1740805. So with both patches landed we should be fine!

Thanks a lot!

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

With this change included it actually works like a charm! Thanks a ton! None of the known failures when the Firefox process is killed are visible anymore. The only Puppeteer unit test that is failing here is removing default arguments which includes --wait-for-browser, and as such fails without the patch from 1740805. So with both patches landed we should be fine!

Thanks a lot!

Thank you for confirming the patch! I also confirmed this change introduced the marionette test failure in comment 3 locally. Let me figure out how to reconcile this conflict.

Note that this test is triggering a shutdown from within Firefox via Services.startup.quit(mode):
https://searchfox.org/mozilla-central/rev/6deb8b6af57a8b5b6b1bcb143ea498e566475d8d/remote/marionette/driver.js#2677

So I assume you should be able to reproduce it even without Marionette by running that command in the devtools browser toolbox, when having Firefox started with mach run --marionette.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

Note that this test is triggering a shutdown from within Firefox via Services.startup.quit(mode):
https://searchfox.org/mozilla-central/rev/6deb8b6af57a8b5b6b1bcb143ea498e566475d8d/remote/marionette/driver.js#2677

So I assume you should be able to reproduce it even without Marionette by running that command in the devtools browser toolbox, when having Firefox started with mach run --marionette.

Yeah, I found the problem is Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart) does not restart Firefox with this patch.

I think I got it. When the browser process restarts a new Firefox instance here, the new instance is automatically included in the job that has been created by the launcher process. When the browser process exits, the new instance is also terminated.

Pushed by tkikuchi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b9cbddea755d Include the launcher process in a Job object when "--wait-for-browser" is set. r=mhowell
Flags: needinfo?(tkikuchi)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

Toshihito thanks a lot for the very quick fix! It works great now.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: