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.

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.

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

Frontend and frontend tests: http://hg.mozilla.org/mozilla-central/rev/0f37ae194f8f

Dammit, wrong patch.

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.

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.

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

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

Review ping.

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

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

Review ping.

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 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 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 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 on attachment 491584 [details] [diff] [review] part m: Windows impl of Transport API r=me.

Comment on attachment 491586 [details] [diff] [review] part o: Use the existing IPC::Channel typedef in AsyncChannel r=me.

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

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

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

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

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

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

Like so?

Review ping.

Review ping.

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

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

http://hg.mozilla.org/mozilla-central/rev/bb9d620459b3 http://hg.mozilla.org/mozilla-central/rev/72b8d4bd224a http://hg.mozilla.org/mozilla-central/rev/92f023b812b5 http://hg.mozilla.org/mozilla-central/rev/cf1d7d326774 http://hg.mozilla.org/mozilla-central/rev/89e1092dd8f5 http://hg.mozilla.org/mozilla-central/rev/99517bc69c73 http://hg.mozilla.org/mozilla-central/rev/c79e66766e15 http://hg.mozilla.org/mozilla-central/rev/373bd1df3979 http://hg.mozilla.org/mozilla-central/rev/76bdbaa76db4 http://hg.mozilla.org/mozilla-central/rev/fed06cc42545 http://hg.mozilla.org/mozilla-central/rev/55159c6e44cb http://hg.mozilla.org/mozilla-central/rev/bfca3d0525da http://hg.mozilla.org/mozilla-central/rev/39217d4a17fb http://hg.mozilla.org/mozilla-central/rev/804819e48cec http://hg.mozilla.org/mozilla-central/rev/6588b377a600 http://hg.mozilla.org/mozilla-central/rev/d11b3f7ed46a http://hg.mozilla.org/mozilla-central/rev/3bc8c3c8b753 http://hg.mozilla.org/mozilla-central/rev/f821b1e13478