Last Comment Bug 564086 - IPDL: Offer a PFoo::Bridge(process1, process2) interface to open a new IPC channel between process1/process2
: IPDL: Offer a PFoo::Bridge(process1, process2) interface to open a new IPC ch...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla7
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 666981
Blocks: 556846 562770 598867 613442 e10s-plugin-ipc
  Show dependency treegraph
 
Reported: 2010-05-05 18:20 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-06-24 11:14 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Frontend support for IPDL process graphs and Bridge()ing processes (26.66 KB, patch)
2010-05-06 15:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mozilla+ben: review+
Details | Diff | Splinter Review
part b: Suppress some spurious static-ctor/dtor warnings in IPDL tests (1.12 KB, patch)
2010-11-18 10:28 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part c: Remove dependency on XPCOM in subprocesses-spawning code (3.42 KB, patch)
2010-11-18 10:29 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part d: Emit common code for PFoo into PFoo.h and PFoo.cpp (1.98 KB, patch)
2010-11-18 10:29 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that are not unit tests in and of themselves (4.72 KB, patch)
2010-11-18 10:29 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
ted: review+
Details | Diff | Splinter Review
part f: Compile IPDL specs in two passes so that the full process graph is available at codegen time (1.98 KB, patch)
2010-11-18 10:30 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part g: Allow opening an AsyncChannel with an explicit parent/child "flavor" so that Transport::Connect can be called for parent-side channels that need it (3.30 KB, patch)
2010-11-18 10:30 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part h: One protocol can bridge multiple endpoints (oops!); add convenience process-graph querying functions (6.49 KB, patch)
2010-11-18 10:30 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part i: Add an (IPDL-private) interface for getting the underlying AsyncChannel from a top-level actor (6.05 KB, patch)
2010-11-18 10:31 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part j: Add IPC::Channel support for getting OS-level pipe info and creating from existing pipe descriptors (12.67 KB, patch)
2010-11-18 10:31 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part k: API for creating new IPC "Transport" not tied to a ProcessHost (2.93 KB, patch)
2010-11-18 10:31 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: superreview+
Details | Diff | Splinter Review
part l: POSIX impl of Transport API (6.29 KB, patch)
2010-11-18 10:32 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part m: Windows impl of Transport API (6.72 KB, patch)
2010-11-18 10:32 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part n: Build new Transport code (1.77 KB, patch)
2010-11-18 10:32 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part o: Use the existing IPC::Channel typedef in AsyncChannel (3.02 KB, patch)
2010-11-18 10:33 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part p: Library support of |bridge| API (7.05 KB, patch)
2010-11-18 10:33 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part q: Generate C++ goop for creating |bridge| channels (21.70 KB, patch)
2010-11-18 10:33 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part r: Test IPDL |bridge| (13.57 KB, patch)
2010-11-18 10:34 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part d: Emit common code for PFoo into PFoo.h and PFoo.cpp (13.32 KB, patch)
2010-11-18 10:35 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part g: Allow opening an AsyncChannel with an explicit parent/child "side" so that Transport::Connect can be called for parent-side channels that need it, v2 (3.29 KB, patch)
2011-04-27 22:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part j: Add IPC::Channel support for getting OS-level pipe info and creating from existing pipe descriptors, v2 (14.12 KB, patch)
2011-04-27 23:29 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part p: Library support of |bridge| API, v2 (8.68 KB, patch)
2011-04-28 00:51 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part c: Remove dependency on XPCOM in subprocesses-spawning code, v2 (4.59 KB, patch)
2011-05-02 13:12 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
benjamin: review+
Details | Diff | Splinter Review
part c.1: Allow non-XPCOM processes (which don't use omnijar) to spawn other non-XPCOM subprocesses (3.63 KB, patch)
2011-06-02 22:03 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mh+mozilla: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 18:20:19 PDT
IPDL will be extended with two new constructs to implement this.  The first is

  protocol PFoo { (parent|child) spawns PBar [as parent]; }

which means that "either the parent or child side of a PFoo connection will launch a new PBar process, optionally as a 'parent' of the launching PFoo process".  Second is

  protocol PFoo { bridges PBar, PBaz; }

which means that PFoo can be created as a "bridge" protocol between PBar- and PBaz-speaking processes.  The order of the arguments says which side will get the new PFoo parent actor and which gets the child: parent first, child second.

Using these two types of declarations, the IPDL compiler can infer the entire process graph, and generate exactly the Bridge() methods it needs (rather than a generic interface).  The process graph is useful for other things too, like checking that the graph is a dag wrt parent/child edges (and thus deadlock free) and generating pretty pictures.  The constraint on Bridge() is that it must be called from a process that has "handles", top-level actors, that refer to the processes being bridged.

This is a somewhat confusing system, but an example should help.  For jetpacks, we'll have

  protocol PContent { parent spawns PJetpack; }
  protocol PJetpack { }
  protocol PJetpackContent { bridges PJetpack, PContent; }

From this, the IPDL compiler will infer the following process graph, where each box is a process

 +----------------+----------------+
 | PContentParent | PJetpackParent | (chrome)
 +----------------+----------------+==============++
     ||                                           ||
     ||                                           \/
     ||             +-----------------------+---------------+
     ||             | PJetpackContentParent | PJetpackChild | (jetpack)
     ||             +-----------------------+---------------+
     ||                   ||
     \/                   \/
 +---------------+----------------------+
 | PContentChild | PJetpackContentChild | (content)
 +---------------+----------------------+

Because IPDL knows that PJetpackContent bridges PContent and PJetpack, and it knows that PContentParent and PJetpackParent live in the same process, it knows to spit out a

  bool PJetpackContent::Bridge(PJetpackParent*, PContentParent*)

static method (thanks for benjamn for the nice syntax there) that's only callable from the chrome process.

There's a remaining C++ design to finish, which is how to notify PJetpackChild and PContentChild that the new PJetpackContent connection was opened, and have them allocate concrete PJetpackContent objects.  Probably a combination of  Alloc*/Dealloc* methods and an OnBridge() notification will suffice.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-06 15:41:15 PDT
Created attachment 443980 [details] [diff] [review]
Frontend support for IPDL process graphs and Bridge()ing processes

The meat of this patch is ipc/ipdl/ipdl/type.py.  There's a bigger-than-necessary change to parser.py because I had to make the ProtocolBody grammar right-recursive rather than piecewise left-recursive to avoid a stupid shift/reduce conflict stemming from "parent:" and "parent spawns ..." syntax.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-06 15:44:52 PDT
Here's what the compiler deduces for the new content/jetpack/et al. abstraction in the IPDL unit tests:

$ ipdlc -I ~/mozilla/mozilla-central/ipc/ipdl/test/ipdl/ok ~/mozilla/mozilla-central/ipc/ipdl/test/ipdl/ok/jetpackContent.ipdl
Generated C++ headers will be generated relative to "."
Generated C++ sources will be generated in "."
Parsing specification /home/cjones/mozilla/mozilla-central/ipc/ipdl/test/ipdl/ok/jetpackContent.ipdl
Processes
   |contentParent|jetpackParent|compositorChild|
     (contentParent)--spawns-->(compositorParent)
     (contentParent)--spawns-->(jetpackChild)
   |jetpackContentChild|
   |jetpackContentParent|
   |jetpackChild|
   |contentChild|pluginParent|
     (contentChild)--spawns-->(pluginChild)
Bridges
   (jetpackChild)--jetpackContent bridge-->(contentChild)
Comment 3 Ben Newman (:bnewman) (:benjamn) 2010-05-07 18:48:22 PDT
Comment on attachment 443980 [details] [diff] [review]
Frontend support for IPDL process graphs and Bridge()ing processes

Assuming you fixed the spawnsSuprotocol/subprotocolSpawns test regressions, this all looks good / makes sense to me.

Just one comment about python idioms.  In a number of places you use len(container) as a truth value to test the non-emptiness of the container, f.e. (changes mine):

@@ -1063,14 +1061,14 @@ class CheckTypes(TcheckVisitor):
         
         # check that we require no more "power" than our manager protocols
         ptype, pname = p.decl.type, p.decl.shortname
 
-        if len(p.spawnsStmts) and not ptype.isToplevel():
+        if p.spawnsStmts and not ptype.isToplevel():
             self.error(p.decl.loc,
                        "protocol `%s' is not top-level and so cannot declare |spawns|",
                        pname)
 
-        if len(p.bridgesStmts) and not ptype.isToplevel():
+        if p.bridgesStmts and not ptype.isToplevel():
             self.error(p.decl.loc,
                        "protocol `%s' is not top-level and so cannot declare |bridges|",
                        pname)
 
This is unnecessary in python, as

  not not [] == False
  not not set() == False
  not not {} == False
  not not () == False

By itself this doesn't really matter, but I mention it just in case you assume otherwise in some spots.  I didn't go looking for examples outside this patch, but I know I've run into bugs in the past by assuming that |if []:| behaved differently from |if None:|.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-22 12:35:59 PDT
Frontend and frontend tests: http://hg.mozilla.org/mozilla-central/rev/0f37ae194f8f
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:28:48 PST
Created attachment 491571 [details] [diff] [review]
part b: Suppress some spurious static-ctor/dtor warnings in IPDL tests
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:29:06 PST
Created attachment 491572 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:29:26 PST
Created attachment 491573 [details] [diff] [review]
part d: Emit common code for PFoo into PFoo.h and PFoo.cpp
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:29:47 PST
Created attachment 491574 [details] [diff] [review]
part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that are not unit tests in and of themselves
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:30:09 PST
Created attachment 491575 [details] [diff] [review]
part f: Compile IPDL specs in two passes so that the full process graph is available at codegen time
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:30:29 PST
Created attachment 491576 [details] [diff] [review]
part g: Allow opening an AsyncChannel with an explicit parent/child "flavor" so that Transport::Connect can be called for parent-side channels that need it
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:30:49 PST
Created attachment 491577 [details] [diff] [review]
part h: One protocol can bridge multiple endpoints (oops!); add convenience process-graph querying functions
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:31:16 PST
Created attachment 491579 [details] [diff] [review]
part i: Add an (IPDL-private) interface for getting the underlying AsyncChannel from a top-level actor
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:31:37 PST
Created attachment 491580 [details] [diff] [review]
part j: Add IPC::Channel support for getting OS-level pipe info and creating from existing pipe descriptors
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:31:59 PST
Created attachment 491581 [details] [diff] [review]
part k: API for creating new IPC "Transport" not tied to a ProcessHost
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:32:19 PST
Created attachment 491582 [details] [diff] [review]
part l: POSIX impl of Transport API
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:32:36 PST
Created attachment 491584 [details] [diff] [review]
part m: Windows impl of Transport API
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:32:56 PST
Created attachment 491585 [details] [diff] [review]
part n: Build new Transport code
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:33:16 PST
Created attachment 491586 [details] [diff] [review]
part o: Use the existing IPC::Channel typedef in AsyncChannel
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:33:34 PST
Created attachment 491587 [details] [diff] [review]
part p: Library support of |bridge| API
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:33:53 PST
Created attachment 491588 [details] [diff] [review]
part q: Generate C++ goop for creating |bridge| channels
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:34:11 PST
Created attachment 491589 [details] [diff] [review]
part r: Test IPDL |bridge|
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 10:35:26 PST
Created attachment 491590 [details] [diff] [review]
part d: Emit common code for PFoo into PFoo.h and PFoo.cpp

Dammit, wrong patch.
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 11:01:07 PST
General overview:

The backend impl of this works approximately as follows, continuing the example of comment 0 where we have a chrome process (P), content process (C), and jetpack process (J).

 - J decides it wants a bridge to C, speaking protocol PJetpackContent (naming scheme TBD!)
 - code in P calls PJetpackContent::Bridge(JetpackParent* jp, ContentParent* cp), using the references to the two top-level actors that exist in P
 - low-level code creates a new IPC Transport, socketpair() or pipe
 - IPDL-generated code takes the "server" end of that transport and sends it to the JetpackChild in J.  This messages says, "hey a bridge has been opened to C"
 - IPDL-generated code takes the "client" end of that transport and sends it to the ContentChild in C.
 - in J, IPDL-generated code asks the JetpackChild to create a new PJetpackContentParent actor
 - in C, IPDL-generated code asks the ContentChild to create a new PJetpackContentChild actor
 - (IPDL-generated code is done at that point)
 - next it's the responsibility of user C++ to call actor->Open(...) on the newly-created actors

I still fear this is somewhat confusing, but there's really not a simpler way.  Taking a look at the unit test might be helpful for understanding the details.  Please ping with questions.

Roadmap:

Parts b and c are just minor fixes for annoyances that only manifest in IPDL unit tests.

Part d has us emit a PFoo.cpp file for common code, instead of just PFoo.h.  We need this for bridge because the PFoo::Bridge(PBar, PBaz) method is emitted in PFoo.h, but its impl needs the definitions of PBar and PBaz.  So the .cpp break cycles that would appear by trying to emit this in .h.  (This might incidentally save a few bytes of codesighs.)

Part e allows IPDL tests to spawn subprocesses that speak protocols that aren't unit tests in and of themselves.  We need this to test |bridge|, because for that the unit-test subprocess needs to spawn its own subprocess.

Part f just has the IPDL compiler parse and type-check all protocols before emitting code.  |bridge| needs this because at codegen time, a bridge protocol PFoo needs to know that it bridges PBar and PBaz so that the proper C++ can be emitted.  In the current setup, if PFoo is parsed and translated before PBar and PBaz, PFoo won't know that it's a bridge.  This is a pretty trivial patch and doesn't change the emitted code at all.  (It does tie IPDL semantics a bit more to compilation strategy, but ... *crickets*.)

Part g is kind of a hack.  In the current setup, GeckoChildProcessHost in parent process inherits an already-opened-and-connected Transport.  The child process does not; it needs to open and connect its Transport.  We use a gross hack in AsyncChannel to figure out which.  For bridge however, both parent *and* child sides need to explicitly be connected.  We can't connect them in low-level code, because the Transport needs "the right" listener immediately after connecting, or else messages can be dropped in between connect and set-listener.  This is a hack on top of a hack to allow overriding the older hack for bridge channels.

Part h fixes some oversights in the original front-end impl.  Pretty trivial.

Part i adds a (private!!) GetIPCChannel() interface to top-level actors.  Bridge needs this because when a new bridge channel PFoo is connected across PBar and PBaz, the generated C++ sends messages to the PBar and PBaz actors to notify them of the new bridge.  We need some way to generically send messages to any actor from low-level code, and this interface is it: the low-level code just grabs the AsyncChannel from the actors and sends a message directly through that interface.

Part j shoehorns more mozilla code into the chromium IPC stuff.  Bridge needs to create arbitrary Transports in arbitrary processes, and that breaks some assumptions in the chromium code.  (It would be cleaner to throw that out and start anew, but that ain't gonna happen in the 4.0/4.1 timeframe.)

Part k is the low-level API used only by part p.  It's a generic interface for creating a Transport and packaging the OS-level descriptors into a TransportDescriptor that can be shared across IPC.

Parts l and m are the system hackery to impl the API.  Part n just turns it on in the build.

Part o has us rely slightly less on the chromium classes, minor patch.

Part p is the (private!!) interface used by IPDL-generated C++ to create the bridge Transport and notify the bridged endpoints.

Part q is all the codegen goop to spit out the PFoo::Bridge interface, and the AllocPBridge() interfaces.

Again, please ping me with questions.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 11:07:38 PST
The one "big-picture" issue that arose while writing these patches is that they allow creating IPC transport things in child processes and sending descriptors to the chrome process, say.  I couldn't think of any security issues that arise from this; a compromised subprocess can already send arbitrary garbage over existing pipes to the chrome process, which is why we want to sanitize data and check protocol state.  Sending a descriptor to something not an IPC pipe doesn't seem to present new issues either.  At worst I think we'll just get silent failure.  If we wanted to be really strenuous, we could probably use OS-level interfaces to check the descriptor type (is this a pipe?) and creating process (did the process that said it created it actually do so?).

In a sandbox world, we'd need to allow socketpair() and CreateNamedPipe() in subprocesses (which should be fine, AFAICT).  The other alternative is proxying all Transport-creation to chrome, but the complexity of that makes my knees wobble a bit.  It's doable though.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 11:09:38 PST
(Allowing CreateNamedPipe() might lead to issues since the names go into a global namespace, but we should change this code to use anonymous pipes.  Gecko never uses the name for anything, it's just a waste.)
Comment 26 Ted Mielczarek [:ted.mielczarek] 2010-11-30 06:32:58 PST
Comment on attachment 491574 [details] [diff] [review]
part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that are not unit tests in and of themselves

># HG changeset patch
># User Chris Jones <jones.chris.g@gmail.com>
># Date 1290104871 21600
># Node ID 5d3af0fe9ac89e14569eb3fcf0e99ac1a318e2d5
># Parent  dd339523b9b47315f5d9a579eb038c83a71cec77
>Bug 564086, part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that are not unit tests in and of themselves. r=ted
>
>diff --git a/ipc/ipdl/test/cxx/Makefile.in b/ipc/ipdl/test/cxx/Makefile.in
>--- a/ipc/ipdl/test/cxx/Makefile.in
>+++ b/ipc/ipdl/test/cxx/Makefile.in
>@@ -81,16 +81,18 @@ IPDLTESTS = \
>   TestSyncWakeup \
>   TestSyncHang \
>   $(NULL)
> 
> ifeq ($(OS_ARCH),Linux)
> IPDLTESTS += TestSysVShmem
> endif
> 
>+EXTRA_PROTOCOLS =

You fill this in in a later patch, right?

>@@ -102,17 +104,17 @@ CPPSRCS =  \
> include $(topsrcdir)/config/config.mk
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
> include $(topsrcdir)/config/rules.mk
> 
> 
> IPDLUNITTEST_BIN = $(DEPTH)/dist/bin/ipdlunittest$(BIN_SUFFIX)
> 
> IPDLUnitTests.cpp : Makefile.in $(GENTESTER) $(TESTER_TEMPLATE) $(IPDLTESTHDRS)
>-	$(PYTHON) $(GENTESTER) $(TESTER_TEMPLATE) $(IPDLTESTS) > $@
>+	$(PYTHON) $(GENTESTER) $(TESTER_TEMPLATE) -t $(IPDLTESTS) -e $(EXTRA_PROTOCOLS) > $@

Will this cause an error if EXTRA_PROTOCOLS is empty?

>diff --git a/ipc/ipdl/test/cxx/genIPDLUnitTests.py b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
>--- a/ipc/ipdl/test/cxx/genIPDLUnitTests.py
>+++ b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
>@@ -32,71 +32,91 @@
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> import string, sys
> 
>+def usage():
>+    print >>sys.stderr, """
>+%s template_file -t unit_tests... -e extra_protocols...
>+
>+  TEMPLATE_FILE is used to generate to generate the unit-tester .cpp
>+  UNIT_TESTS are the top-level protocols defining unit tests
>+  EXTRA_PROTOCOLS are top-level protocols for subprocesses that can be
>+                  spawned in tests but are not unit tests in and of
>+                  themselves
>+"""% (sys.argv[0])
>+    sys.exit(1)
>+
> def main(argv):
>     template = argv[1]
>-    unittests = argv[2:]
>+
>+    if argv[2] != '-t': usage()
>+    i = 3
>+    unittests = []
>+    while argv[i] != '-e':
>+        unittests.append(argv[i])
>+        i += 1

This cmdline handling is getting ugly, just use OptionParser: http://docs.python.org/library/optparse.html

> '''    case %s: {
>         %sChild** child =
>             reinterpret_cast<%sChild**>(&gChildActor);
>         *child = new %sChild();
>         (*child)->Open(transport, parent, worker);
>         return;
>     }
>-'''% (t, t, t, t) for t in unittests ])
>+'''% (t, t, t, t) for t in unittests+extras ])

You might consider something like:

'''%(class)s** child =
...

''' % {'class': t} for t in unittests+extras

r=me with those nits addressed.
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-30 09:20:01 PST
(In reply to comment #26)
> Comment on attachment 491574 [details] [diff] [review]
> part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that
> are not unit tests in and of themselves
> 
> ># HG changeset patch
> ># User Chris Jones <jones.chris.g@gmail.com>
> ># Date 1290104871 21600
> ># Node ID 5d3af0fe9ac89e14569eb3fcf0e99ac1a318e2d5
> ># Parent  dd339523b9b47315f5d9a579eb038c83a71cec77
> >Bug 564086, part e: Allow IPDL/C++ tests to spawn subprocesses that speak protocols that are not unit tests in and of themselves. r=ted
> >
> >diff --git a/ipc/ipdl/test/cxx/Makefile.in b/ipc/ipdl/test/cxx/Makefile.in
> >--- a/ipc/ipdl/test/cxx/Makefile.in
> >+++ b/ipc/ipdl/test/cxx/Makefile.in
> >@@ -81,16 +81,18 @@ IPDLTESTS = \
> >   TestSyncWakeup \
> >   TestSyncHang \
> >   $(NULL)
> > 
> > ifeq ($(OS_ARCH),Linux)
> > IPDLTESTS += TestSysVShmem
> > endif
> > 
> >+EXTRA_PROTOCOLS =
> 
> You fill this in in a later patch, right?
> 

Yes.

> >@@ -102,17 +104,17 @@ CPPSRCS =  \
> > include $(topsrcdir)/config/config.mk
> > include $(topsrcdir)/ipc/chromium/chromium-config.mk
> > include $(topsrcdir)/config/rules.mk
> > 
> > 
> > IPDLUNITTEST_BIN = $(DEPTH)/dist/bin/ipdlunittest$(BIN_SUFFIX)
> > 
> > IPDLUnitTests.cpp : Makefile.in $(GENTESTER) $(TESTER_TEMPLATE) $(IPDLTESTHDRS)
> >-	$(PYTHON) $(GENTESTER) $(TESTER_TEMPLATE) $(IPDLTESTS) > $@
> >+	$(PYTHON) $(GENTESTER) $(TESTER_TEMPLATE) -t $(IPDLTESTS) -e $(EXTRA_PROTOCOLS) > $@
> 
> Will this cause an error if EXTRA_PROTOCOLS is empty?
> 

No.

> >diff --git a/ipc/ipdl/test/cxx/genIPDLUnitTests.py b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
> >--- a/ipc/ipdl/test/cxx/genIPDLUnitTests.py
> >+++ b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
> >@@ -32,71 +32,91 @@
> > # and other provisions required by the GPL or the LGPL. If you do not delete
> > # the provisions above, a recipient may use your version of this file under
> > # the terms of any one of the MPL, the GPL or the LGPL.
> > #
> > # ***** END LICENSE BLOCK *****
> > 
> > import string, sys
> > 
> >+def usage():
> >+    print >>sys.stderr, """
> >+%s template_file -t unit_tests... -e extra_protocols...
> >+
> >+  TEMPLATE_FILE is used to generate to generate the unit-tester .cpp
> >+  UNIT_TESTS are the top-level protocols defining unit tests
> >+  EXTRA_PROTOCOLS are top-level protocols for subprocesses that can be
> >+                  spawned in tests but are not unit tests in and of
> >+                  themselves
> >+"""% (sys.argv[0])
> >+    sys.exit(1)
> >+
> > def main(argv):
> >     template = argv[1]
> >-    unittests = argv[2:]
> >+
> >+    if argv[2] != '-t': usage()
> >+    i = 3
> >+    unittests = []
> >+    while argv[i] != '-e':
> >+        unittests.append(argv[i])
> >+        i += 1
> 
> This cmdline handling is getting ugly, just use OptionParser:
> http://docs.python.org/library/optparse.html
> 

I started with a rewrite to OptionParser, but I couldn't find a clean way to handle two vararg lists with its API ("-e PX -e PY -e PZ" is silly).  So, the added goop didn't seem to pay itself off, especially considering humans don't use this script, only this Makefile.in.

> > '''    case %s: {
> >         %sChild** child =
> >             reinterpret_cast<%sChild**>(&gChildActor);
> >         *child = new %sChild();
> >         (*child)->Open(transport, parent, worker);
> >         return;
> >     }
> >-'''% (t, t, t, t) for t in unittests ])
> >+'''% (t, t, t, t) for t in unittests+extras ])
> 
> You might consider something like:
> 
> '''%(class)s** child =
> ...
> 
> ''' % {'class': t} for t in unittests+extras
> 

