Currently Mozilla's interaction with external programs is limited to launching "helper" applications to handle certain MIME types and protocols (even this feature is not fully implemented yet; see bug 68406). This is a one-way interaction in which a detached process is created. However, the process does not interact back with Mozilla. It would be nice to extend necko so that Mozilla can execute an attached process, write to its standard input and read from its standard output, using either anonymous pipes or TCP socket pairs. This would facilitate the following: - writing external protocol handlers that return HTML and other types of MIME content back to Mozilla for display in the browser window ("client side CGI") - executing arbitrary programs from Javascript using xpconnect and capturing the output of the command as a string. (this is similar to the `backtick` feature found in many shells) These features can be used to add very general protocol handler capabilities to Mozilla and support the registration of new URI schemes (see bug 68406). NSPR already supports attaching anonymous pipes or TCP socket pairs to a process to capture its stdin/stdout. However, an IPC implementation must also allow asynchronous I/O to interact with the process, to be able to run the program without blocking the UI thread. A working C++/XPCOM IPC implementation is already available as part of the external Protozilla project (http://protozilla.mozdev.org). This bug is simply a request by the author of the code that it be incorporated into the Mozilla CVS tree (unless someone can come up with an alternative IPC implementation). You can browse the IPC code directly using the following link: http://mozdev.org/lxr/source/protozilla/pipella/base/ There are three possible locations where this code could be incorporated: - necko - xpcom - mozilla/extensions/ipc Since the first two locations would require module owner approval and more extensive review, in the short term it may be better to import it into mozilla/extensions/ipc and let it bake there for a while. Even if mozilla.org approves this extension, the code still needs to reviewed and super-reviewed, of course. If approved, it is important that the extension be normally enabled for default compilation. Otherwise, there isn't that much benefit to it being a part of the Mozilla code base. Code issues: - interaction with other modules: the code is self-contained and should only affect those who start using the new IPC features - security: I don't think there are any new security issues associated with adding the IPC feature, since it is quite similar to the nsIFile.spawn method, which already allows xpconnect to execute an arbitrary program. (Protozilla itself has other security issues associated with the use of special URLs, which is addressed in bug 68406, but they do not affect the IPC component of Protozilla) - threading and ownership models: the code was written by someone (me!) who only has a limited understanding of how necko and the URL loading process work. (Blame the poor or non-existent documentation for this!) So it is important that the code be reviewed by a necko expert.

cc'ing to dougt@netscape.com per his request.

Adding some keywords. Given that this bug blocks the implementation of Protozilla, and thereby about six 4xp and 3xp bugs relating to new URI schemes, upping severity. neeti: as module owner of Networking, what's your plan for getting this reviewed? Gerv

-->gagan

gagan: Could you possibly let us know roughly when you will be reviewing these patches? Thanks :-) Gerv

my apologies for not getting to this sooner. But I will review this soon. Setting milestone for 1.0

Thanks for agreeing to review it. I'd be happy to work with you to make any changes. You can check out the latest version of the IPC code from mozdev.org using the following commands: cvs -d :pserver:guest@mozdev.org:/cvs login (password is guest) cvs -d :pserver:guest@mozdev.org:/cvs co protozilla/pipella/base At the moment the code compiles with mozilla 0.8 and doesn't include the recent necko/nsIChannel changes. I hope to update it so that compiles with 0.8.1 within a week or so.

I have updated the IPC code to be compatible with the necko/nsIChannel changes. I have attached a source tarball that compiles with Mozilla0.8.1 on Windows, Linux, and Solaris. I hope we can get the IPC extension into the tree for Mozilla0.9.

Gagan, can you give a more concrete plan when you are going to review this? What does the Mozilla1.0 target milestone mean? Does it mean that the IPC extension will have to wait until after Mozilla 0.9.1 (or even after 0.9.2)? How soon is "soon"?

Gagan, can you please outline exactly why protozilla is not a good fit to be checked into the mozilla tree? All noble platforms let you do IPC. Mozilla only has nsIFile spawn which is lame. This addition would extend mozilla the platform. Thanks --pete

My apologies for not having commented on this bug as our discussions went along. But I have been trying to understand how Protozilla works and why it should be in mozilla. I think my asking these questions to Saravanan has been grossly misinterpreted as my denying this to be in mozilla. Let me clarify that what you are trying to achieve is definitely cool and I will be more than happy to see those end results-- however my job is to evaluate whether this is the right way of getting to those end results. So far I haven't been convinced about that. Rather than spam the world with bugzilla emails, I will list my concerns in reply to Saravanan's proposal email. Thanks for hanging in there!

gagan: I didn't think you had made a final decision on Protozilla, but our rather short chat on #mozilla led me to believe that you were inclined to leave it where it is, as a mozdev extension. Since Protozilla users had requested that it be incorporated into mozilla, I posted a note on the Protozilla newsgroup saying that they need to speak up and say why it needs to be part of mozilla. I think rather than discuss this further via bugzilla or IRC, it would be better to discuss it an open mozilla forum - hence my recent posting to n.p.m.netlib, which I cc'd to many of the people on this bug's cc list. Why don't we continue to discuss the issue there until a decision is made and then return to this bug? I apologize for the misunderstanding.

The more I think about this, the more I think that this is just messing around with the fd's of an application that you are going to invoke. Why not just have this functionality in xpcom? So, we have this interface nsIProcess. Why not add the functionality so that you can obtain/set the fd's of a process? Thoughts? Some random comments: nsIIPCService interface: exec and execPipe seam like they belong in the partially complete nsIProcess interface. execPipe shouldn't this use streams? getRandomHex? Why is this even in this interface? getEnv - can this in a more base interface? PipeChannel interface in string executable - should be a nsIFile? nsIPipeTransport interface don't mix terms... the "UI" thread IS the primordial thread. comment that beings with "The object that instantiates" is confusing. should derive from a nsITransport and a nsITransportRequest. nsIPCService: PRLogModuleInfo should only be defined if PR_LOGGING is true. How will nsIPCService::mService ever be free'ed? I think that you have a leak here. No need to double addref. The service manager will do this for you. Why do you have RegisterProc/UnregisterProc? In GetParent() and GetConsole() (and other places), writing this is more effienent: NS_IF_ADDREF(*_retval = mParent.get()); in ReadURI(), could you add a comment that SetOpenHasEventQueue() is a hack. in ReadURI(), pull the char buf[1024]; outside of the for() statement. more after we know where this patch should go (extension/xpcom) ccing scc@mozilla.org. he is the module owner of xpcom. We will want his blessing if this goes into xpcom....

