Closed Bug 68702 (ipc) Opened 24 years ago Closed 13 years ago

Implement inter-process communication (IPC) in Mozilla (protozilla)

Categories

(Core :: Networking, enhancement, P2)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: svn, Assigned: patrick)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(2 files, 32 obsolete files)

review comments for v6.2
55.39 KB, text/plain
Details
Patch v7
231.08 KB, patch
Details | Diff | Splinter Review 
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.
Blocks: protozilla
Target Milestone: --- → Future
Severity: normal → major
Keywords: mozilla1.0, patch, review
Target Milestone: Future → ---
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
Assignee: neeti → 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
Target Milestone: --- → mozilla1.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.
QA Contact: tever → benc
One comment on getEnv. This should be handled per process, IMHO. It's a process
property, not a system property.

Axel
->dougt
Assignee: gagan → 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.
Assignee: dougt → svn
Target Milestone: mozilla1.0 → mozilla0.9.4
> 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.


Assignee: svn → dougt

    
    

    
  
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
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  


  

    
    

    
  
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.

    
    

    
  
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
Target Milestone: mozilla0.9.5 → mozilla0.9.6

    
    

    
  
reassigning to brendan.  Waiting on his review.
Assignee: dougt → brendan

    
    

    
  
*** 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
Target Milestone: mozilla0.9.6 → mozilla0.9.7

    
    

    
  
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?
Target Milestone: mozilla0.9.7 → mozilla0.9.8

    
    

    
  
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.

    
  
Blocks: 80439
Summary: [RFE] Implement inter-process communication (IPC) in Mozilla → [RFE] Implement inter-process communication (IPC) in Mozilla (protozilla)

    
    

    
  


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

        Attachment #29704 -
        Attachment is obsolete: true

        Attachment #45652 -
        Attachment is obsolete: true

        Attachment #47176 -
        Attachment is obsolete: true

        Attachment #48170 -
        Attachment is obsolete: true

        Attachment #61625 -
        Attachment is obsolete: true

    
    

    
  
On target for 0.9.9.

/be
Target Milestone: mozilla0.9.8 → mozilla0.9.9

    
    

    
  
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
Target Milestone: mozilla0.9.9 → mozilla1.0

    
    

    
  
> 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
Target Milestone: mozilla1.0 → ---

    
    

    
  
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.
Keywords: mozilla1.0 → mozilla1.2

    
  
Alias: ipc

    
    

    
  
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.

        Attachment #66139 -
        Attachment is obsolete: true

    
    

    
  
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.
Severity: major → enhancement

    
  
Summary: [RFE] Implement inter-process communication (IPC) in Mozilla (protozilla) → Implement inter-process communication (IPC) in Mozilla (protozilla)

    
    

    
  
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.)

    
  
Blocks: 156493

    
    

    
  
Fixing TM.

/be
Target Milestone: --- → mozilla1.3beta

    
    

    
  
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.
Blocks: 161125

    
    

    
  
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.
Status: NEW → ASSIGNED
Target Milestone: mozilla1.3beta → mozilla1.4alpha

    
    

    
  
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
No longer blocks: 156493
Keywords: mozilla1.2
Priority: -- → P2
Target Milestone: mozilla1.4alpha → mozilla1.5alpha

    
    

    
  
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
Target Milestone: mozilla1.5alpha → mozilla1.6alpha

    
  
No longer blocks: protozilla

    
    

    
  
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
Assignee: brendan → svn
Status: ASSIGNED → NEW
Target Milestone: mozilla1.6alpha → ---

    
    

    
  


  
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)

    
  
Blocks: 332503

    
    

    
  
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.
Assignee: svn → patrick.brunschwig

        Attachment #45653 -
        Attachment is obsolete: true

        Attachment #93374 -
        Attachment is obsolete: true

        Attachment #135791 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #217001 -
        Flags: review?

    
  

        Attachment #217001 -
        Flags: review? → review?(dougt)

    
    

    
  
darin, benjamin,

