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)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: irakli, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.04 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
Alex, did you already work on something like this?
Flags: needinfo?(poirot.alex)
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
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)
Priority: -- → P3
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
Comment on attachment 8416459 [details] [diff] [review] exitcode.patch So who would be able to review this?
Attachment #8416459 -
Flags: review?
Comment 10•10 years ago
|
||
(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?
Comment 11•10 years ago
|
||
(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).
Comment 12•10 years ago
|
||
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.
Flags: needinfo?(dtownsend+bugmail)
Comment 13•10 years ago
|
||
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
Comment 14•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
(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.
Comment 17•10 years ago
|
||
any chance you have some time now to review this?
Flags: needinfo?(benjamin)
Comment 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
> I'm still not clear that this is something good to do at all.
Why?
Florent
Comment 20•9 years ago
|
||
(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)
Comment 21•9 years ago
|
||
Also see http://www.helpandmanual.com/help/hm_advanced_commandline_exitcodes.htm
Comment 22•9 years ago
|
||
https://en.wikipedia.org/wiki/Exit_status
Comment 23•9 years ago
|
||
(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
Comment 24•9 years ago
|
||
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)
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Comment 25•8 years ago
|
||
Comment on attachment 8416459 [details] [diff] [review] exitcode.patch Dropping review request without owner on long-closed bug.
Attachment #8416459 -
Flags: review?
See Also: → 1449637
You need to log in
before you can comment on or make changes to this bug.
Description
•