Agree, obviously `exec' should live next to spawn. --pete

> > The more I think about this, the more I think that this is just messing around > with the fd's of an application that you are going to invoke. Why not just have > this functionality in xpcom? So, we have this interface nsIProcess. Why not > add the functionality so that you can obtain/set the fd's of a process? > Thoughts? > > xpcom would be a fine place for the IPC module to reside in. > > Some random comments: > > > nsIIPCService interface: > exec and execPipe seam like they belong in the partially complete nsIProcess Agreed. > interface. > execPipe shouldn't this use streams? execPipe is primarily meant to be a high-level scriptable interface, for use in Javascript components. Using streams would make it cumbersome to use in a script. If necessary, we can add a lower level interface using streams. > getRandomHex? Why is this even in this interface? > getEnv - can this in a more base interface? > They don't really belong to this interface, but they were needed for the scriptable protocol handling code. We will need to move them elsewhere, but they are kind of essential for protocol handling and security. Any suggestions on where to put them? Somewhere else in xpcom? > PipeChannel interface > in string executable - should be a nsIFile? > Perhaps, but see the following quote from nsIFile.idl: readonly attribute string path; * Accessor to the string path. These strings are * not guaranteed to be a usable path to pass to NSPR * or the C stdlib... Since we will need to pass the string path to NSPR, having a string argument makes this explicit. It is also easier to script. > nsIPipeTransport interface > don't mix terms... the "UI" thread IS the primordial thread. > comment that beings with "The object that instantiates" is confusing. comments need to be cleaned up. > should derive from a nsITransport and a nsITransportRequest. > I guess so. Perhaps we need a dummy AsyncWrite for the moment. > nsIPCService: > > PRLogModuleInfo should only be defined if PR_LOGGING is true. I was blindly following nsComponentManager.cpp; is it wrong there as well? > How will nsIPCService::mService ever be free'ed? I think that you have a > leak here. No need to double addref. The service manager will do this for you. I know about this leak, but I couldn't really figure out how to register a component as service. nsIPCService is at the moment just a singleton that pretends to be a service. This needs to be fixed. > Why do you have RegisterProc/UnregisterProc? Not really needed. Just used to print out diagnostic messages when registering the component. > In GetParent() and GetConsole() (and other places), writing this is more > effienent: NS_IF_ADDREF(*_retval = mParent.get()); will follow new style. > in ReadURI(), could you add a comment that SetOpenHasEventQueue() is a hack. will do. ReadURI too does not belong to the IPC module. But it is a very useful method for scripting. Can we move it somewhere else? > in ReadURI(), pull the char buf[1024]; outside of the for() statement. > will do.

getRandomHex - This should be in a new interface that security library could use as well - assuming that they use it for entropy. getEnv - How about a nsISystemProperties deriving from nsIProperties? Take a look at how the nsDirectoryService works. I am okay with string and not streams... We might need both at some point. blindly following nsComponentManager.cpp is probably not a good idea. Take a look at some of the necko protocols. Basically it is more of a nit than anything else.... nsIPCService::mService - leak. You don't have to do anything special.... If the client code goes through the service manager, it will keep track of that extra addref. Take a look at this macro: NS_GENERIC_FACTORY_CONSTRUCTOR_INIT Sounds really good. Can you attach a patch that can be applied to mozilla/xpcom?

dougt: I have now modified the Protozilla CVS tree to compile with the April 19 source tarball of Mozilla. This means that it will most likely compile with the tip. You can pull the latest Protozilla code by following instructions at http://protozilla.mozdev.org/source.html I have also changed the code to address pretty much all of your comments. The following issues remain to be addressed: 1. Where in mozilla/xpcom is the IPC code to be imported? In a new subdirectory mozilla/xpcom/ipc, perhaps? 2. I thought about moving the methods exec/execPipe from nsIIPCService to your nsIProcess implementation. Technically, this is not difficult to do, but I'm not sure this is the right thing to do. nsIProcess executes nsIFile objects, whereas the exec method executes a command line. The exec method also sends STDERR to a console owned by nsIIPCService, so that the user can look at error messages resulting from command execution. If you still want to move exec/execPipe to nsIProcess, that's OK with me. However, nsIIPCService will still need to exist to manage the console and provide the asynchronous execAsync method etc. 3. I have factored out the getEnv method out of nsIIPCService to a new nsISystemEnvironment, which is just a wrapper for PR_GetEnv. Where should this reside? xpcom/base? Or should I just leave it in xpcom/ipc for the moment? 4. I have also factored out the getRandomHex method to nsIRandomNoiseService, which is just a wrapper for PR_GetRandomNoise. Where should this reside? Once I have answers to the above questions, I will test the IPC code against the mozilla 0.9 release and then post it as a patch against mozilla/xpcom. Saravanan

wow you are fast! 1. Where in mozilla/xpcom is the IPC code to be imported? In a new subdirectory mozilla/xpcom/ipc, perhaps? Sounds good to me. 2. I thought about moving the methods exec/execPipe from nsIIPCService to your nsIProcess implementation.... okay. Have you thought about the macintosh platform where native paths are a bad thing? 3. nsISystemEnvironment xpcom/base is a good place for this. 4. I have also factored out the getRandomHex method to nsIRandomNoiseService, which is just a wrapper for PR_GetRandomNoise. Where should this reside? I think xpcom/base as well would be a good place

Who should act as xpcom/threads owner? I see *Process* stuff there, and now xpcom/ipc is being proposed. Perhaps xpcom/process is the best place for both of these things? Cc'ing roc,shaver, and waterson. Someone act ownerly, or xpcom will continue to grow like topsy! /be

DanM or I would be the person which "owns" the xpcom/threads directory. Or at least that is what blame would tell you. xpcom/threads would be a good place for this IPC code.

There's more to xpcom than threads, of course. If you're willing to entertain a new subdir (ipc), why not avoid splitting process (the p in ipc) stuff among threads and ipc, by making xpcom/process? I don't think xpcom/base should be a dumping ground either (I write this with a bad conscience, cuz I've dumped stuff there such as bloatblame.c that should move to tools/trace-malloc). I'm asking folks to think a bit about why we might want to separate process-level XPCOM sources from other sources (perhaps to make a single-process, boots-on-the-metal subset). There are aesthetic reasons along those lines, too. /be

mass move, v2. qa to me.

One comment on getEnv. This should be handled per process, IMHO. It's a process property, not a system property. Axel

->dougt

This bug also blocks bug 91702 (Enable simple MAPI support) in a way. We are at the design stage for 91702. If this IPC support is available, we'll make use of it. Now we're working on alternatives. If you want to know more details, check out the 1st attachment on bug 91702.

The above discussion is generally outside of my own knowledge of this area, but can someone confirm that it basically means: 1) add a general IPC mechanism to NSPR (eg. anon pipes) 2) Use this in a specific XPCOM extension Cheers, Chris

i don't think we need to implement ipc as part of nspr, i think we're talking about doing it at the networking xpcom level.

dougt: I was waiting for the netscape people to get NS6.1 out the door! I'm almost ready to submit a patch to XPCOM to add IPC. Can we target it for 0.9.4? tiantian: The patch attached to this bug implements simple IPC using anonymous pipes through NSPR, and works on Unix and Win32 systems. The attachment to bug 91702 suggests that a more sophisticated IPC mechanism may be needed for MAPI, although it would only need to work on Win32. Chris: This patch will make anon. pipe IPC mechanism accessible through XPCOM. (NSPR already supports anon. pipes.)

sure. Attach a working patch, and reassign it back to me for review and I will get get some reviewers for you.

> Chris: This patch will make anon. pipe IPC mechanism accessible through XPCOM. > (NSPR already supports anon. pipes.) AOK.

The IPC code is ready for incorporation into mozilla and has been tested in the trunk using the Aug 10 mozilla source tarball, on Linux and Win32 platforms. The code has been revised to address all the comments made thus far. Reviewers are needed! dougt: I am transferring the bug back to you for review as instructed earlier. In previous discussion of this bug, the idea was to incorporate the IPC code into the XPCOM module. However, the IPC code extends the necko interfaces nsITransport and nsIChannel. It doesn't seem that it would fit naturally into XPCOM. It seems logical to incoporate the IPC code into a new directory mozilla/netwerk/ipc (or something along those lines). If the necko module owner does not approve it, perhaps we could create mozilla/extensions/ipc. Anyway, this is open for discussion. I have created two attachments, assuming incorporation in mozilla/netwerk: 1. Source tarball (ipc-aug10.tar.gz) of the IPC code Ungzip/untar the file in mozilla/netwerk to create the IPC directory 2. Patch against Aug. 10 trunk to modify the necko build to include this directory, to be applied in mozilla/netwerk.

Lets move this to extensions/. nsIIPCService: newStringChannel() should be moved somewhere more common. Can you possibly use nsIStringStream? nsIPipeChannel: The comment "(NOTE: This is not a thread-safe interface)" is incorrect. It is possible that your impl is not threadsafe, but there is nothing inheirently thread-unsafe about this interface. nsIProcessInfo: I would like to see this interface merged with nsIProcess. nsIRandomNoiseService: Does this interface have to be exposed? nsIPipeConsole: Could you rename GetPipe to something like GetFileDesc? Do you want to use nsIProcess as a data structure to hold the executable name, arguments, and environment? Then use this in some of your other apis such as the init of nsIChannel? Do you have any test cases? More soon. Next to look at the impl. Brendan, if you have some time, please review this too.

re-bucket-ing milestone

dougt: I have added a source tarball to compile IPC as an extension (mozilla/extensions/ipc). I have not yet made any changes yet in response to your comments on the interfaces, but I will do so once you have looked at the implementation. There are test programs in ipc/tests to exercise the IPC capabilities. Response to interface review: nsIPCService: nsIStringStream doesn't seem to be scriptable. Hence it won't serve the purpose. newStringChannel could be moved elsewhere (necko?) nsIPipeChannel: True, it is only the implementation that isn't thread-safe. I will change the comment nsIProcessInfo: I don't think it should be merged with nsIProcess, although it could be moved to xpcom and/or renamed. This interface provides info about the current process, not about any newly created process (like nsIProcess) nsIRandomService: This is necessary for Protozilla and could be useful otherwise too. It too could be moved to XPCOM, if it isn't to stay in the extensions. nsIPipeConsole: I will rename GetPipe to GetFileDesc nsIProcess: I could add another method initWithProcess to nsIPipeChannel to initiate IPC using an nsIProcess. But you would need to add proprerties to nsIProcess so that I can access the command arguments.

The api that I think should move to nsIProcess is GetEnv. This interface, already has API related to the running of an exe, namely Kill(). Lets get rid of the RandomNoiseService and interface. It is only used from the nsIPCService so maybe a simplier implementation can live there. Maybe I am missing something?? nsIPCService.cpp: Since mConsole is a nsCOMPtr, you do not need to initialize it to nsnull. Same for mCookieStr (but I am a little less certain about this case). In the ~(), you do not have to explictly null mConsole. Should the Open() parameters on the nsIPipeConsole be turned into preferences - or at least #define's? In the Init(), you really don't need to have the local var pipeConsole. Just use mConsole. Your really not using mConsole anywhere to test to see if Init() failed. In GetConsole(), I do not think that you need to call mConsole.get(). Just use mConsole and let the the nsCOMPtr do its magic. You do this in other places too which need to be fixed. In ExecPipe, you init most of the out vars, but not all of them to null. You should also init env? When closing a nsIOutputStream, there is no need to explictly Flush as you do on line 269 and in other places. Nit: use a 'while (1)' instead of a 'for (;;)' This is done in a couple places. also note, if you want to optimize the emptying of the inputstream, you could call ReadSegments. nsIPCService.h I see RegisterProc an UnregisterProc defined, but I don;t see that they are implemented anywhere... Create too - in all of the headers? nsPipeChannel This class is not threadsafe. Not sure if you intend it to be. don't init nscomptr's to null. General Rule. If you do this: nsCOMPtr<nsIURL> url( do_QueryInterface( aURI, &rv ) ); to test success, all you must do is check url, not rv or both. When getting an out string from a API, it is better to use nsXPIDL(C)String. Change: char* contentType = nsnull; rv = MIMEService->GetTypeFromURI(url, &contentType); to nsXPIDLCString contentType; rv = MIMEService->GetTypeFromURI(url, getter_Copies(contentType)); In GetName, you should probably protect against a null mURI In OnDataAvailable, I think that this sum is wrong. aLength is always the relative length. mContentReceived += aLength - aSourceOffset; nsPipeConsole you do not have to have a local variable here, just use mPipeThread nsCOMPtr<nsIThread> pipeThread; rv = NS_NewThread(getter_AddRefs(pipeThread), this, 0, mThreadState); See if you can easily use a Lock instead of a Monitor? Monitors are much heavier than a lock, and I don't see you using the CondVar properties of the monitor.

dougt: I have attached a source tarball of the IPC extension, modified in response to your reviews. Most of your suggested modifications have been incorporated. Thanks for all the tips on improving my coding style. Should we get someone at mozilla.org to super-review the code so that we can have imported into extensions/ipc? Response to review: > The api that I think should move to nsIProcess is GetEnv. This interface, > already has API related to the running of an exe, namely Kill(). I'll remove GetEnv from the IPC extension as soon as you add it to nsIProcess > Should the Open() parameters on the nsIPipeConsole be turned into preferences - > or at least #define's? IPC does not have a UI at this time > In ExecPipe, you init most of the out vars, but not all of them to null. You > should also init env? env is actually an input variable! (an array of strings) > nsPipeChannel > This class is not threadsafe. Not sure if you intend it to be. Do nsIChannel implementations need to be thread-safe? If not, I'd leave it the way it is.

Brendan, can you review the current patch?

Does this work on Mac? Is it an IAC implementation there?

it does not work on the mac. Not yet anyways.

Fwiw, info on IPC under Mac OS X is at [http://gemma.apple.com/techpubs/macosx/Essentials/SystemOverview/InverEnvironissues/Interproces_mmunication.html], and general IAC info for Mac OS is at [http://gemma.apple.com/techpubs/mac/IAC/IAC-2.html].

This IPC implementation relies entirely upon NSPR to create processes and access anonymous pipes. To the extent that these NSPR functions work on Mac OS X, I would expect this implementation to also work. Since I don't have a Mac, I can't test this. (I don't about the build environment; maybe some configuration information needs to be added.)

not enough time. 0.9.5 -> 0.9.6

reassigning to brendan. Waiting on his review.

*** Bug 103297 has been marked as a duplicate of this bug. ***

I'm sorry I didn't review this sooner, but it falls roughly behind mozilla1.0 priorities -- not to say it shouldn't go in before 1.0, just that I can't justify spending time when higher priority problems are burning around me. I do not want to starve this patch till it's too late, either. I will do my best to finish the review for 0.9.7, preferably before three weeks have elapsed. /be

I am keeping the patch up to date with the trunk. If you need a compilable version any time, let me know. You can also access the latest version directly via CVS from http://protozilla.mozdev.org

Still no time to review this, but can I solicit a new tarball or should I pick up where I left off reviewing the last one attached here?

I realise that there is a lot to do for Mozilla, but a 10 month old bug that has a fix and only needs reviewing to be included in the tree is ridiculous. There are projects which currently can't be finalised until this happens, and projects which can't start until it does. I showed Protozilla to the director of CNI (Coalition for Networked Information, http://www.cni.org/) who said that this would be a God Send for many projects out there that he knew of, but that obviously it needed to be stabilised first. Please reassign this bug a higher priority.

Brendan: I have attached a recent source tarball of the IPC extension that works with Mozilla 0.9.6. There are no major changes from the old version, although there sre some bug fixes.

Brendan: Here's the updated tarball for the IPC extension

On target for 0.9.9. /be

what happened here?

This is not a mozilla1.0 priority, and I have not had time to review all the code. If you can find another super-reviewer who does have time, I'd wonder what that freedom is costing mozilla1.0. How important is it that this land for 1.0? We're pushing off all sorts of latecomers, including SVG. /be

in that case can you retarget this?

Done. No promises, sorry. /be

> How important is it that this land for 1.0? We're pushing off all sorts of > latecomers, including SVG. I also have been waiting for this for a while. Its understandable that SVG is behind schedule-- its a new technology and a lot of code. IPC, on the other hand, is considerably less code, and is omething we've had in Netscape 4 and IE for years. Don't know if this makes it any more important than anything else targeted for 1.0, but thought I would point it out.

Interesting- someone retargeted this back to 1.0. Does it look like someone will manage to get this reviewed/sr'd in time, or was that just a wishful thinker with lots of bugzilla priviliges? It seems that it would be a good idea to try- authors of apps which work with NS6 but not Moz would probably be turned off a bit if 1.0 didn't include IPC- but scheduling is another matter entirely.

Brendan bumped it; presumably because he got a nag mail around the time of 0.9.9. Removing target milestone. This is _so_ not getting into 1.0 :-) One of the patchers/owners - please retarget. Gerv

I'm not seeing it targetted for 1.1... could we please see some changes from someone who can do that? The patch is pretty much done (it seems).. r=, sr= ???

Suggesting Mozilla 1.2.

removed tiantian from cc (bad alias; sends mail to jhooker)

A fresh tarball, one that works with the frozen 1.1 trunk, would be good so I can review this in time for the 1.2alpha trunk opening. Thanks, /be

I have attached a recent tarball of IPC, which works with Mozilla 1.1b. After doing "make" in the "ipc" directory, you also need to do "make" in the build subdirectory to create DLL.

saravanan: I have dropped the last tarball into the extensions directory and, after replacing the NS_INIT_REFCNT macro, have it compiled. But when I try to run the pipetest app it fails with the following error:"pipetest: ERROR: creating PipeTransport [80040154]" This is in a trunk build on win2k, which really should be your target, as the next possible landing point is for 1.2. Also, I've pulled the tip of the protozilla/ipc source on my linux machine and have gotten that built, but am having trouble getting the test to run. It complains that it can't find my libxpcom.so, when it is very clearly lying right there. Perhaps an environment setting? Any clues? Lastly, we've moved from makefile.win to using the Makefile.in on windows and I can't run makemake because my environment doesn't have it (bash and sh, but no csh). If there is something you can do to alleiviate that problem, great! That way I could pull the latest code straight into my tree instead of having to get tarball drops. It's looking good, let's put this thing to bed and get it in the tree!

[RFE] is deprecated in favor of severity: enhancement. They have the same meaning.

On Linux, you will need to add the directory containing libxpcom.so to the environment variable LD_LIBRARY_PATH to be able to use xpcshell. I have been having problems with pipetest on Win32. You should still be able to test IPC using xpcshell and ipctest.js I'll try to conver the makemake script to Perl, to make it easier to use on Win32. (You can actually choose install tcsh when you install the cygwin package to use the current version of makemake.)

Fixing TM. /be

One must ask, what is the purpose of moving protozilla into the mozilla.org cvs tree? Protozilla is very successful existing as it is today.

Doug T: because some proposed functionality depends on it. Check the bugs blocked by this one.

I do not agree that protozilla is required to fix most of these depend bugs. Just for the record, I am not against moving protozilla into the folds of the mozilla CVS repository, nor I am for this action.

Why does this block bug 156493?

Whether the code to provide end-user ability to register new URI schema and handlers (bug 68406) is taken from protozilla or comes from some other source, I personally don't care. But IMO Netscape Navigator 3.0-equivalent functionality for Mozilla should not depend on the user having to hunt down external projects. Mozilla should not be less functional in any area than Netscape Navigator 3.0

dougt: because the ability to register new URI handlers is an extremely useful one, and it is very convenient to be able to depend on its presence. Gerv

I do not believe that most users will require or will use protozilla functionality. Protozilla is the 48th most active project on mozdev. If we are going to start moving functionality into mozilla cause it is useful, I would think that the more popular add-on's be included first. i.e. the top ten mozdev projects.

If I may add, it is also the most underrated feature. This feature is _extremely_ powerful, and is what makes people use browsers like konquerer over mozilla. The reason why it is not so popular is because it is a highly technical feature, however a _very_, _very_ important one.

dougt: you are assuming a false correlation between the number of downloads and "usefulness". Protozilla is a piece of infrastructure; it allows the writing and installing of new protocol handlers. Not many have been written, and I would venture to suggest that this is because the function isn't very high profile, and authors who know about it can't depend on its presence. Were it to be included, I suspect many more handlers for a variety of protocols would be written - and you could quite legitimately claim that _those_ should be mozdev projects. Adding the functions of Protozilla to Mozilla is consistent with a desire to make Mozilla expandable rather than bloated. I can't speak for the Phoenix developers, but I would have thought that this is the sort of thing they'd want, as well, for that reason. Gerv

Gerv, suggesting that mozilla requires new infrastructure to allow writing and installing of new protocol handlers is false. Mozilla has been extensible since the conception of Necko. Protozilla makes it easier for users to map a URI scheme to an external application. the decision to directly include protozilla with mozilla rests on brendan.

dougt: Sorry I wasn't more clear in my understanding or precise with my language. But my basic point is, I believe, still valid - including this allows other function to be excluded and provided by other programs. Gerv

such as?

A couple of examples of URL schemes I'd like to map to external applications: callto:// to invoke conferencing clients (see http://msdn.microsoft.com/library/en-us/netmeet/nm3_1l4o.asp) freenet:// to invoke a FreeNet client (anonymous peer-to-peer) irc:// to invoke an IRC client with more capabilities than Chatzilla More importantly (unofficially, but as someone who works for a company who makes conferencing products), I'd like to be able to tell users that registering callto:// for themselves is as simple as putting a few text strings in a configuration dialog, or even allowing a signed script to run. Gerv

As per Gerv, there are -many- URI schemes that would be made accessable by Protozilla which people -are- asking for. For example: isbn://XXXX / issn://XXXX z3950://host:port/database/query SOAP based protocols of all descriptions as URIs The reason that there doesn't seem to be much going on with the project is that it has been finished for a VERY long time. The first source tarball is for 0.8.1 and had already had a lot of development put into it by that stage, in April 2001! Please, there are many projects which -NEED- a standard protozilla. See my comment #52 ( http://bugzilla.mozilla.org/show_bug.cgi?id=68702#c52 ) which still stands.

Jumping a little late into this discussion to clarify things: This bug does not deal with incorporating protozilla in Mozilla. It deals with a completely generic IPC mechanism, which just happens to used by protozilla. (DougT: You must be aware of this, because you reviewed the code some eons ago!) For those familiar with Perl, the C++ code attached to this bug provides functionality very similar to the IPC::Open2 and IPC::Open3 functions permitting interprocess communication. This functionality will be accessible from privileged Javascript. The advantage of having this generic IPC mechanism in Mozilla is that the rest of Protozilla is written in pure Javascript/XUL, and it will be easy to maintain it as a cross-platform package. Currently, the IPC code needs to be compiled separately for each platform, gcc version etc., which is a real pain. I know that there is a supposed "IPC" module being added to the mozilla tree, to allow communication with a mozilla daemon from multiple mozilla client processes. However, that is not a generic IPC mechnism like the Perl IPC package, allowing arbitrary programs to be executed, and their STDOUT captured as a string.

All right, where should this land? If it's an extension, go ahead and land it as is under extensions/protozilla. If it is built-in and other non-extensions count on it, we need to talk more. /be

If this *is* a generic IPC solution, why isn't the IPC module being written in terms of it? If the IPC module you speak of is indeed *not* generic, why not? Why is Mozilla getting an IPC module that isn't general? If it's specific to a particular function, shouldn't it be labeled and positioned as such? (Perhaps it is, and I'm being misled by the vague references in this bug. Pointers would be appreciated.) And I reiterate, why does bug 156493 depend on this one? In other words, is the IPC mechanism provided by Protozilla *really* appropriate for solving that problem?

the protozilla IPC mechanism is generic in so far as it allows mozilla to spawn a child process and communicate with that child process using the child's stdin/stdout. however, it does not support communicating with an already running application. profile sharing requires communication with an already running process. a separate "IPC module" is being developed for that. as a result we have two different IPC problems with two separate solutions. the problems are orthogonal, and hence the solutions don't have that much in common.

I can't understand why those two issues have nothing to do with each other. Could you outline this point. Maybe it's not a bad idea to develop multiple IPC-Solution as a kind of evolution. Finally you should try to get best of all of them. And make subimplementations for special cases. BTW: What's the Bug-Nr of the IPC-Solution for Profile sharing?

see bug 178806. my point is simple. protozilla IPC enables you to do the following: exec a child process, connect to its stdin/stdout using anonymous pipes. for profile sharing, we need mozilla to connect to an already running application; call it the profile manager. to do this anonymous pipes cannot be used. the application needs to use some sort of "named pipe." actually, UNIX domain TCP sockets work well on UNIX/MacOSX and WM_COPYDATA messages (using FindWindow to locate the profile manager) work well on Win32 (Win9x does not support named pipes or UNIX domain sockets unfortunately). so, the entire interprocess communication mechanism is different. there is little commonality. now you could probably implement protozilla using the IPC mechanism that profile sharing requires, but that seems overkill for protozilla. it just needs the lightweight anonymous pipes solution.

I don't believe bug 156493 depends on this bug. I agree with Braden's comment 88, last paragraph. I'm in favor of protozilla landing as an optional .mozconfig feature for 1.5alpha, so people can experiment with it, tell us the footprint and performance (if any) costs on various platforms, and get it ready for prime time, if it's to be turned on by default. The mozilla/ipc container directory can host it. Calling the subdirectory there protozilla is a little longwinded, and it uses the trademark-challeneged -zilla suffix, which we're trying to avoid (we're allowed the old uses: Mozilla, bugzilla, chatzilla). How about a new name? Funny or descriptive, shorter is better. /be

http://bugzilla.mozilla.org/show_bug.cgi?id=68702#c92 I wish it to be optional, too. mozilla/IPC for BeOS if far from being implemented

svn, should we still do this? Darin landed mozilla/ipc/ipcd. Do you want to land mozilla/ipc/protozilla? What about the -zilla trademark badness? /be

Giving to svn now that there's a place to park protozilla in the tree. Issues to resolve: - any necko, mailnews, or other stuff that needs patching? - yet another -zilla name in the mozilla.org tree, in violation of our trademark deal with the Godzilla folks, seems like a bad idea, so: new name needed. /be

Maybe I can step in and help here, at least partially. svn is the original author, but since I have more or less taken over Enigmail development, I also adapted the ipc package to the Mozilla changes to ensure it continues to work for Enigmail (honestly, I only did it to keep enigmail running, not because I'm interested in maintaining ipc...). IPC doesn't require any changes to other existing code in Mozilla.

Patrick, do you want to take this bug? What should the name under mozilla/ipc/ be for this code? We don't want mozilla/ipc/ipc, and *zilla is out. The only other mozilla/ipc subdir, so far, is named ipcd, for the Mozilla IPC daemon. How would you name this code in a way that complements that "d is for daemon" name? /be

Concerning the name, "protozilla" is actually a project at mozdev.org that adds a frontend and some more stuff around this ipc module. This module in fact is "only" the kernel of protozilla that mainly takes care of piping data to/from other processes. How about "ipcpipe" or "ipcpiping" as new name?

Yeah, pipe is a good name or name-part. But first let's make sure there is an active owner ready to own mozilla/ipc/ipcpipe or whatever it should be called (mozilla/ipc/pipes?). I'd like to hear from svn about whether he wants to move to cvs.mozilla.org and be the owner. /be

IPC communication is a transport level issue. In the biggest picture it will have channels & protocols layered over the top one day, so naming that anticipates such a trend would be appreciated. IPC = a subclass of all transports.

Nigel: sorry, no: our directory structure does not impose one man's tyrannical hierarchy to make bogus extra levels. We decided not to make darin's ipcd module part of Necko, so it needs a different top-level than mozilla/netwerk. The easy to find and type result was mozilla/ipc. That exists, it's the right place to put protozilla, should everyone agree to make protozilla an owned, optional module in cvs.mozilla.org. There's no gain and only loss in putting protozilla under a layer of extra directories that no one will usefully populate. /be

Actually, I didn't specify single inheritence, mandate a deeper hierarchy, use the universal quantifier, or in general do anything. But I guess my remark can be read that way on a cloudy day. So, more gently, I merely say that transport-like uses of IPC are very likely in future, and therefore closing the door too tightly on such uses is worth avoiding. I do object to mozilla/ipcpipe over mozilla/ipc/pipe, on the basis that each IPC technology should be clearly identified. This is in case, one day, each form of IPC gets a URI scheme (shm:, doors:, msg:, pipe: etc), or some similarly obvious exposure (nsISharedMemoryTransport). If that breeds problems at the module level, then I vote for a simple single module: "mozilla/ipc". In that case, separate identification of IPC choices can be left off the module radar until later. No ownership problem there. But "ipcpipe" should not stand for all of IPC. A "pipe" is not required for an IPC implementation. Such an implementation can be done entirely with semaphores, for example.

>But "ipcpipe" should not stand for all of IPC. no objection here, and i don't think brendan was suggesting so either. re-read his comments... he outlined the fact that mozilla/ipc is where different forms of ipc stuff should live. we need a short moniker for the feature set that protozilla-ipc provides. in a nut-shell that feature set is the ability to open an external process and communicate with it via many different "pipe" like methods, including simple "give me the result of stdout as a string" all the way to "give me a nsIChannel corresponding to the output from the helper app."

So my question is: whether module name or moniker, how am I, with my "dumb learner" practice hat strapped on, to guess that "ipcpipe" means "mozilla specific stream-based transport for IPC, supporting a subset of IPC mechanisms" rather than "one-to-one wrapper for a UNIX pipe"? Can the higher level pipe concept be renamed so that it doesn't clash with the very-nearby UNIX pipe, and so that it doesn't imply suppport of all available IPC mechanisms? Could it be part-named a stream or something? ipcpipestream? pipestream?

protozilla is perhaps more akin to POSIX popen than the general concept of anonymous pipes. this code is basically just providing a bunch of different variants on popen.

It seems to me some of the naming contention arises from attempts to reconcile whether POSIX support is implemented or not. If it is, use POSIX language, if it's not, don't use it. I've been working up some thoughts on POSIX, since Ch 16 of my work showed it up as a gap in the offered XPCOM components. I've some brief and very draft notes, but if they're welcome here, I'll attach them for consideration. They're more general than this specific bug, but there's no "implement POSIX" bug yet. Want or not?

POSIX support in the underlying OS has nothing to do with this bug. Mozilla does not implement POSIX, and will not. Any veneer needed above the standard C library comes from NSPR, XPCOM, or a few other such low-level modules (intl comes to mind). Let's cut back on the bugspam until svn comments. /be

Brendan, Given the amount of time since svn last spoke on this bug (and also given your last comment). Is it safe to assume a 'basic' landing/re-review of this is simply waiting on someone to step up as module owner [of course giving Mozilla folk a chance to approve that person] and the other more-minor issues addressed. (Do note the code is MPL/GPL iirc)

The inclusion of Enigmail/OpenPGP [bug 332503] into SeaMonkey (and probably TB - who knows) has a working interprocess communication as a prerequisite. Patrick Brunschwig (as seen also her in this bug) has maintained the IPC module needed by Enigmail over the last years and has volunteered to own it if noone else can be found... Patrick, this still holds? Dougt, would/could you review here (when asked)?

Note that nsIProcessInfo / getEnv in its current form already exists in the tree - see nsIEnvironment. http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsIEnvironment.idl I also think that this functionality (reading/writing from/to process' stdout/in) belongs conceptually to nsIProcess. (Though I realize that it might be hard to implement in a sane way.) (For my future reference, comments 89 and 91 nicely summarize what exactly is being worked on in this bug.)

Here is the latest tarball. I'm ready to take the ownership for the moment, but I would be happy if someone else could take it over in the longer term.

darin, benjamin, any thoughts on landing IPC support as implemented here? see : news://news.mozilla.org:119/lp6dnSv55cdsswDZnZ2dnUVZ_sednZ2d@mozilla.org Doug

Here's a mail that Patrick Brunschwig did write as an API overview to ease review of that IPCpipe code so we can get it into the tree.

dougt, bsmedberg, et al: is there a reasonable chance of getting this reviewed and checked in Gecko 1.9? This bug is one of the most requested for Mozilla-based app developers.

I don't remember the code exactly, but why can't this just be an extension?

Not a blocker, but we'd take it.

Patrick: does this code still work on 1.9 trunk?

No, it won't, nsSpecialSystemDirectory.h and nsSpecialSystemDirectory.cpp are gone. See bug 351921. http://beaufour.dk/jst-review/ had a lot to say about the code in this tarball. I have a copy of this code locally in one of my Linux trees, if you want me to try updating it - but I'm going to need help.

Comment on attachment 217001 [details] IPC tarball working with mozilla-1-8 branch and trunk cancelling review request: no point reviewing something that can't compile.

I have attached the latest IPC tarball; it works with the current 1.9 trunk

This is just a first-draft attempt to make a review-friendly patch, with some cleanup attempts by me. The assumption is this will get dropped into extensions/ipc, and will have xpcshell-based unit tests under extensions/ipc/test/unit. Also, that users will build this with --enable-extensions=default,ipc. Observations from skimming over the code: --- * When I build with this patch included, the IPC service component isn't available. I suspect this is because a needed file (on Linux, it's called libipc.so) isn't getting compiled and dropped into dist/bin/components. (This is why I'm not asking for review right now. "I broke it!") * http://beaufour.dk/jst-review/ makes a lot of complaints - many of them invalid :) * The xpcshell test cases can use the new test harness code in greater detail (do_check_true, for example). * There's a lot of cases where you write: if (NS_FAILED(rv)) { return NS_ERROR_FAILURE; } I worry about that; you're taking a specific error message and replacing it with a generic one. * This code almost never use |NS_ENSURE_SUCCESS(rv, rv)|, substituting |if (NS_FAILED(rv)) return rv;|. Most of the time, this is fine, but at least for QI calls I think a bit of console noise, pointing out the line number, is good. * I also see this several times: + if (NS_FAILED(rv)) + return rv; + + return NS_OK; Here, you might as well just return rv (unless you anticipate other NS_SUCCESS codes and want to absorb them). * Windows newlines, long lines (especially with DEBUG_LOG and ERROR_LOG), and white-space at the end of lines... bad vibes. :) * We probably don't want these lines on every Makefile.in: +#!gmake +# * Many JavaDoc'd methods do not detail the arguments (@param). * Indentation style seems to favor 3 spaces per block. This is fine, but a little unusual. * I notice a few hard-coded contract ID's from other services; maybe get the appropriate .h files, now that this is presumably going into the tree? * Operators rarely have spaces around them: (a+b) instead of (a + b). * Where your classes implement a mInitialized property, it's probably worth throwing in some checks of that value in your public methods (NS_ERROR_NOT_INITIALIZED, NS_ERROR_ALREADY_INITIALIZED). * We have a NS_ENSURE_ARG_POINTER macro; it should probably be used, and should probably be the first lines of code in any C++ method with arguments - even before declaring |nsresult rv;| * I don't see many NS_ASSERTION, NS_PRECONDITION, NS_POSTCONDITION lines in this code. I do see the occasional PR_ASSERT (which given the context makes sense). * nsPipeChannel::SetOriginalURI has a mRestricted check. If the user's trying to set something and it's restricted, should it return an error code, or perhaps have a NS_WARNING call here? * This line is amusing: +// Mods for Mozilla version prior to 1.3b It just shows how old this code is, and how remiss we've all been in helping get this bug fixed. --- Patrick, I hope you and your team don't mind my weighing in here. I am somewhat motivated to try to get this working natively in Mozilla, since I'd like to do a few things with IPC.

Adding the following lines to extensions/ipc/src/Makefile.in made all the difference (and meant I could remove FORCE_STATIC_LIB=1). EXTRA_DSO_LDOPTS = \ $(XPCOM_GLUE_LDOPTS) \ $(NSPR_LIBS) \ $(MOZ_COMPONENT_LIBS) \ $(NULL) Pretty soon, I'm going to start tinkering with this code, trying to figure out how to use it. In particular, nowhere in this bug nor on protozilla's site do I see a guide stating how you can launch a process and interact with it. For example, running a program, feeding input to it and reading its output and stderr. Such a guide would be essential. Patrick, it's been three weeks since my comment, submitting the patch. How much are you and your team willing to work with me on this? I want to knock this bug out and see if we can get it in for Gecko 1.9, even if it's off by default for Firefox (which, given the size of this and the little amount of time we have left, it probably should be). I'd rather not go it alone... dougt, bsmedberg, brendan et al: When this patch is ready for reviews, who should review it? I'd prefer two separate reviews on this, again because of its sheer size.

Alex, you could have a look at the Enigmail extension source. Within the following file I found the usage of the ipc library: "\components\enigmail.js". Starting from line 1308 the service is initialized and used later. Just my 2 cents.

Alex, forget about "and your team" -- I'm the only person trying to maintain this (which is one of the reasons for the slow reaction). For exploring how things work, I'd recommend you read the API overview attached to this bug (attachment 246983 [details]), and have a look at tests/ipctest.js Concerning the review comments, I'd suggest to take this bilaterally until we think it's in a reasonable state.

I have beat myself senseless (read: three nights, over five hours each night) trying to make this work for me. Simply put, nsIPCService::ExecPipe() works, but nsIPCService::ExecAsync() doesn't. Nor can I figure out from the patch how to bypass the service and build a functional nsIIPCTransport object, and I can see no other way to pull it off. I'm attaching a sample Perl script with the ability to read from STDIN, and the ability to write to STDOUT and STDERR. If I can drive this from the IPC extension - that is, interact with it from a xpcshell test file - then I'm in business and I have something to test against while I clean up the code. As it stands now, I've got bupkus and frustration. Seriously, I am beginning to think we might be better off starting from scratch and designing something new, instead of trying to jam a gigantic blob of external code into the tree - code which has been poorly maintained at best. Do we really need all of it?

(In reply to comment #125) > Created an attachment (id=286121) [details] > A perl script for IPC to drive > > I have beat myself senseless (read: three nights, over five hours each night) > trying to make this work for me. Simply put, nsIPCService::ExecPipe() works, > but nsIPCService::ExecAsync() doesn't. Nor can I figure out from the patch how > to bypass the service and build a functional nsIIPCTransport object, and I can > see no other way to pull it off. ExecAsync is heavily used by Enigmail. Just because you don't know how to use it doesn't mean that it doesn't work, nor that it's badly maintained. I'm adding a small example to demonstrate its use (hope this helps): var gIpcRequest; function demoRequestObserver() {} demoRequestObserver.prototype = { QueryInterface: function (iid) { if (!iid.equals(Components.interfaces.nsIRequestObserver) && !iid.equals(Components.interfaces.nsISupports)) throw Components.results.NS_ERROR_NO_INTERFACE; return this; }, onStartRequest: function (channel, ctxt) { DEBUG_LOG("demoRequestObserver.onStartRequest\n"); }, onStopRequest: function (channel, ctxt, status) { if (gIpcRequest) { try { var pipeConsole = gIpcRequest.stderrConsole.QueryInterface(Components.interfaces.nsIPipeConsole); if (pipeConsole && gpgConsole.hasNewData()) { var text = gpgConsole.getData(); alert(text); } } catch (ex) {} } } } function execAsyncDemo (requestObserver) { command = "/usr/bin/gpg --search-keys 0x12345678"; var exitCodeObj = new Object(); var statusFlagsObj = new Object(); var statusMsgObj = new Object(); var cmdLineObj = new Object(); CONSOLE_LOG("enigmail> "+command.replace(/\\\\/g, "\\")+"\n"); var pipeConsole = Components.classes["@mozilla.org/process/pipe-console;1"].createInstance(Components.interfaces.nsIPipeConsole); // Create joinable console pipeConsole.open(20, 80, true); var ipcRequest = null; try { ipcRequest = gEnigmailSvc.ipcService.execAsync(command, false, "", "", 0, [], 0, pipeConsole, pipeConsole, requestObserver); } catch (ex) { dump("execAsync failed\n"); } return ipcRequest; } function main() { var requestObserver = new demoRequestObserver(); gIpcRequest = execAsyncDemo(requestObserver); if (gIpcRequest == null) { alert("Error"); } }

What's gpgConsole?

sorry, that should have been pipeConsole.

Note to self: Don't forget STDOUT is buffered, and needs flushing. :-)

Tested on Windows, Mac & Linux, 1.9 code base. Now I can finally begin cleaning up the code.

The patch for this needs to be updated due to bug 348748 which gets rid of the NS_REINTERPRET_CAST and NS_STATIC_CAST macros.

That's corrected a while ago in CVS, but I think it's pointless to post every small change here. If you want the latest snapshot, then get it from http://mozilla-enigmail.org/ipc/ipc-latest.tar.gz

I remembered (belatedly) that XULRunner on Mac requires --enable-libxul. I also found IPC won't link when --enable-libxul is enabled... apparently, MOZ_INTERNAL_API doesn't work with that setting.

(In reply to comment #133) > I remembered (belatedly) that XULRunner on Mac requires --enable-libxul. I > also found IPC won't link when --enable-libxul is enabled... apparently, > MOZ_INTERNAL_API doesn't work with that setting. > We just had a discussion on that very topic: http://groups.google.com/group/mozilla.dev.builds/browse_thread/thread/cb807e0f4d08f282 if you get building on a Mac I would appreciate it kindly if you could post the library somewhere. I don't currently have my hands on a Mac so I can't go around building it myself (the non-static library that is).

I have fixed all (currently known) problems with building IPC. The code now works with xulrunner as well as with Thunderbird/Suite builds on Gecko 1.8 and Gecko 1.9. There is now a "makemake" file which can be used to easily create all makefiles if IPC is unpacked in mozilla/extensions.

I have a couple of comments: First, it seems that nsIPCService still does not provide a way to run an external with a specified initial working/current directory. On Linux/"unix" platforms this can be worked around by using the shell to first "cd" to the correct directory and then run the desired program, but on Windows, such an approach doesn't work as running cmd.exe results in the command prompt window opening. Second: On "unix" platforms, the SHELL environment variable should be used as the shell; /bin/sh should only be used a default, perhaps, if the SHELL environment variable is not present.

I have cleaned up the API, mainly because I think the old API could lead to security issues: - instead of providing a single command line with all arguments there are now 3 parameters: executable, args[], argsLength - removed support for invoking shell commands, i.e. removed the "useShell" parameter and the method execSh. (You can still invoke shell commands from within the application see tests/ipc.js for an example) - renamed execXXX to runXXX E.g. this is the new IDL definition for run (formerly exec): string run(in nsIFile executable, [array, size_is(argCount)] in string args, in unsigned long argCount); Furthermore, I have added some more examples in tests/ipctest.js that demonstrate how to use the library. To reply to comment #136: 1: Nobody ever requested it. 2: see above -- it's now up to the caller to define the shell to use

The issue of whether the interface should use a single string command line or an array of individual command line arguments is a rather tricky one, I believe. On "unix" systems like Linux, command line arguments are passed individually in an array, while on MS Windows, as far as I know, due to the CreateProcess API, command line arguments are passed as a single string and it is up to the program itself to parse this string. There is the additional issue that on Linux, a command-line argument is an arbitrary sequence of bytes (except that it cannot contain the 0 byte), while on MS Windows, everything seems to be interpreted as "characters" and it is even possible to pass "wide" (presumably supposed to be UTF-16) command line and environment strings. I don't know exactly how a "wide" command line string would be received by a "non-wide" WinMain, and whether this is at all dependent on the system's locale settings. In any case, I imagine most people don't care too much about providing an API that properly handles the nuances of MS Window's "wide" support, but I think it would at least be useful to provide two APIs for starting processes: one that takes an array of command line arguments, and one that takes a single command line string. The version that is "non"-native to the way command line handling is actually done on the platform will be supported by heuristic algorithms for parsing a single command line or converting an array of command line arguments into a single string. JavaScript programs that detect the current platform and have code to handle both Windows and "unix" separately may then use the more "appropriate" API for the platform, while other programs may just pick one API to use (perhaps the API appropriate for the platform the developer himself uses). Note: The issue is that because any Windows program is free to parse its command line string in any arbitrary way, there can be no single standard way to convert from an array of arguments to a single string. On "unix", the situation is better: there already exist standard quoting/escaping schemes that are common to many shells that could be used by this IPC system to convert a single string to an array of command line arguments, e.g. by letting \ escape spaces and quotes, and passing through directly all characters (even odd ones line CR and LF) other than \, space, and quote. Still, although this is an argument for using just a single command line string interface, such an interface is certainly much more annoying for "unix" users, and so I would not recommend it being the only interface. As far as being able to specify an initial working directory, I'm requesting that now, if that means anything. It isn't very much code at all, but it does mean an API change, so it is best to get that out of the way, I think. On Windows, it is simply an argument to CreateProcess, while on "unix", it is simply a matter of calling chdir(2) in the child process (created by fork) before calling execve. I'd suggest as an interface that an additional (optionally null) nsIFile argument be added. If it is null, then a corresponding null value is passed to CreateProcess and no chdir is done in the child process. This feature is very helpful for allowing users to enter shell commands for various purposes from within Mozilla-based programs.

I'm sorry, but I don't quite agree with your comment regarding whether the command line arguments should be a single string or an array of strings. First of all, the old exec/execPipe commands took the one string and split it up into an array of strings that were then used for calling the process. So from that point of view you don't loose anything with the new API. Second, if you really want to leave parsing the command line to the called process then feel free to just put all arguments into one string and pass that as an array of length 1 to the API. Thus, you even gain something with the new API. Third, and foremost, I don't think it's a good idea to provide two different ways to do the same thing, if that's not needed. And finally, it's a fact that the API of nsIProcess uses the structure that I have now implemented, and I see no reason why not to be consistent with other Mozilla API's as long as there is no technical reason. The chdir() stuff is certainly an easy thing; I'll look into adding that.

My point regarding the command line argument passing style is that there is a fundamental incompatibility between the Windows and "unix" command line argument passing conventions. This incompatibility is evident in the different interfaces provided by execve and CreateProcess. On Windows, programs receive a single string as a command line, and can parse it however they please, irrespective of any particular quoting or escaping conventions. In particular, the amount of white space between "words" may or may not be meaningful. Thus, without knowing how the program parses its command line, there is no single "right" 'way to convert an array of strings into a single command line string for the program. It is true that the msvcrt (C runtime library) includes a command line parser to convert the command line string to pass to main, and that would certainly be one semi-"standard" convention to follow, but programs that don't use msvcrt may not follow that parsing convention. One particularly relevant example of this issue is with cmd.exe. After the /C option, the remainder of the string serves as a command line string. The first "word" is parsed according to some quoting convention to determine the program to run, but otherwise the string is left unparsed and passed directly to the program being called. Note how this differs from the convention for sh -c, where the shell command line is passed as a single argument after -c, e.g. sh -c 'echo hello'. Thus, with an API that only allows passing an array of arguments that will then be converted by this IPC package into a command line string according to one particular quoting convention, in order to allow the user to type in a Windows shell command line to run, it is necessary to assume that the program the user is calling uses the same quoting convention as the IPC package, and then the string the user typed must be parsed into an array of strings, which is concatenated with an array ["cmd.exe", "/c"], and then this is passed to the IPC package which essentially reverses the parsing just done in JavaScript. I agree that in most cases, programs do follow the "standard" quoting convention and there is no problem, so perhaps it is a reasonable compromise to make for a simpler library interface, but for full functionality, it is necessary to have both a single command string version as well as an array of arguments version.

One type I noticed in my last reply: in the last paragraph, first sentence, the phrase "quoting convention and there is no problem" should be "quoting convention and then there is no problem".

Let's assume you want the callee to evaluate the arguments in a non-standard way. Why can't you then just put together all arguments into a string and submit that as array of length 1 to runXXX()? It's the same result as providing a special method for doing so. Eg. runPipe(myTool, [ "-arg1 'arg2 arg3' \"-arg4 -arg5\" -arg6" ] , 1, ...)

On Windows, I believe your library would modify the string, in particular surround it in quotes, which is consistent with the "standard" convention for passing multiple arguments to Windows programs.

Even though a command line on windows is theoretically a single string, standard practice (and the CRT) divide the arguments up according to standard quoting rules. We should provide a normal cross-platform API to pass arguments as unicode strings... which would convert to the native codepage on linux, which is normally UTF8 nowadays, and use the wide-character version of Windows APIs such as CreateProcessW.

I really got tired of trying to manually convert these tarballs into patches that we could commit to mozilla.org CVS and build with. So I wrote this script to do the work for me. This way, as long as Patrick keeps giving us tarballs, we can generate Mozilla-CVS patches from them in about two minutes. Use: cd mozilla perl ipc-tarball.pl --tarball /path/to/tarball.gz This will generate a patch file in mozilla/../ipc-patch.diff, and format the Makefile.in and CVS Entries, Repository, Root files for mozilla.org. No objdir needed.

Hm, this tarball almost compiled for me on Windows. nsPipeTransport.cpp at 741, 742, VC8 complained about NS_ADDREF(pipe), NS_RELEASE(pipe): cannot access private member declared in class 'nsDerivedSafe<T> with [ T=nsIPipe ] Commenting the two lines out allows it to compile.

Comment 146 was talking about Fx3b3 code. On Fx trunk, Windows, I hit this compile error, VC8 environment: c:/trunk/mozilla/extensions/ipc/src/nsPipeTransport.cpp(694) : error C2491: 'NS_NewPipe' : definition of dllimport function not allowed c:/trunk/mozilla/extensions/ipc/src/nsPipeTransport.cpp(724) : error C2491: 'NS_NewPipe2' : definition of dllimport function not allowed

Changing "NS_COM nsresult" to "nsresult" fixes this, but leaves a couple warnings behind.

I don't see any reason this would go into 1.9.0.x.

For those interested: I have prepared binary versions for Linux and Windows that can be used with XULRunner 1.9.0.x, available at http://mozilla-enigmail.org/ipc/ Simply unpack the tar/zip file in the xulrunner installation tree.

Thanks Patrick. But I've still one question, what reason stops us to get it into trunk? Could you give a short overview of things todo?

Another year gone by of a bug opened in 2001. :) FireFTP and FireGPG still rely on this library (but conflict with each other since they're using different versions). We still hope that this could be landed finally. The comments from last year seem to support that this could have landed but somewhere the ball got dropped...

Wait, this still isn't in? What's blocking it?

Electrolysis is happening. bsmedberg and cjones may have thoughts on what to do with this bug. /be

The status is that the current patch is unacceptable because it doesn't work well with the existing stream APIs or nsIProcess.

Benjamin: Are you and cjones working on that, or is Patrick?

Benjamin, I agree that the API could profit from adaptation to nsIProcess. But why doesn't the library work well with the existing stream APIs?

(In reply to comment #155) > The status is that the current patch is unacceptable because it doesn't work > well with the existing stream APIs or nsIProcess. Do you have suggestions for the patch author to make it work well with the existing APIs?

(In reply to comment #154) > Electrolysis is happening. bsmedberg and cjones may have thoughts on what to do > with this bug. Benjamin or Chris, can you both please give some suggestions which steps would have to be done to make those requested changes from comment 155? Thanks.

anyone working on updating this to current mozilla-central?

No. In terms of the API, what I want is a description of how you *use* the APIs from JS. My preference is for an API that mimics the best of the python subprocess module, and the API would be something like: Components.utils.import("resource://gre/modules/subprocess.jsm"); p = subprocess.call({ command: '/bin/foo', arguments: ['-v', 'foo'], stdin: subprocess.WritablePipe(), stdout: subprocess.ReadablePipe(function(data) { dump("Got data on stdout: " + data); }), stderr: '/dev/null', onFinished: function() { dump("Process finished with result code: " + p.result); } }); Neither cjones nor I has any intention of working on this bug.

such a subprocess interface would still require an xpcom component that can open processes cross platform and read/write from them. i.e. current nsIProcess still pops up a black window on windows and is thus not usable to call external programs.

It might need an XPCOM interface (probably better than trying to use ctypes for this). But the question of nsIProcess opening a console window is an entirely different question, and one that can be solved by you recompiling your program as a windows program, instead of a console program.

The runAsync method in nsIIPCRequest provides an API similar to the one proposed in comment 161. I'll try to write a JS module that wraps it.

I have created a new subprocess.jsm module that provides an API very similar to what Benjamin proposed. The API makes use of some of the existing ipc-pipe components (but not all). Feedback appreciated.

I don't understand subprocess.WritablePipe or subprocess.WriteToPipe: how do you know *which* process you're writing to with WriteToPipe? What does "The function is automatically called when the process is created and ready to receive input." actually mean? Why can't we just create a subprocess.WritablePipe() and then .write() to it? What's the difference between ReadablePipe and ErrorPipe? They are the same API, right? Is there a way to combine stdout/stderr into a single stream? Is the data ever cached and chunked, or is it always delivered as soon as we receive it? I prefer "environment" to "envVars", but we should make it clear that it is *additive* over the current environment, not a completely new environment block.

(In reply to comment #166) > I don't understand subprocess.WritablePipe or subprocess.WriteToPipe: how do > you know *which* process you're writing to with WriteToPipe? What does "The > function is automatically called when the process is created and ready to > receive input." actually mean? Why can't we just create a > subprocess.WritablePipe() and then .write() to it? My thinking was wrong. I changed it, writing is done via this.write(). > What's the difference between ReadablePipe and ErrorPipe? They are the same > API, right? Is there a way to combine stdout/stderr into a single stream? Is > the data ever cached and chunked, or is it always delivered as soon as we > receive it? It's the same API; I removed ErrorPipe. There is an option to merge stderr into stdout; I added it to the API. In case stdout is not defined, all output data is cached and can be retrieved in the onFinished method. I changed the API for this to wrap onFinished into an object as well. If stdout is defined, the data is cached and delivered in chunks of up to 2048 bytes or when the subprocess signals flushing. > I prefer "environment" to "envVars", but we should make it clear that it is > *additive* over the current environment, not a completely new environment > block. I changed the name. For the moment it's a completely new block, but I could easily change it to always pass the current environment and use "environment" only for additional env. vars.

why is there a need for subprocess.ReadablePipe and subprocess.Terminate to be exposed to people using call? Would it not be better to just accept callback functions for stdout, stderr and onFinished and wrap those functions into the given objects inside the call function? so one would just call it like this: var stdin = subprocess.WritablePipe(); var p = subprocess.call({ command: '/bin/foo', arguments: ['-v', 'foo'], environment: [ "XYZ=abc", "MYVAR=def" ], stdin: stdin, stdout: function(data) { dump("Got data on stdout: "+data+"\n"); }, stderr: function(data) { dump("Got data on stderr: "+data+"\n"); }, onFinished: function() { dump("Process finished with result code: "+this.exitCode+"\n"); }, });

I know that stdout, stderr and onFinished are for the moment just callbacks. But I could imagine that we want to do something more with them (especially stdout and stderr) in the future. E.g. we could provide methods to initialize and finalize the objects, or provide ways to allow for own implementations of stream listeners. Having them already now as objects will make it easier to adapt changes in the future. For onFinished, I simply thought I'd use the same approach as for the other "callbacks" to have some consistency. Alternatively, exitCode and stdoutData would need to be provided via the object created in call().

your current patch does not provide a way to kill the process. something like: var p = subprocess.call(...) p.kill() should work.

There are at least two ways to terminate a subprocess: a) close stdin/stdout/stderr streams b) kill -SIGNAL <pid> Option a) does not directly kill the subprocess but since it does not receive any new data / cannot write its output anywhere, the subprocess will terminate. The streams can be closed at any time. Option b) is usually not really desired with pipes, unless the subprocess does is somewhat buggy. Which option do you mean with "kill"?

#2, which we already have with nsIProcess.kill

I was thinking a bit about the killing. The current implementation of subprocess.call() will only return when the subprocess has terminated. This has two implications: - p is only defined after call() returns - the example in comment #170 can only be useful either in a separate thread, or in the stdout(...) callback. I propose to change the API to something like this: var p = subprocess.call(...) p.waitFor() p.kill() Alternatively this.kill() could be made available in the stdin/stdout methods.

I have prepared a new version of subprocess.jsm and extracted the minimum set of APIs/modules required from IPC-pipe to use it (i.e. nsIPipeTransport and nsIIPCBuffer). Changes to the API of subprocess: - subprocess.call(...) returns now after the subprocess is started. - p.waitFor() can be used to wait for the process to terminate. - p.kill() can be used to kill the process. See modules/subprocess.jsm for complete documentation and source code. I also improved nsIPipeTransport, which now implements nsIProcess (with the exception run(w) and run(w)async which return NS_ERROR_NOT_IMPLEMENTED).

what about calling waitFor just wait, would be more inline with python api. right now onFinished only gets called if one calls waitFor() for async calls this should not be needed.

(In reply to comment #175) > what about calling waitFor just wait, would be more inline with python api. fine with me > right now onFinished only gets called if one calls waitFor() > for async calls this should not be needed. Not exactly: onFinished gets called either if wait() is called of if stdout is defined. I have attached a new version of subprocess.jsm with: - waitFor() renamed to wait() - onFinished() gets called in all situations, independently if wait() is used or not.

trying to build ipc-pipe with mozilla-central on win32 failed here with this error: IPCProcess.cpp c:/mozilla-build/python25/python2.5.exe -O c:/Users/mozilla/src/mozilla-central/build/cl.py cl -FoIPCProcess.obj -c -D_HAS_EXCEPTIONS=0 -I../../../dist/stl_wrap pers -DMOZILLA_MAJOR_VERSION=2 -DMOZILLA_MINOR_VERSION=0 -DOSTYPE=\"WINNT6.1\" -DOSARCH=WINNT -I../../../../extensions/ipc-pipe/src/../build -I../../../.. /extensions/ipc-pipe/src -I. -I../../../dist/include -I../../../dist/include /nsprpub -Ic:/Users/mozilla/src/mozilla-central/ff-opt/dist/include/nspr -Ic:/U sers/mozilla/src/mozilla-central/ff-opt/dist/include/nss -GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -O1 -MD -FI ../../../dist/include/mozilla-config.h -DMOZILLA_CLIENT /c/Users/mozilla/src/mozilla-central/extensions/ipc-pipe/src/IPCProcess.cpp IPCProcess.cpp c:/Users/mozilla/src/mozilla-central/extensions/ipc-pipe/src/IPCProcess.cpp(453) : error C2653: 'nsMemory' : is not a class or namespace name c:/Users/mozilla/src/mozilla-central/extensions/ipc-pipe/src/IPCProcess.cpp(453) : error C3861: 'Alloc': identifier not found c:/Users/mozilla/src/mozilla-central/extensions/ipc-pipe/src/IPCProcess.cpp(546) : error C2653: 'nsMemory' : is not a class or namespace name c:/Users/mozilla/src/mozilla-central/extensions/ipc-pipe/src/IPCProcess.cpp(546) : error C3861: 'Free': identifier not found make[2]: *** [IPCProcess.obj] Error 2 make[2]: Leaving directory `/c/Users/mozilla/src/mozilla-central/ff-opt/extensions/ipc-pipe/src'