any thoughts on landing IPC support as implemented here?  

see : news://news.mozilla.org:119/lp6dnSv55cdsswDZnZ2dnUVZ_sednZ2d@mozilla.org

Doug

    
    

    
  

      
      
      
      

        Attached file
          
        IPCpipe API overview (obsolete)
        — 
      

    


  
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.
Flags: blocking1.9?

    
    

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

    
    

    
  
Not a blocker, but we'd take it.
Flags: blocking1.9? → blocking1.9-

    
    

    
  
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.

        Attachment #217001 -
        Flags: review?(dougt)

    
    

    
  


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

        Attachment #217001 -
        Attachment is obsolete: true

    
    

    
  


  
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.

    
  
Blocks: wanted4seamonkey

        Attachment #246983 -
        Attachment mime type: message/rfc822 → text/plain

    
    

    
  

      
      
      
      

        Attached file
          
        A perl script for IPC to drive (obsolete)
        — 
      

    


  
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).

    
    

    
  

      
      
      
      

        Attached file
          
        Frozen-linkage compliant IPC tarball  (obsolete)
        — 
      

    


  
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.

        Attachment #273308 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        IPC tarball v1.2.0 (obsolete)
        — 
      

    


  
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

        Attachment #273365 -
        Attachment is obsolete: true

        Attachment #292602 -
        Attachment is obsolete: true

    
    

    
  
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.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?

    
    

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

Flags: wanted1.9.0.x?
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1-

    
    

    
  
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.
URL: http://mozilla-enigmail.org/ipc/

    
    

    
  
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.

        Attachment #458330 -
        Attachment is obsolete: true

    
    

    
  
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).

        Attachment #246983 -
        Attachment is obsolete: true

        Attachment #286121 -
        Attachment is obsolete: true

        Attachment #286445 -
        Attachment is obsolete: true

        Attachment #299132 -
        Attachment is obsolete: true

        Attachment #458635 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        Improved version of subprocess.jsm (obsolete)
        — 
      

    


  
