Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Description • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 1 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 2 • 14 years ago

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)

	Ben Newman (:bnewman) (:benjamn)
	Comment 3 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 4 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 22 • 14 years ago

Dammit, wrong patch.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 23 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 24 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 25 • 14 years ago

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

	(not currently active) Ted Mielczarek
	Comment 26 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 27 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 28 • 13 years ago

Review ping.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 29 • 13 years ago

Ben S. proposed going over this and bug 613442 in person at the all-hands/work-week.  Sounds like a great idea to me.

	Benjamin Smedberg
	Comment 30 • 13 years ago

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?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 31 • 13 years ago

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

	Benjamin Smedberg
	Comment 32 • 13 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 33 • 13 years ago

Review ping.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 34 • 13 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 35 • 13 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 36 • 13 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 37 • 13 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 38 • 13 years ago

Comment on attachment 491584 [details] [diff] [review]
part m: Windows impl of Transport API

r=me.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 39 • 13 years ago

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

r=me.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 40 • 13 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 41 • 13 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 42 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 43 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 44 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 45 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 46 • 13 years ago

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

	Benjamin Smedberg
	Comment 47 • 13 years ago

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?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 48 • 13 years ago

Like so?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 49 • 13 years ago

Review ping.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 50 • 13 years ago

Review ping.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 51 • 13 years ago

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.

	Mike Hommey [:glandium]
	Comment 52 • 13 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 53 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 54 • 13 years ago

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