Sorry, I hardly build on Windows, so I didn't spot this one -- and one more. The attached tarball should now build on Windows, at least it does on mine.

This looks great! I haven't tried it, as I don't understand how to integrate it into the build system (yet). I might try protozilla as it seems close to what I need anyway. I want to cast a vote for exposing the cwd in the javascript object, though. I think it would be appropriate for it to be an optional 'cwd' property, alongside 'environment'; if present and non-empty, the cwd would be set for the child process. This is important since, for example, the CGI spec insists the current directory be set to the directory of the script when starting an interpreter. Suggested patch to subprocess.jsm:221...: if (typeof (cmdObj.command) == "string") { var localfile= Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile); localfile.initWithPath(cmdObj.command); cmdObj._commandFile = localfile.QueryInterface(Ci.nsIFile); } else { cmdObj._commandFile = cmdObj.command; } if (typeof (cmdObj.arguments) != "object") cmdObj.arguments = []; if (typeof (cmdObj.environment) != "object") cmdObj.environment = []; + if (typeof (cmdObj.cwd) == "string") { + var localfile= Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile); + localfile.initWithPath(cmdObj.cwd); + cmdObj._cwd = localfile.QueryInterface(Ci.nsIFile); + else { + cmdObj._cwd = null; + } this._pipeTransport = Cc[NS_PIPETRANSPORT_CONTRACTID].createInstance(Ci.nsIPipeTransport); - this._pipeTransport.initWithWorkDir(cmdObj._commandFile, null, - Ci.nsIPipeTransport.INHERIT_PROC_ATTRIBS); + this._pipeTransport.initWithWorkDir(cmdObj._commandFile, cmdObj._cwd, + Ci.nsIPipeTransport.INHERIT_PROC_ATTRIBS);

OK. Man, am I feeling proud of myself! I've managed to get the component built and cobbled together an extension that uses it in Firefox. Or, at least, it does a whole bunch of crashing which I think is to do with the component! I get a couple of these: XPConnect WrappedNative is being accessed on multiple threads but the underlying native xpcom object does not have a nsIClassInfo with the 'THREADSAFE' flag set wrapper: [xpconnect wrapped (nsISupports, nsIPipeTransport, nsIRequest) @ 0x11d75f1a0 (native @ 0x11d75f030)] JS call stack... JavaScript stack is empty Then one of these: Main Thread Only wrapper accessed on another thread wrapper: [object InnerChromeWindow @ 0x11ae27aa0 (native @ 0x11ae244b8)] JS call stack... 0 anonymous(data = "Writing example data ") ["chrome://ipcpipe/content/browser.js":14] this = [object Object] this.onDataAvailable = [function] 1 anonymous(count = 21, offset = 0, aInputStream = [xpconnect wrapped (nsISupports, nsIInputStream, nsIAsyncInputStream, nsISeekableStream, nsISearchableInputStream) @ 0x11d762c30 (native @ 0x11d7605e8)], aContext = null, aRequest = [xpconnect wrapped (nsISupports, nsIPipeTransport, nsIRequest) @ 0x11d75f1a0 (native @ 0x11d75f030)]) ["resource://ipcpipe/subprocess.jsm":195] av = 21 this = [object Object] this._inputStream = [xpconnect wrapped nsIBinaryInputStream @ 0x11d762fe0 (native @ 0x11d762f90)] this._observer = [object Object] this._cmdObj = [object Object] Then: Assertion failure: CURRENT_THREAD_IS_ME(cx->thread), at /Volumes/LeftVentricle/src/comm-central/mozilla/js/src/jsapi.cpp:792 And after a pause of a minute or so during which FF is unresponsive, a segfault. I'm on Mac OS X 10.6.4 running a debug version built from a checkout of the 4.0b5 code, the tarball from above, with my modification as detailed in my last post (but including the missing brace before the else!). I'm happy to provide as many more details as you like, though may need a bit of guidance where to find Mozilla-specific things, as I've only been hacking on Mozilla stuff for about 2 days. Also very happy to provide my cobbled-together extension. And happy to continue to help testing. I would LOVE to see this land in in the trunk at least in the next few months so that it is likely to get into a release by the end of next year if possible, as it would definitely simplify some of my work. (Shall we plan a 10th birthday celebration for the bug in 5 months' time?!)

I guess the culprit is nsIBinaryInputStream which (like most standard streams) does not seem to be thread-safe. I'm not sure what you did, but I assume the problem is that you are trying to read data from a different thread than you started it from, which I guess would fail with nsIBinaryInputStream. If you send me your extension, I can check what you did and hopefully see what's wrong. Your idea for "cwd" is nice, but I'd call the additional option "workdir".

(In reply to comment #181) > I guess the culprit is nsIBinaryInputStream which (like most standard streams) > does not seem to be thread-safe. nsIBinaryInputStream is likely not what you want to use. On trunk, we recently added a method to NetUtil to help with this exact task: https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm#readInputStreamToString%28%29

Ah, cool thanks! I just noticed that you fixed the NULL character issue which was preventing me from using nsIScriptableInputStream :-)

While Ben and I were discussing why he had crashes with ipc-pipe, he pointed out correctly that the callbacks in ReadablePipe and Terminate (ie. stdout, stderr, onFinshed) are performed on a thread controlled by nsIPipeTransport, and not on the main thread. This was actually the cause for his crashes. I think this needs somehow be solved: 1) add documentation for it (mention that DOM events etc. needs to go to the main thread) 2) always dispatch stdout, stderr and onFinished to the main thread 3) allow the user to choose, by providing additionally something like "subprocess.MainThreadReadablePipe" Preferences?