Yeah good idea, it's time for that.
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-23 12:12:03 PDT
Review ping.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-28 13:22:57 PDT
Ben S. proposed going over this and bug 613442 in person at the all-hands/work-week.  Sounds like a great idea to me.
Comment 30 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-04-12 10:02:08 PDT
Comment on attachment 491572 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code

This feels kinda wrong, but that's partly because I don't understand the problem. Is the problem that we need the GRE_DIR code on mac, but we can't have it for the IPDL tests? Can we only run this codepath for chrome-type processes?
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-12 11:02:31 PDT
(In reply to comment #30)
> Comment on attachment 491572 [details] [diff] [review]
> part c: Remove dependency on XPCOM in subprocesses-spawning code
> 
> This feels kinda wrong, but that's partly because I don't understand the
> problem. Is the problem that we need the GRE_DIR code on mac, but we can't have
> it for the IPDL tests? Can we only run this codepath for chrome-type processes?

The problem here was that a subprocess needed to spawn its own subprocess and that didn't work.  In the time after this patch was written, an independent IPDL test was pushed that spawns its own subprocess and ISTR fixing that for mac.  This patch may be unnecessary now.
Comment 32 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-04-13 13:09:16 PDT
Comment on attachment 491572 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code

Marking r- for now, talk to me about it if you end up needing this or something like it.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 18:03:02 PDT
Review ping.
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 18:03:55 PDT
Comment on attachment 491572 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code

It turns out we still need this.  Otherwise we crash trying to get the GRE_DIR from the directory service, as you guessed.

So let's talk :).
Comment 35 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:42:39 PDT
Comment on attachment 491576 [details] [diff] [review]
part g: Allow opening an AsyncChannel with an explicit parent/child "flavor" so that Transport::Connect can be called for parent-side channels that need it

>     if(!aIOLoop) {
>+        NS_PRECONDITION(aFlavor == Unknown, "expected default flavor arg");

Nit: Reverse this if test since both conditions have a block.

>+    enum Flavor { Parent, Child, Unknown };

I don't really like 'Flavor', it's totally ambiguous. Potential choices: CommunicationRole, ChannelDirection, PipeEnd (?), Mode. What do you think?
Comment 36 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:42:43 PDT
Comment on attachment 491577 [details] [diff] [review]
part h: One protocol can bridge multiple endpoints (oops!); add convenience process-graph querying functions

>-        if actor not in cls.actorToProcess:
>+        if actor not in cls.actorToProcess:           

Looks like you added a bunch of trailing whitespace there.

r=me with that fixed.
Comment 37 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:42:51 PDT
Comment on attachment 491580 [details] [diff] [review]
part j: Add IPC::Channel support for getting OS-level pipe info and creating from existing pipe descriptors

>--- a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
> ...
>+Channel::ChannelImpl::ChannelImpl(int fd, Mode mode, Listener* listener)

Eek. Can we make an Init function or something that sets all these members to be called by both constructors? Duplicating all this seems like a bad idea. (I hear C++0x has a fix for this type of problem!)

>--- a/ipc/chromium/src/chrome/common/ipc_channel_win.cc
> ...
>+Channel::ChannelImpl::ChannelImpl(const std::wstring& channel_id,

Same comment about duplicating all the member initialization.

>+  if (mode == MODE_SERVER) {
>+    // Use the existing handle that was dup'd to us
>+    pipe_ = server_pipe;
>+    EnqueueHelloMessage();
>+  } else {
>+    // Take the normal init path to connect to the server pipe
>+    CreatePipe(channel_id, mode);
>+  }
>+}

Hm... You didn't do this in the posix impl (you just always call EnqueueHelloMessage). Why the difference? 


>+void* Channel::GetServerPipeHandle() const {

DCHECK that we're a server here like you did in the posix impl?
Comment 38 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:42:58 PDT
Comment on attachment 491584 [details] [diff] [review]
part m: Windows impl of Transport API

r=me.
Comment 39 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:43:05 PDT
Comment on attachment 491586 [details] [diff] [review]
part o: Use the existing IPC::Channel typedef in AsyncChannel

r=me.
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 10:53:05 PDT
Comment on attachment 491587 [details] [diff] [review]
part p: Library support of |bridge| API

>+class ChannelOpened : public IPC::Message
>+{
>+public:
>+  ChannelOpened(TransportDescriptor aDescriptor,
>+                ProcessId aOtherProcess,
>+                ProtocolId aProtocol)
>+    : IPC::Message(MSG_ROUTING_CONTROL, // these only go to top-level actors
>+                   CHANNEL_OPENED_MESSAGE_TYPE,
>+                   PRIORITY_NORMAL)
>+  {
>+    IPC::WriteParam(this, aDescriptor);
>+    IPC::WriteParam(this, aOtherProcess);
>+    IPC::WriteParam(this, static_cast<uint32>(aProtocol));

Static assert that ProtocolId is same size as uint32?

>+Bridge(const PrivateIPDLInterface&,
> ...
>+  return (aParentChannel->Send(new ChannelOpened(parentSide,
>+                                                 childId,
>+                                                 aProtocol)) &&
>+          aChildChannel->Send(new ChannelOpened(childSide,
>+                                                parentId,
>+                                                aProtocol)));

Hm, if either of those fail you'll presumably leak the HANDLE you duplicated on windows... I think you need a cleanup process.

>diff --git a/ipc/glue/ProtocolUtils.h b/ipc/glue/ProtocolUtils.h
> ...
>+struct PrivateIPDLInterface {};

Hm, what's the point of this if it's declared in a public header?
Comment 41 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-21 11:22:11 PDT
Comment on attachment 491588 [details] [diff] [review]
part q: Generate C++ goop for creating |bridge| channels

Rename _backstagePass() to _privateIPDLInterface()? BackstagePass is the global class for JS components, confusing name.

>+    def genBridgeFunc(self, bridge):
> ...
>+        parentvar = ExprVar('parentHandle')
>+            

Nit: whitespace

r=me with that.
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 22:52:49 PDT
Created attachment 528795 [details] [diff] [review]
part g: Allow opening an AsyncChannel with an explicit parent/child "side" so that Transport::Connect can be called for parent-side channels that need it, v2

(In reply to comment #35)
> Comment on attachment 491576 [details] [diff] [review]
> part g: Allow opening an AsyncChannel with an explicit parent/child "flavor" so
> that Transport::Connect can be called for parent-side channels that need it
> 
> >     if(!aIOLoop) {
> >+        NS_PRECONDITION(aFlavor == Unknown, "expected default flavor arg");
> 
> Nit: Reverse this if test since both conditions have a block.
> 

Done.

> >+    enum Flavor { Parent, Child, Unknown };
> 
> I don't really like 'Flavor', it's totally ambiguous. Potential choices:
> CommunicationRole, ChannelDirection, PipeEnd (?), Mode. What do you think?

Went with "Side".
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 22:53:44 PDT
(In reply to comment #36)
> Comment on attachment 491577 [details] [diff] [review]
> part h: One protocol can bridge multiple endpoints (oops!); add convenience
> process-graph querying functions
> 
> >-        if actor not in cls.actorToProcess:
> >+        if actor not in cls.actorToProcess:           
> 
> Looks like you added a bunch of trailing whitespace there.
> 
> r=me with that fixed.

Fixed.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 23:29:08 PDT
Created attachment 528804 [details] [diff] [review]
part j: Add IPC::Channel support for getting OS-level pipe info and creating from existing pipe descriptors, v2

(In reply to comment #37)
> Comment on attachment 491580 [details] [diff] [review]
> part j: Add IPC::Channel support for getting OS-level pipe info and creating
> from existing pipe descriptors
> 
> >--- a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
> > ...
> >+Channel::ChannelImpl::ChannelImpl(int fd, Mode mode, Listener* listener)
> 
> Eek. Can we make an Init function or something that sets all these members to
> be called by both constructors? Duplicating all this seems like a bad idea. (I
> hear C++0x has a fix for this type of problem!)
>
> >--- a/ipc/chromium/src/chrome/common/ipc_channel_win.cc
> > ...
> >+Channel::ChannelImpl::ChannelImpl(const std::wstring& channel_id,
> 
> Same comment about duplicating all the member initialization.
> 

OK.

> >+  if (mode == MODE_SERVER) {
> >+    // Use the existing handle that was dup'd to us
> >+    pipe_ = server_pipe;
> >+    EnqueueHelloMessage();
> >+  } else {
> >+    // Take the normal init path to connect to the server pipe
> >+    CreatePipe(channel_id, mode);
> >+  }
> >+}
> 
> Hm... You didn't do this in the posix impl (you just always call
> EnqueueHelloMessage). Why the difference? 

The windows pipe code is different.  Both ends of the POSIX socketpair are connected after creation, so all the setup has already been done, but the client of the windows named pipe has to open its end of the pipe.

> >+void* Channel::GetServerPipeHandle() const {
> 
> DCHECK that we're a server here like you did in the posix impl?

Can't; the windows impl doesn't remember its |mode| after the ctor finishes.
Comment 45 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-28 00:51:50 PDT
Created attachment 528807 [details] [diff] [review]
part p: Library support of |bridge| API, v2

(In reply to comment #40)
> Comment on attachment 491587 [details] [diff] [review]
> part p: Library support of |bridge| API
> 
> >+class ChannelOpened : public IPC::Message
> >+{
> >+public:
> >+  ChannelOpened(TransportDescriptor aDescriptor,
> >+                ProcessId aOtherProcess,
> >+                ProtocolId aProtocol)
> >+    : IPC::Message(MSG_ROUTING_CONTROL, // these only go to top-level actors
> >+                   CHANNEL_OPENED_MESSAGE_TYPE,
> >+                   PRIORITY_NORMAL)
> >+  {
> >+    IPC::WriteParam(this, aDescriptor);
> >+    IPC::WriteParam(this, aOtherProcess);
> >+    IPC::WriteParam(this, static_cast<uint32>(aProtocol));
> 
> Static assert that ProtocolId is same size as uint32?

The storage size for enums is up to compiler implementations, so that's not a valid assertion.  We use this cast in a million places in our code, so if we find a compiler it's broken on this will be just one of the sites we need to auto-rewrite ;).

> 
> >+Bridge(const PrivateIPDLInterface&,
> > ...
> >+  return (aParentChannel->Send(new ChannelOpened(parentSide,
> >+                                                 childId,
> >+                                                 aProtocol)) &&
> >+          aChildChannel->Send(new ChannelOpened(childSide,
> >+                                                parentId,
> >+                                                aProtocol)));
> 
> Hm, if either of those fail you'll presumably leak the HANDLE you duplicated on
> windows... I think you need a cleanup process.

Good call.  Added CloseDescriptor().

> >diff --git a/ipc/glue/ProtocolUtils.h b/ipc/glue/ProtocolUtils.h
> > ...
> >+struct PrivateIPDLInterface {};
> 
> Hm, what's the point of this if it's declared in a public header?

The header is "public" for build-system reasons; it's not to be used by random C++.  That struct forces code calling the functions to say |Foo(PrivateIPDLInterface(), ...)|, which is hopefully hint enough that the code trying to call the functions is wrong :).
Comment 46 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-28 00:53:05 PDT
(In reply to comment #41)
> Comment on attachment 491588 [details] [diff] [review]
> part q: Generate C++ goop for creating |bridge| channels
> 
> Rename _backstagePass() to _privateIPDLInterface()? BackstagePass is the global
> class for JS components, confusing name.
> 
> >+    def genBridgeFunc(self, bridge):
> > ...
> >+        parentvar = ExprVar('parentHandle')
> >+            
> 
> Nit: whitespace
> 
> r=me with that.

Fixed.
Comment 47 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-02 09:36:34 PDT
Comment on attachment 491572 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code

I really don't like calling do_GetService at all from code which doesn't know whether XPCOM has been initialized. AFAICT, the only case where we want to follow the XPCOM path is in a chrome process, right?

Could we put this whole block in "if (GeckoProcessType_Default == XRE_GetProcessType())", and fail harder if the directory service is null in that case?
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-02 13:12:53 PDT
Created attachment 529554 [details] [diff] [review]
part c: Remove dependency on XPCOM in subprocesses-spawning code, v2

Like so?
Comment 49 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-04 15:27:08 PDT
Review ping.
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-10 10:05:50 PDT
Review ping.
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-02 22:03:52 PDT
Created attachment 537078 [details] [diff] [review]
part c.1: Allow non-XPCOM processes (which don't use omnijar) to spawn other non-XPCOM subprocesses

IPDL unit test subprocesses don't use XPCOM so don't have "Omnijar" initialized.  The subprocesses spawned by the test subprocesses don't need omnijar either.
Comment 52 Mike Hommey [:glandium] 2011-06-02 22:21:49 PDT
Comment on attachment 537078 [details] [diff] [review]
part c.1: Allow non-XPCOM processes (which don't use omnijar) to spawn other non-XPCOM subprocesses

Review of attachment 537078 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +528,5 @@
>    childArgv.push_back(exePath.value());
>  
>    childArgv.insert(childArgv.end(), aExtraOpts.begin(), aExtraOpts.end());
>  
> +  if (Omnijar::IsInitialized()) {

r=me, provided one of your other patches adds "using mozilla;", because at the moment, ipc/glue/GeckoChildProcessHost.cpp doesn't.
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-02 23:58:41 PDT
(In reply to comment #52)
> r=me, provided one of your other patches adds "using mozilla;", because at
> the moment, ipc/glue/GeckoChildProcessHost.cpp doesn't.

It's not necessary, mozilla:: decls are visible to mozilla::ipc::GeckoChildProcessHost::PerformAsyncLaunch.

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