(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.

    
  

        Attachment #470193 -
        Attachment mime type: application/octet-stream → text/plain

    
    

    
  
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'

    
    

    
  

      
      
      
      

        Attached file
          
        Tarball fixed for Windows (obsolete)
        — 
      

    


  
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.

        Attachment #469938 -
        Attachment is obsolete: true

        Attachment #470193 -
        Attachment is obsolete: true

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached file
          
        Tarball v3 (obsolete)
        — 
      

    


  
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!

        Attachment #470775 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        Tarball v4 (obsolete)
        — 
      

    


  
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.

        Attachment #477087 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        Tarball v5 (obsolete)
        — 
      

    


  
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.

        Attachment #478861 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  
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?
Depends on: 608142

    
    

    
  
(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 ...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #302192 -
        Attachment is obsolete: true

        Attachment #480522 -
        Attachment is obsolete: true

        Attachment #486170 -
        Attachment is obsolete: true

    
    

    
  
(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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6.1 (obsolete)
        — 
          — Splinter Review
      

    


  
(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.

        Attachment #492190 -
        Attachment is obsolete: true

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6.2 (obsolete)
        — 
          — Splinter Review
      

    


  
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?

        Attachment #492940 -
        Attachment is obsolete: true

        Attachment #496841 -
        Attachment is obsolete: true

    
    

    
  
(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 :-)

    
    

    
  
The onFinished handler isn't called for me, and, perhaps consequently, capturing stderr also doesn't work. I can't spot any interface changes in the docs, and I'm pretty sure my test script was working before the patch was rewritten, so I don't think this is my fault, but a bug of some kind. If you can't reproduce, I can provide some specific code.

It also seems to be leaking memory somewhere again.

And still outputting this bogus error message in a debug build:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80470002: file .../mozilla/extensions/ipc-pipe/src/nsPipeTransport.cpp, line 1641

Using Patch v6.2 (186.39 KB, patch) 2010-12-17 07:47 PST, Patrick Brunschwig

    
    

    
  
I'm using Thunderbird trunk builds from today, and I don't have this issue on Windows and Mac OS X. Are you sure that your subprocess really terminates correctly? Feel free to mail me your code directly

    
    

    
  
Patrick - I'm still going through the patch - it's taking me far longer than I thought it would.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6.3 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #226)
> The onFinished handler isn't called for me, and, perhaps consequently,
> capturing stderr also doesn't work. I can't spot any interface changes in the
> docs, and I'm pretty sure my test script was working before the patch was
> rewritten, so I don't think this is my fault, but a bug of some kind. If you
> can't reproduce, I can provide some specific code.

Right, my fault -- I posted a wrong patch which called onStartRequest instead of onStopRequest at the end.

> It also seems to be leaking memory somewhere again.

I think this is a consequence of the above. If onStopRequest isn't called, then the referenced stream & objects are not released.

The attached new patch also fixes all relevant errors found with http://beaufour.dk/jst-review/. I'm still working on the unit tests.

        Attachment #498332 -
        Attachment is obsolete: true

    
    

    
  
Sorry, I forgot to mention something important for those who use nsPipeTransport and nsIPCBuffer directly: I changed the Contract ID's to avoid conflicts with Enigmail.

    
    

    
  


  
(In reply to comment #229)
> Created attachment 498734 [details] [diff] [review]
> Patch v6.3
> The attached new patch also fixes all relevant errors found with
> http://beaufour.dk/jst-review/. I'm still working on the unit tests.

Heh, dang it, I just finished the 6.2 review!  :p

A couple observations from the interdiff:

(1) Ignore what the review tool says about JavaScript.  It parses JS and does its reviews incorrectly, except for the line length parts.  Seriously, don't trust it for that.

(2) It looks like most of the things I found in the v6.2 patch also apply to the v6.3 patch - that is, I spotted several things that the tool didn't suggest, and that consequently you probably haven't noticed.

(3) Please don't take any of the comments I made personally.  I really went through it with a fine-tooth comb, and found many parts that did not look right.  I understand you didn't necessarily write most of the code I'm critiquing, but you are submitting the patch for review.  The onus is on you to do what you can to fix it.

Major issues:
* No LGPL licensing.
* I spotted four different ways JS could cause a crash.
* Lots of missing JavaDoc.
* No tests (but you already said you're working on it).
* Everything has thread-safety on the implementations, but I'm not seeing any evidence of cycle collection (maybe not necessary - I don't know if threadsafe and CC are mutually exclusive).
* Inconsistent use of nsresult rv.

    
    

    
  
(In reply to comment #231)
> * Everything has thread-safety on the implementations, but I'm not seeing any
> evidence of cycle collection (maybe not necessary - I don't know if threadsafe
> and CC are mutually exclusive).
Anything that might be addrefed/released off of the main thread cannot be cycle collected (last I knew; I'd be surprised if that changed).

    
    

    
  
Thanks a lot for the review!

(In reply to comment #231)
> Major issues:
> * No LGPL licensing.

This is really an issue. As you may be aware I'm not the original author of the C++ part (that's the reporter of the bug), even though I touched quite a bit of it since I took it over. But I certainly can't change the licensing without his permission, and I'm not sure if he is still somehow reachable :-(

    
    

    
  
(In reply to comment #233)
> This is really an issue. As you may be aware I'm not the original author of the
> C++ part (that's the reporter of the bug), even though I touched quite a bit of
> it since I took it over. But I certainly can't change the licensing without his
> permission, and I'm not sure if he is still somehow reachable :-(

Looks like you can reach him via his mozdev address at least:
http://protozilla.mozdev.org/contact.html

    
    

    
  
Working for me now in an FF build from the Hg repo tagged 4.0b8. Leaks and bogus error message also gone.

    
    

    
  
(In reply to comment #234)
> (In reply to comment #233)
> > This is really an issue. As you may be aware I'm not the original author of the
> > C++ part (that's the reporter of the bug), even though I touched quite a bit of
> > it since I took it over. But I certainly can't change the licensing without his
> > permission, and I'm not sure if he is still somehow reachable :-(
> 
> Looks like you can reach him via his mozdev address at least:
> http://protozilla.mozdev.org/contact.html

That page is outdated for ca. 7 years :-) But I managed to find him -- he's now professor at the Texas A&M University. He granted me permission to add LGPL to the source code.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v7
        — 
          — Splinter Review
      

    


  
Here is a new patch. Alex, many thanks for the review! I followed most of your suggestions and I think the result is a much more robust implementation than before.

Changelog:
* added LGPL license to all files
* added checks for parameters passed
* improved return codes
* added unit tests
* added JavaDocs
* fixed platform-specific bugs on Mac OS X and Windows (1 each)

Who can formally review this patch now?

        Attachment #498734 -
        Attachment is obsolete: true

    
  

        Attachment #500665 -
        Flags: review?(doug.turner)

    
    

    
  
is your patch missing file name?

    
    

    
  
I'm not sure I understand what you mean. It shouldn't miss any file name -- at least I can download and compile/test it as expected.

    
    

    
  
dougt:  if you look at the patch from the "diff" link, then it looks like filenames are missing.  If you look at the patch raw, the filenames are there.

    
  

        Attachment #500665 -
        Flags: review?(doug.turner)

    
    

    
  
Happy 10th Birthday, Bug 68702!

May you never turn 11!

(And thanks to all the devs over the last 10 years who have worked on this bug, and particularly to the people who have been working so hard the last couple of months to get this up to scratch so it can get into the trunk soon. It is really appreciated.)

    
    

    
  
The ipc-pipe code has received new home at http://hg.mozilla.org/ipccode/ (i.e. a HG toplevel repository)!

Benjamin agreed to decide whether or not to include the code by default in future versions of Firefox -- post FF 4.0 obviously.

For the moment, I simply committed the patch v7 attached here, and thus I consider this bug "fixed" :-).
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
    

    
  
Congrats on finally "landing" it, Patrick :)

Benjamin, even though this bug is resolved - for those of us watching it, it would be great if you could post your decision here once you make it.  Thanks!

    
    

    
  
(In reply to comment #242)
> The ipc-pipe code has received new home at http://hg.mozilla.org/ipccode/ (i.e.
> a HG toplevel repository)!
> 

Can you possibly commit a README of some sort that tells us/others how to build a Firefox/Gecko with ipccode?

Such as "Clone the repo to: <m-c>/some/dir/"
"...."
etc.

I can probably figure it out, but as long as it lives, self-sustained, in a separate repo anyone wishing to use it will be happy to get that information.

    
    

    
  
(In reply to comment #244)
> Can you possibly commit a README of some sort that tells us/others how to build
> a Firefox/Gecko with ipccode?
> 
> Such as "Clone the repo to: <m-c>/some/dir/"
> "...."
> etc.
> 
> I can probably figure it out, but as long as it lives, self-sustained, in a
> separate repo anyone wishing to use it will be happy to get that information.

I think I can have a decent README written up for Patrick's review this weekend, if you're willing to wait.  However, a new bug should be filed for that.  

This bug gets lots of traffic.  I recommend using this bug in the future only for marking other IPC bugs as blocking this.
Depends on: 635573

    
  
Blocks: 93822

    
  
Depends on: 662160

    
    

    
  
Where's the bug for including the code/ turning it on by default?
Depends on: 731716

    
    

    
  
Jan Gerber and I have developed an entirely new version of subprocess, written in pure JavaScript using js-ctypes and ChromeWorkers (i.e. no need to compile anything except for the test cases). 

Due to the fact that some OS (eg. Windows) require separate native threads if pipes shall not block, the new library requires Gecko 8.0 or newer. 

The API provided is very similar but not identical to the old subprocess API.

The new code is now committed to hg.mozilla.org/ipccode.
Depends on: 732889

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: