Closed Bug 442393 Opened 12 years ago Closed 11 years ago

nsIProcess.kill() does not work on Win32

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: hippytrail, Assigned: jboston)

References

Details

(Keywords: addon-compat, dev-doc-complete, fixed1.9.1)

Attachments

(3 files, 8 obsolete files)

21.28 KB, patch
benjamin
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.55 KB, patch
Details | Diff | Splinter Review
814 bytes, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0

After calling proc.run() with blocking set to false my process starts correctly but a subsequent call to proc.kill() does nothing at all.


Reproducible: Always

Steps to Reproduce:
1. proc = Components.classes["@mozilla.org/process/util;1"].createInstance(Components.interfaces.nsIProcess);
2. proc.init('D:\\bin\\bzip2.exe');
3. pid = proc.run(false, ['-dk', 'foo.bz2'], 2);
4. tproc.kill();

Actual Results:  
Process continues to run with no error result or exceptions seen.


Expected Results:  
Process should terminate.
Summary: nsIProcess does not work on Win32 → nsIProcess.kill() does not work on Win32
Component: General → XPCOM
Product: Firefox → Core
QA Contact: general → xpcom
I had the same problem on Windows XP. I would add to that nsIProcess.run() returns a pid value of zero for me everytime.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is because the current non-blocking implementation is 'fire and forget' and does not retain any information about the child process. Calling the 'pid' method on a non-blocking process doesn't work either.

To be useful for wrapping command line tools, this component needs more implementation love.
This eliminates the Windows specific code. Instead all platforms now use the NSPR to create processes. Additionally, the logic for blocking is changed. Non-blocking processes are no longer detached. This fixes the kill method. 

This patch does NOT appear to cause a regression to the problem where starting a console app causes a console window to open. The behavior is correct.

This patch is the first step in a larger project of implementing inter-process communication.
Attachment #340506 - Flags: review?
Attachment #340506 - Flags: review? → review?(benjamin)
Rob, I remember reviewing something similar for you recently... did that land yet? Can you coordinate your changes with James?
(In reply to comment #4)
> Rob, I remember reviewing something similar for you recently... did that land
> yet? Can you coordinate your changes with James?
I haven't landed it yet and I'm considering a different approach for the PFS bug so I have no problem with this change.

Though this patch fixes the Win32 case it doesn't appear to fix it for Mac OS X which should probably be done at the same time.
I didn't realize that the NSPR hasn't implemented the code for starting Mac processes:
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/mac/macthr.c#442

It looks as though the code actually creates a Mac process and then makes a call to the NSPR that does nothing. Hmm...

I'm going to revise the patch to make the Mac code work and then see if I can port it to NSPR.
Attachment #340506 - Flags: review?(benjamin)
This is a revision of my previous patch. I was able to use the NSPR for Mac processes after all. 

Removed all Windows and MacOS specific code. All platforms now definitely use the NSPR.

Changed the blocking logic. Non-blocking processes are not detached. This retains mProcess reference until it is used by the kill method.

Added destructor code to detach a process that has not been killed.

Tested on Vista, XP, OSX Leopard, Fedora8. Tested starting/killing a gui app, a gui app with arguments, and a console app. Run and kill work now on all tested platforms.
Attachment #340506 - Attachment is obsolete: true
Attachment #341723 - Flags: review?
Attachment #341723 - Flags: review? → review?(benjamin)
Comment on attachment 341723 [details] [diff] [review]
Revised patch. Fixes OSX as well as other platforms now.

This patch looks fine. However, before it lands, I really think we need tests. We should at least test the following:

1) that exit codes and crashing processes are reflected correctly
2) that blocking operates correctly

I think you can probably do this by building a simple test executable which waits for a few seconds and returns an exit code specified on the command line, and perhaps programmatically crashes, and then launching that executable from an xpcshell test.
Attachment #341723 - Flags: review?(benjamin) → review+
Attached patch Unicode support for Windows (obsolete) — Splinter Review
My approach depends on Unicode support in the NSPR, but it doesn't exist and will take a long time to achieve.

This new patch is the first step in bringing Unicode support to nsIProcess. This is a Windows only solution that enables the use of Unicode arguments and allows starting and stopping of Windows processes. It does not include a Unix solution as yet.

It is based on an extension developed by dafi:
http://dafizilla.wordpress.com/2008/10/08/nsiprocess-windows-and-unicode/

I plan to port some code for Unix process creation from the NSPR to nsIProcess.
Attachment #341723 - Attachment is obsolete: true
I really think we should break out the unicode work into a separate bug. Getting support for kill() is a big plus of the current implementation.

Furthermore, I would recommend that this patch, with GetPid support, be used:
http://jamesboston.ca/patches/patch102808.txt

I don't think the approach is hacky at all - you have seen the current "run" method right?

That patch gives us a cleaner nsProcess implementation and gives us:
* kill() support
* run(...) returns the pid
* pid property works (I would consider returning a safe invalid pid instead of causing and error)