#2, the API should be single-threaded (main thread only)

1) passing unicode arguments on linux seams to cause problems now. 2) trying to build on windows i now get this: Microsoft (R) Windows (R) Resource Compiler Version 6.1.7600.16385 Copyright (C) Microsoft Corporation. All rights reserved. link -NOLOGO -DLL -OUT:ipc.dll -PDB:ipc.pdb -SUBSYSTEM:WINDOWS nsIPCModule.obj ./module.res -NXCOMPAT -DYNAMICBASE -SAFESEH -IMPLIB:fak e.lib @../src/ipc_s.lib.fake c:/Users/mozilla/src/mozilla-central/ff-opt/dist /lib/xpcomglue_s.lib c:/Users/mozilla/src/mozilla-central/ff-opt/dist/lib/xpcom. lib c:/Users/mozilla/src/mozilla-central/ff-opt/dist/lib/mozalloc.lib c:/Users/m ozilla/src/mozilla-central/ff-opt/dist/lib/xpcom.lib c:/Users/mozilla/src/mozill a-central/ff-opt/dist/lib/xul.lib c:/Users/mozilla/src/mozilla-central/ff-opt/di st/lib/mozalloc.lib c:/Users/mozilla/src/mozilla-central/ff-opt/dist/lib/nspr4.l ib c:/Users/mozilla/src/mozilla-central/ff-opt/dist/lib/plc4.lib c:/Users/mozill a/src/mozilla-central/ff-opt/dist/lib/plds4.lib kernel32.lib user32.lib gdi3 2.lib winmm.lib wsock32.lib advapi32.lib MSVCRT.lib(MSVCR90.dll) : error LNK2005: _strpbrk already defined in LIBCMT.lib( strpbrk.obj) MSVCRT.lib(MSVCR90.dll) : error LNK2005: ___iob_func already defined in LIBCMT.l ib(_file.obj) MSVCRT.lib(MSVCR90.dll) : error LNK2005: __fdopen already defined in LIBCMT.lib( fdopen.obj) MSVCRT.lib(MSVCR90.dll) : error LNK2005: __open_osfhandle already defined in LIB CMT.lib(osfinfo.obj) Creating library fake.lib and object fake.exp LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; us e /NODEFAULTLIB:library ipc.dll : fatal error LNK1169: one or more multiply defined symbols fou nd make[2]: *** [ipc.dll] Error 145

