Closed Bug 894697 Opened 11 years ago Closed 8 years ago

Implement platform API to exit firefox with a specific exit code

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: irakli, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

At the moment there is no way to exit firefox process with non 0 exit code. This requires ugly workarounds on SDK side:

https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/system.js#L59-L68

We should implement platform API to exit with specific exit code.
Alex, did you already work on something like this?
Flags: needinfo?(poirot.alex)
I don't remmember having worked on that. I mostly focused on console topic, especially on windows.

I'd tend to say it requires tweaking each platform nsINativeAppSupport class Exit method to be tuned:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportWin.cpp#775

They are called from nsIAppStartup.quit() method via the quit-application event:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#431
Flags: needinfo?(poirot.alex)
I have some recollections of this being discussed in the past and bsmedberg being against it. Benjamin, what do you think to this?
Flags: needinfo?(benjamin)
The only discussion I can remember/find was bug 692161, which was mainly closed for lack of need. I'm not completely opposed to this, but I still don't see a particularly good use-case for it.

Technically this would go on nsIAppStartup; nsINativeAppSupport doesn't need to be involved at all, since it's the return code from XRE_main that actually matters (note that in the future it will be the exit code from _exit since we won't be doing full shutdown, bhg 662444).
Flags: needinfo?(benjamin)
Attached patch exitcode.patchSplinter Review
So here is my attempt to add such an exit code.
Please be gentle, I haven’t done any C++ in years :-)

Also, I see some tests that use `quit()` but none that can actually assert the exit code. How would I go about writing such a test?
Attachment #8416459 - Flags: review?(benjamin)
Comment on attachment 8416459 [details] [diff] [review]
exitcode.patch

I really don't have time to review this and I don't think it's a high priority.
Attachment #8416459 - Flags: review?(benjamin)
we have seen lots of intermittent test failures because we are writing 'OK' to files as a workaround to this.. re-prioritize?
OS: Mac OS X → All
Priority: P3 → --
Hardware: x86 → All
(In reply to Tomislav Jovanovic [:zombie] from comment #7)
> we have seen lots of intermittent test failures because we are writing 'OK'
> to files as a workaround to this.. re-prioritize?

What makes you say that that is the cause?
Comment on attachment 8416459 [details] [diff] [review]
exitcode.patch

So who would be able to review this?
Attachment #8416459 - Flags: review?
(In reply to Dave Townsend [:mossop] from comment #8)
> What makes you say that that is the cause?

sorry for not being clear enough, i was in this state of mind after spending half the weekend on the issue from bug 1006043, and i read Jordan's comment so i suspect something similar (lingering sub-processes) might be the issue as in bug 942111 comment 194.

also, this would probably make jpm test suite easier/cleaner and maybe even help with travis?
(In reply to Tomislav Jovanovic [:zombie] from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > What makes you say that that is the cause?
> 
> sorry for not being clear enough, i was in this state of mind after spending
> half the weekend on the issue from bug 1006043, and i read Jordan's comment
> so i suspect something similar (lingering sub-processes) might be the issue
> as in bug 942111 comment 194.

I'm not sure that either of those cases are to do with the resultfile we use in our tests. Likely our biggest problem right now is that we do file logging and results at all. No other test harness does it so I've never figured out why we need to. It's possible we need an updated mozrunner in order to properly handle stdout from the Firefox process though (bug 897370).
Hi,

I'm highly interesting by the support of an exit code for my "almost headless" browser, SlimerJs https://github.com/laurentj/slimerjs/issues/50

I'll try to do a first review of the patch this week.
Blocks: slimerjs
Flags: needinfo?(dtownsend+bugmail)
This isn't useful for the SDK. What is this needed for?
Component: General → Startup and Profile System
Flags: needinfo?(dtownsend+bugmail) → needinfo?(arpad.borsos)
Product: Add-on SDK → Toolkit
Testing frameworks that use slimerjs for example can indicate failure status with that.
Things like casperjs or mocha-phantomjs can use slimerjs/gecko as a dropin replacement for phantomjs/webkit.

Since phantomjs has an ancient implementation for standards (no indexeddb or newer js features), replacing it with something gecko based is more than ever.
Flags: needinfo?(arpad.borsos)
(In reply to Dave Townsend (Gone till June 16th) [:mossop] from comment #13)
> This isn't useful for the SDK. What is this needed for?

It actually is.

We do terrible things in cfx to workaround the fact that we can't just exit process with error code on test failures. I would really prefer not to repeat same hacks in JPM which is why I submitted this bug in first place.
(In reply to Tomislav Jovanovic [:zombie] from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #8)
> also, this would probably make jpm test suite easier/cleaner and maybe even
> help with travis?

This is indeed true, at the moment jpm is reading the stdout waiting for error messages or a success message to be printed when it should just use the firefox exit code which is more reliable.
any chance you have some time now to review this?
Flags: needinfo?(benjamin)
I'm still not clear that this is something good to do at all. I'll happily defer to Dave if he has time to understand this, but otherwise I really think this should be WONTFIX.
Flags: needinfo?(benjamin)
> I'm still not clear that this is something good to do at all.

Why?

Florent
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> I'm still not clear that this is something good to do at all. I'll happily
> defer to Dave if he has time to understand this, but otherwise I really
> think this should be WONTFIX.

It's very typical for applications to exit with an exit code of 1, or something none 0, if there was some issue, like failing tests.

https://msdn.microsoft.com/en-us/library/ms194959%28v=vs.100%29.aspx

Is there some issue you have with providing this functionality?
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> I'm still not clear that this is something good to do at all. I'll happily

hihi, i maintain the npm package https://www.npmjs.com/package/phantomjs-lite,
and i just got bit by this bug.
i'm trying to add auto-detection for slimerjs, and found out i have to do some fugly checks on console.log to verify slimerjs is detected and usable.

1. auto-detecting slimerjs binary exists and executable is no problem.
2. auto-detecting slimerjs is CORRECTLY configured and USABLE is not easy to do without exit-code status checking (e.g. slimerjs v0.9.4 will pass simple executable test, but on codeship.io, u won't realize its incompatible with they're recently upgraded firefox 35, until u actually run the browser tests)

here's a valid use case to auto-detect slimerjs for a build system that normally uses phantomjs:

// one-line file auto-detect.js
phantom.exit(111);


# in shell
slimerjs auto-detect.js
if [ "$?" = 111 ]
then
  # run phantomjs browser tests on slimerjs as well
fi
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.
Flags: needinfo?(benjamin)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Comment on attachment 8416459 [details] [diff] [review]
exitcode.patch

Dropping review request without owner on long-closed bug.
Attachment #8416459 - Flags: review?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: