Include the launcher process in a job if "--wait-for-browser" is set
Categories
(Firefox :: Launcher Process, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox96 | --- | fixed |
People
(Reporter: toshi, Assigned: toshi)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Puppeteer cannot terminate Firefox because it has a handle to the launcher process.
Assignee | ||
Comment 1•3 years ago
|
||
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.
Comment 3•3 years ago
|
||
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
Comment 4•3 years ago
|
||
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
Comment 5•3 years ago
|
||
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!
Assignee | ||
Comment 6•3 years ago
|
||
(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.
Comment 7•3 years ago
|
||
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
.
Assignee | ||
Comment 8•3 years ago
|
||
(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#2677So 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.
Assignee | ||
Comment 9•3 years ago
|
||
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.
Comment 10•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 11•3 years ago
|
||
bugherder |
Comment 12•3 years ago
|
||
Toshihito thanks a lot for the very quick fix! It works great now.
Description
•