1) I don't understand what you mean, do you mean to use paths containing unicode characters? If so, you better use nsIFile. 2) Fortunately your build didn't work, otherwise I'm sure you would have reported a crash bug :-/. I modified the makefile, but it seems I'm using a different version of Visual Studio, so I'm not 100% sure if my fix will work for you -- please try it. Attached is a new version of the tarball. I have improved the API and the implementation as follows: - dispatch callback events to main thread - added "this.close()" method to stdin to allow for closing output to stdin - fixed a crash bug on Windows when starting a new subprocess - improved documentation -- thanks to Ben Schmidt for his feedback!

1) something like this fails if i select a file with non ascii characters, dump still looks ok, an fprintf in some/command looks ok but fopen fails saying it can not find the file. this could also be a problem elsewhere but if i remember right this used to work on linux and os x before. var fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker); fp.init(this._window, "Select file", Ci.nsIFilePicker.modeOpen); fp.appendFilters(Ci.nsIFilePicker.filterAll); var rv = fp.show(); if (rv == Ci.nsIFilePicker.returnOK || rv == Ci.nsIFilePicker.returnReplace) { var command = 'some/command'; var arguments = [fp.file.path]; dump('\nsubprocess:\n'+command+' \\\n'); for(var i=0;i<arguments.length;i++) dump('\t"' + arguments[i] + '" \\\n'); var p = subprocess.call({ command: command, arguments: arguments, }); p.wait(); } 2) builds and works now.