I'd LOVE to get those improvements into 1.9.1, but time is not on our side. Other parts of nsIProcess can be fixed as time permits.
I propose a new patch that takes a more conservative approach. It might be what you are looking for if you need something to go into the tree soon.

It doesn't throw out the existing Windows specific code. Therefore, it should not cause any new problems when creating processes. A little bit of code has been added to return the PID on all platforms and to implement an isRunning() method for all platforms (except WinCE). The kill() method should work across all platforms now. 

Some Mac code has been trimmed for cases where there are no arguments, but NSPR code appears to handle no args fine.

I haven't got an xpcshell unit test for this yet. The problem that is holding me up is that I need at least two binary executables to play with and I'm not sure how add those to the tree. I'm trying to figure out how to include them as source code that gets compiled when tests are enabled.
Attachment #347521 - Attachment is obsolete: true
(In reply to comment #11)

> I haven't got an xpcshell unit test for this yet. The problem that is holding
> me up is that I need at least two binary executables to play with and I'm not
> sure how add those to the tree. I'm trying to figure out how to include them as
> source code that gets compiled when tests are enabled.

You should be able to use xpcshell itself, along with some simple JS scripts, to test the features.

Also, I think isRunning should be a property, not a method.
James, I see you have a set of unit tests in a patch (http://jamesboston.ca/patches/patch112808.txt) on your blog (http://jamesboston.ca/cms/node/102).  How close are these to being ready to post here and get reviewed?
Very close. I solved the problem of creating some binaries to play with. I have another project on the go, but after that I hope to upload a patch that includes a full set of unit tests. If I have access to a Mac, it should be ready by Friday or Saturday.
Attached patch Bug fix that includes unit test (obsolete) — Splinter Review
This new patch is:
* Tested on XP SP3, Ubuntu 8.10 and Mac OS X 10.5.6
* Fixes kill.
* Captures exit code of blocking processes.
* Returns PID.
* Implements new attribute isRunning to poll running processes
* Includes cross platform xpcshell unit test
Attachment #349631 - Attachment is obsolete: true
Attachment #358763 - Flags: review?
Attachment #358763 - Flags: review? → review?(benjamin)
Comment on attachment 358763 [details] [diff] [review]
Bug fix that includes unit test

>-DIRS        = dynamic services external
>+DIRS        = dynamic services external simple-programs

Is there a particular reason you need to have a separate directory for these programs. You can't use SIMPLE_PROGRAMS in this directory?

>diff -r 4f398c5824e1 xpcom/tests/simple-programs/Makefile.in

Needs a license block

>+LIBS += \
>+    $(NULL)

This doesn't make sense, remove it ;-)

>diff -r 4f398c5824e1 xpcom/tests/simple-programs/TestEndlessLoop.cpp

>+int main () {
>+
>+  while (1);
>+  return 0;

I'm a little worried this will eat CPU time and mess with the unittest VMs. Is it possible to have it read from stdin instead? That will be a blocking call, but won't use CPU.

>diff -r 4f398c5824e1 xpcom/threads/nsIProcess.idl

> [scriptable, uuid(9da0b650-d07e-4617-a18a-250035572ac8)]

You need to generate a new uuid since the interface is changing.

>+//Destructor
>+nsProcess::~nsProcess()
>+{
>+#if defined(XP_WIN) && !defined (WINCE) /* wince uses nspr */

This "wince uses nspr" comment is getting tiring. Why not create a set of defines for the specific things you're testing:

PROCESSMODEL_WINAPI
PROCESSMODEL_NSPR
PROCESSMODEL_X

>-#if defined(XP_MACOSX)
>-    if (count == 0) {
>-        FSSpec resolvedSpec;
>-        OSErr err = noErr;

Have you looked at history to figure out why mac didn't use NSPR before? I suspect this is fine, but I don't want to get caught causing a regression that was fixed before.

Really close!
Attachment #358763 - Flags: review?(benjamin) → review-
This ammended patch:
* moves the test apps into the xpcom/tests
* uses a test app that blocks by getting input rather than an endless loop
* changes the uuid of nsIProcess
* adds a license block to the unit test
* creates a define for cases where XP_WIN is defined and WINCE is not

I believe that the removal of the Mac specific code is safe because it was copied into nsProcessCommon.cpp from nsProcessMac.cpp when the later was removed from the tree but NSPR did not correctly support Mac processes:
https://bugzilla.mozilla.org/show_bug.cgi?id=270107

The unit test I provide shows that NSPR can handle Mac processes (with or without arguments, blocking and non-blocking).
Attachment #358763 - Attachment is obsolete: true
Attachment #365651 - Flags: review?
Attachment #365651 - Flags: review? → review?(benjamin)
Attachment #365651 - Flags: review?(benjamin) → review+
Comment on attachment 365651 [details] [diff] [review]
Ammended according to reviewer's comments

>diff -r cee4777f599d xpcom/threads/nsProcess.h

>+#if defined(XP_WIN) && !defined (WINCE) /* wince uses nspr */

use PROCESSMODEL_WINAPI here

>diff -r cee4777f599d xpcom/threads/nsProcessCommon.cpp

+#elif defined(XP_MACOSX)
 #include <Processes.h>
 #include "nsILocalFileMac.h"
+#include <signal.h>

Why do we need special headers for mac still? Looks like you removed all the mac-specific code in favor of NSPR.
Attached patch Minor ammendments. (obsolete) — Splinter Review
I have made those changes. The OSX header stuff doesn't belong there anymore.
Attachment #365651 - Attachment is obsolete: true
Blocks: 478687
Assignee: nobody → jamesboston
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/a632978b885f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
backed out

http://hg.mozilla.org/mozilla-central/rev/2b9ed1c47f46
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch The problem explained (obsolete) — Splinter Review
The reason it fails on Linux is because test_bug476919.js uses nsIProcess run and checks for a return value of 0 as a sign of success. However, this is useless with the old code. Run would *always* return zero. It is a check that can never fail. With my patch run returns a non-zero integer (the PID) if it is successful (as per the idl) and zero if it fails. My unit test checks for this behavior and verifies that this is what happens on all platforms. I recommend that rather than change my patch it makes more sense to change test_bug476919.js. I have uploaded a patch that does this. However, I will prepare an alternate patch that changes run to 0=success -1=fail in case in is wanted.
Attachment #365940 - Attachment is obsolete: true
No longer depends on: 481964
I've consulted with Ted, the author of the other unit test, and he is okay with that one line change to it.
Keywords: checkin-needed
Keywords: checkin-needed
The new patch here lacks the new files, I'm not sure if you meant to make changes to those. Would be nice to have a new ready-to-go patch.
I've changed the run method to a void return. I've also changed code elsewhere in the tree affected by this. All xpcom unit tests pass now.
Attachment #366034 - Attachment is obsolete: true
Attachment #366315 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attached patch Minor fix.Splinter Review
This fixes a minor problem with the last patch.
bustage fix pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/f2816d1a110d
James, thanks for thinking about OS/2. However there is another occurrence of PRINT32 pid in nsMimeInfoOS2.cpp.
E:/usr/src/hg/mozilla-central/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp: In member function 'virtual nsresult nsMIMEInfoOS2::LoadUriInternal(nsIURI*)':
E:/usr/src/hg/mozilla-central/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:547: error: no matching function for call to 'nsIProcess::Run(int, const char* [3], PRInt32&, PRUint32*)'
../../dist/include/xpcom/nsIProcess.h:49: note: candidates are: virtual nsresult nsIProcess::Run(PRBool, const char**, PRUint32)
make.exe[5]: *** [nsMIMEInfoOS2.o] Error 1

Fixing it like you did in attachment 366354 [details] [diff] [review] helps.
Could as well open a new bug if it's not appropriate to add it here. Thanks
Attachment #366412 - Flags: review?(jamesboston)
Sorry. I didn't mean to cause an error on OS/2. Unfortunately I don't have commit priviledges. But I'll talk to someone who does and see if your patch can be pushed from here (since it is a minor edit) or if a new bug needs to be filed.
Why was isRunning implemented as an integer attribute? Surely a boolean would be more appropriate?
Filed bug 483626 to clean up some issues here.
Depends on: 483626
Flags: in-testsuite+
Comment on attachment 366412 [details] [diff] [review]
another build fix for OS2 [checkin comment 34]

Pushed this build fix to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/032ffaec3006
(Converted the patch to Unix linebreaks before applying...)
Attachment #366412 - Attachment description: another build fix for OS2 → another build fix for OS2 [checkin comment 34]
Attachment #366412 - Flags: review?(jamesboston)
Attachment #366315 - Flags: approval1.9.1?
Comment on attachment 366315 [details] [diff] [review]
Changes run to void return

I would like to land this on 1.9.1. It introduces a very small API change, but I do not believe it will impact JS callers. It adds more tests to this API than existed before. Will assume any approval applies to the follow up fixes too.
Attachment #366315 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 366315 [details] [diff] [review]
Changes run to void return

a191=beltzner for this and the associated buildfixes in the onward patches.
Sheppy, this is an IID change for nsIProcess which should be called out in late-compate FF3.5 changes. It would only affect extension authors which use nsIProcess from binary components, which is pretty unusual.
Keywords: late-compat
Re Comment #32; I agree with Mossop here; isRunning should be a boolean, shouldn't it?
The documentation has been updated; see https://developer.mozilla.org/En/NsIProcess
(In reply to comment #38)
> Re Comment #32; I agree with Mossop here; isRunning should be a boolean,
> shouldn't it?

Sorry I followed that up and fixed it in bug 483626. I've fixed the docs accordingly.
You need to log in before you can comment on or make changes to this bug.