Closed
Bug 1449637
Opened 7 years ago
Closed 4 years ago
Allow nsAppExitEvent to set main process exit code
Categories
(Toolkit :: Startup and Profile System, enhancement, P3)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
DUPLICATE
of bug 1675329
People
(Reporter: ato, Unassigned)
References
Details
Marionette calls Services.startup.quit(Ci.nsIAppStartup.eForceQuit) if
it is unable to bind to the designated port on startup. Internally,
this calls nsAppStartup::Quit which despatches a nsAppExitEvent
[1] to the curren thread.
As far as I can tell this eventually ends up in the IPC subsystem
instructing various different subprocesses and threads to finish
their work and join.
As whimboo points out in [2] it would be rather useful if we could use
an exit code to communicate to the parent process of Firefox, in this
case geckodriver, that Firefox failed to start because it failed to
bind to a port. An exit code is the perfect IPC mechanism for doing this.
An API such as Servics.startup.exitCode = N or .setExitCode(N)
would be sufficient. This could be picked up by nsAppExitEvent and
be propagated to the appropriate exit point. I didn’t actually
find the eventual exit point in Gecko.
[1] https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/toolkit/components/startup/nsAppStartup.cpp#103-121
[2] https://github.com/mozilla/geckodriver/issues/979#issuecomment-376941655
Updated•7 years ago
|
Component: General → Startup and Profile System
A previous variant of this discussion occurred in bug 894697.
Reporter | ||
Comment 3•7 years ago
|
||
Thanks, jryans for doing some archeology.
Quoting bsmedberg in https://bugzilla.mozilla.org/show_bug.cgi?id=894697#c24:
> As noted several times, my issue with providing this functionality
> is that it's likely to be incorrect in some edge cases (various
> kinds of profile restarts), and AFAIK we don't have any automated
> test suites which can be used to test this behavior. So unless
> we're going to fix this in all the cases and have automated tests,
> I continue to oppose adding an API which is an untested footgun.
The first point about identifying the edge cases is still valid.
The second about tests is important, but the situation since this
comment has changed and we now have Marionette which can use to test this.
Comment 4•7 years ago
|
||
(In reply to Andreas Tolfsen ‹:ato› from comment #3)
> Thanks, jryans for doing some archeology.
>
> Quoting bsmedberg in https://bugzilla.mozilla.org/show_bug.cgi?id=894697#c24:
>
> > As noted several times, my issue with providing this functionality
> > is that it's likely to be incorrect in some edge cases (various
> > kinds of profile restarts), and AFAIK we don't have any automated
> > test suites which can be used to test this behavior. So unless
> > we're going to fix this in all the cases and have automated tests,
> > I continue to oppose adding an API which is an untested footgun.
>
> The first point about identifying the edge cases is still valid.
I'm not sure there are many edge cases left here, unless you're testing profile selection through the startup profile manager.
> The second about tests is important, but the situation since this
> comment has changed and we now have Marionette which can use to test this.
Yeah definitely valuable.
I think my only question is whether we've explored other options for getting this data in another method, maybe by logging to stdout/stderr and having marionette capture that? Something like that would likely be a lot easier than feeding the exit code through to the eventual main return.
Flags: needinfo?(dtownsend)
Comment 5•7 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #4)
> I think my only question is whether we've explored other options for getting
> this data in another method, maybe by logging to stdout/stderr and having
> marionette capture that? Something like that would likely be a lot easier
> than feeding the exit code through to the eventual main return.
That would not help other processes which started the Firefox process and use the exit code to determine the quit reason of the process. Also the shell would still see a 0, which in case of bug 1370520 wouldn't be appropriate.
So Marionette is just one of those tools, but can be used for testing this proposed feature because it also evaluates the exit code of the handled child process.
Comment 6•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to Dave Townsend [:mossop] from comment #4)
> > I think my only question is whether we've explored other options for getting
> > this data in another method, maybe by logging to stdout/stderr and having
> > marionette capture that? Something like that would likely be a lot easier
> > than feeding the exit code through to the eventual main return.
>
> That would not help other processes which started the Firefox process and
> use the exit code to determine the quit reason of the process.
Presumably there aren't any other such processes currently since we always exit with code 0 right now.
> Also the
> shell would still see a 0, which in case of bug 1370520 wouldn't be
> appropriate.
Why not appropriate?
Comment 7•7 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #6)
> > That would not help other processes which started the Firefox process and
> > use the exit code to determine the quit reason of the process.
>
> Presumably there aren't any other such processes currently since we always
> exit with code 0 right now.
>
> > Also the
> > shell would still see a 0, which in case of bug 1370520 wouldn't be
> > appropriate.
>
> Why not appropriate?
Currently other processes falsely assume that Firefox quit without problems in case of content crashes and MOZ_CRASHREPORTER_SHUTDOWN set. So quitting with a more appropriate exit code, would help to better diagnose problems. Or should we use a different approach as normally shutting down Firefox in those cases? Maybe a MOZ_CRASH? Please see bug 1370520.
Comment 8•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Dave Townsend [:mossop] from comment #6)
> > > That would not help other processes which started the Firefox process and
> > > use the exit code to determine the quit reason of the process.
> >
> > Presumably there aren't any other such processes currently since we always
> > exit with code 0 right now.
> >
> > > Also the
> > > shell would still see a 0, which in case of bug 1370520 wouldn't be
> > > appropriate.
> >
> > Why not appropriate?
>
> Currently other processes falsely assume that Firefox quit without problems
> in case of content crashes and MOZ_CRASHREPORTER_SHUTDOWN set. So quitting
> with a more appropriate exit code, would help to better diagnose problems.
> Or should we use a different approach as normally shutting down Firefox in
> those cases? Maybe a MOZ_CRASH? Please see bug 1370520.
If a main process crash is detectable in the ways you need then that's probably going to be by far the simplest thing to implement.
That said, this request for setting the exit code does seem to keep coming up (and I bet headless mode could make use of it) so I don't have objections to seeing it implemented, I just expect it to be hairy.
My suggestion for the API would be to just add an optional second argument to nsIAppStartup.quit for the exit code. I would also add a new constant to nsIAppStartup for each exit code you want to use, this will help different components not use conflicting exit codes.
Comment 9•7 years ago
|
||
Probably worth getting Nathan's input here too as the owner of XPCOM he may have opinions.
Flags: needinfo?(nfroyd)
Comment 10•7 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #9)
> Probably worth getting Nathan's input here too as the owner of XPCOM he may
> have opinions.
The proposed API sounds completely reasonable and obviously useful to people today. If we can have some tests for it, I don't see why we shouldn't add it.
Flags: needinfo?(nfroyd)
Updated•7 years ago
|
Priority: -- → P3
Comment 11•4 years ago
|
||
So I assume bug 1675329 fixed that, right? So it looks like we can dupe this bug.
Flags: needinfo?(dtownsend)
Comment 12•4 years ago
|
||
I completely forgot about this bug. Looks like I even implemented the same API I suggested two years ago.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(dtownsend)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•