Unicode was not supported until now in nsPipeTransport. I attached a new version which adds Unicode support for arguments and environment variables; the new version also fixes a small memory leak in subprocess.jsm. My Windows build machine broke; I hope the tarball also compiles on Windows.

with v4 unicode works for me, thanks. also compiled on windows. did not test unicode support yet. another issue though, i sometimes get this: Error: too much recursion Source File: subprocess.jsm Line: 310 if (typeof(this._pipeObj._cmdObj.stderr) == "object" && (! this._pipeObj._cmdObj.mergeStderr)) {

I've seen that a couple of times, too, but for me it's Error: too much recursion Source File: resource://ipcpipe/subprocess.jsm Line: 251 which is the line onStartRequest: function(aRequest, aContext) { in StdoutStreamListener.prototype I can't see how deep recursion could happen--or even shallow recursion--from the code, as everything seems in order. My best guess is that it's some kind of multi-thread race condition that's messing with the js interpreter, and actually nothing to do with ipc-pipe per se. It is very rare, and no matter how much I try, I can't make it happen on demand. When it did happen, FF hung as it was shutting down. Anyone got a way to make this happen reliably? That would aid immeasurably to tracking it down, and either fixing it, or filing another bug.

I tried to reproduce this but without any success. I cannot see how you would get a recursion error as the code does not have any recursive call. Could you give me some hints of what you are doing?

Recursion errors can show up in places that are not obviously recurring, when the event loop is being spun (i.e thread.processNextEvent()) or other multithreading cases. If you set a cpp breakpoint at http://mxr.mozilla.org/comm-central/source/mozilla/js/src/jscntxt.cpp#1423 and get an insanely large stack (my last was >2000 frames) that often has processNextEvent in it, then you're getting exactly this.

That sounds like a good reason. I'll try to prepare a patch that waits for the main thread to process the event before onDataAvailable() et. al. return.

I think this was easier to fix than I hoped: the events are now dispatched synchronously to the main thread. Attached is a new tarball; the only changed file is subprocess.jsm.

just got around testing non ascii arguments on windows and that does not work yet.

Patrick, do you see a chance to attach a patch instead of a tarball in the future? Beside applying it to the tree, it will be also possible to check the code without downloading the source.

For now here the corresponding patch to the tarball v5.

Patrick, something seems to be wrong at least on OS X where I'm initially want to use this module. The call works fine as long nothing gets written to stdout or stderr by the process executed. Whenever the script just echo's something, Minefield cannot be closed anymore, even I call this function from the main thread and "ended" appears in the Error Console: this._process = subprocess.call({ command: "/bin/bash", arguments: [script.path], onFinished: subprocess.Terminate(function() { Components.utils.reportError("Exit code: " + this.exitCode + "\n"); }) }); this._process.wait(); Components.utils.reportError("ended"); Also the specified onFinished function never gets called, means I never see this output in the Error Console.

(In reply to comment #199) > Patrick, something seems to be wrong at least on OS X where I'm initially want > to use this module. The call works fine as long nothing gets written to stdout > or stderr by the process executed. Whenever the script just echo's something, > Minefield cannot be closed anymore, even I call this function from the main > thread and "ended" appears in the Error Console: So the callbacks are broken because of the landing of bug 608142 (Disallow sending JS objects to a different thread). Same applies to the hang on exit. Andreas, does it mean the patch has to be updated to use chrome workers for sending callback function references to background threads?

(In reply to comment #200) > Andreas, does it mean the patch has to be updated to use chrome workers for > sending callback function references to background threads? I don't think chrome workers have the needed APIs to make this work.

Yes, you cannot send JS closures (or any objects or strings) between threads. This has never worked reliably, and is now asserted not to happen between main thread and non-main thread threads (its unsafe between non-main threads also but thats harder to catch so we don't stop it right now). Please use chrome workers. And if Shawn specifies what APIs are needed, we can add those.

(In reply to comment #202) > but thats harder to catch so we don't stop it right now). Please use chrome > workers. And if Shawn specifies what APIs are needed, we can add those. Ideally this should have been done the other way around to not break existing extensions. :( Shawn, please file a bug so we can make sure to get this implemented as soon as possible. thanks.

(In reply to comment #203) > Ideally this should have been done the other way around to not break existing > extensions. :( Shawn, please file a bug so we can make sure to get this > implemented as soon as possible. thanks. It's not clear to me exactly what is running on what thread in the code, but it looks like it's passing streams around. Not really sure what kind of API we could expose there on chrome workers that would make sense (short of most of our networking API). Comments from the patch author would be more useful.

Please stop peddling ChromeWorker snake oil. ChromeWorkers don't equate to all uses of XPCOM threads, and they lack many APIs. And we are not going to delay Firefox 4 or Mozilla 2 for a minute adding new ChromeWorker APIs. This is a very old bug that uses XPCOM, JS, and threads. It dates from when that seemed like a good idea. It does not translate to ChromeWorkers. What we should do, which would help this bug and I am pretty sure a myriad of bugs yet to be filed, is MT wrappers (bug 566951). /be

In *this* bug I'm not convinced we need separate threads at all. All of the callbacks and API usage here should be entirely on the main thread.

I have no idea how I would do asynchronous reading from another process without creating a thread for it. But I think it's possible to keep the callbacks and listeners on the main thread in the C++ code and/or move the StreamListener from JS to C++.

I've looked at some OS APIs recently, and I think you're right, that you can't do asynchronous IO without involving additional threads, at least on some OSes. But if the multi-thread stuff can be kept in C++, not JS, I guess that would work. Just have to move the dispatch code, really, maybe including creating additional C++ objects. I'm happy to help any way that I can. Just shoot me an email if there's anything I can do.

Is this currently not working at all or is there some way it should work? With a current nightly I get: "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIPipeTransport.openPipe]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: subprocess.jsm :: anonymous :: line 388" data: no]

Please read the last comments (comment 199 onwards). This module doesn't work with current nightly builds.

(In reply to comment #210) > Please read the last comments (comment 199 onwards). This module doesn't work > with current nightly builds. Hm, because of this FireFTP is now broken (which depends on IPC when making an SFTP connection). How do I rework the IPC code to overcome this new hurdle?

I'm working on a fix ...

I have attached a new version that works with FF 4.0b7 and current nightly builds of FF and TB (tested on Mac OS X x64). I created a new helper class implementing nsIRunnable which is used to dispatch callbacks to on{Start|Stop}Request and onDataAvailable back to the thread which initiated asyncRead(). Given comment #202, that's not necessarily the main thread, but the thread that must have created the referenced objects. I didn't yet look into the non-ascii parameter issue on Windows.

(In reply to comment #213) > I have attached a new version that works with FF 4.0b7 and current nightly > builds of FF and TB (tested on Mac OS X x64). Thanks Patrick! Sadly Bugzilla isn't able to show the differences between both patches. Is that a problem when attaching non-git format enabled patches? Would have been nice to be able to see a diff between both versions.

(In reply to comment #214) > Thanks Patrick! Sadly Bugzilla isn't able to show the differences between both > patches. Is that a problem when attaching non-git format enabled patches? Would > have been nice to be able to see a diff between both versions. It's really just a problem with bugzilla being stupid. There isn't much one can do about it.

Great work, Patrick! I've got it to work on Mac and Linux. Windows is having some problems though... I'm compiling off the code at Mozdev. One thing to note is that the Makefile's in the build directory a little out-of-date there - they're missing the EXTRA_BUILD_OPTS = /NODEFAULTLIB:LIBCMT part. After adding that, I got it to compile. However, Firefox crashes (only on Windows) when calling this._pipeTransport.openPipe. Not sure what that's about...ideas?

(In reply to comment #216) > However, Firefox crashes (only on Windows) when calling > this._pipeTransport.openPipe. Not sure what that's about...ideas? Do you have a crash report?

(In reply to comment #216) > I'm compiling off the code at Mozdev. If I get it right, you compile the IPC library from the Enigmail repository? That's not exactly identical to the patch here, the patch is only a subset of the IPC library in the Enigmail repository. > One thing to note is that the Makefile's > in the build directory a little out-of-date there - they're missing the > EXTRA_BUILD_OPTS = /NODEFAULTLIB:LIBCMT part. After adding that, I got it to > compile. Well, yes that's possible, I have stopped maintaining the creation of the library for other purposes than Enigmail given the patch attached here. > However, Firefox crashes (only on Windows) when calling > this._pipeTransport.openPipe. Not sure what that's about...ideas? I'd say this is a compile/compatibility issue between your build and the Firefox version you're using. My build works fine on Windows, I don't have any crashes (neither with Enigmail nor with subprocess.jsm). I have attached a slightly corrected patch; it fixes compile issues on Windows.

Here's a crash report: http://crash-stats.mozilla.com/report/index/c4250297-da3c-460f-9e31-9be062101123 Yes, I'm compiling off the library in the Enigmail repository. I realize it's not exactly the same as this patch - but it seems to be almost exact for all intensive purposes? Again, for what it's worth, it works fine on Mac and Linux. I'm using Visual Studio 2008 Express and it's Firefox 4beta7 and I'm running ./makemake in the ipc part of Enigmail. I'll continue fiddling with it unless there's something that I'm missing.

Ok, I got it working now. You're right - I see now the code in the Enigmail repository is more than not exactly in line with the patch that's here. I'm able to compile/run it correctly with the code that you've posted here. Thank you!

I had to make some minor modifications to the makefiles to get this extension to be included in a normal Firefox build. Details in the attached diff against patch 6.1. Or am I missing the something? Is this extension going to be included with Firefox by default at some point in the future?

Here is a new patch with some minor corrections. I fixed the non-ascii issue on Windows mentioned in comment 196 and incorporated Rufus' changes. From my point of view the patch should now be ready for review. Who would do this?

(In reply to comment #222) > From my point of view the patch should now be ready for review. Who would do > this? http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbug68702.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D498332&file= gives a spot check. Some of its concerns are valid. I can read over the patch on the weekend and offer my feedback, but I'm not a qualified reviewer. I'm deeply concerned that this patch has no automated tests with it, especially for a patch over 50KB in size. Also, for anyone who's unclear on the subject, I see no way this is going to make it for Firefox 4.

(In reply to comment #223) > I'm deeply concerned that this patch has no automated tests with it, especially > for a patch over 50KB in size. Having test coverage would be a requirement for landing in mozilla-central per the tree rules.

(In reply to comment #223) > http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbug68702.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D498332&file= > gives a spot check. Some of its concerns are valid. I fixed all those that I found valid. > I'm deeply concerned that this patch has no automated tests with it, especially > for a patch over 50KB in size. I have a number of test cases; I'll try to make them compliant with usual test cases and add them to the patch. > Also, for anyone who's unclear on the subject, I see no way this is going to > make it for Firefox 4. I did not expect anything else :-)