885653 - Add support for IPDL bridges that connect two of the same protocol

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Teach the compiler that protocols can bridge two of the same protocol

The issue with this patch is that we end up generating code like 
    
    virtual PBrowserParent*
    AllocPBrowser(
            Transport* transport,
            ProcessId otherProcess) = 0;
    virtual PBrowserChild*
    AllocPBrowser(
            Transport* transport,
            ProcessId otherProcess) = 0;

which isn't legal C++.  How would you feel about renaming the (De)Alloc functions to contain Child/Parent in the name?

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Use sets for channelOpenedActors

We need this because we want both PContentParent and PContentChild to be able to open PBrowser, but don't want two PBrowserChild* AllocPBrowser declarations in PContentChild.

Comment 3

[David Zbarsky (:dzbarsky)]

Comment on attachment 765807 [details] [diff] [review]
Part 2: Use sets for channelOpenedActors

I don't think we need this.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Add an IPDL test for recursive bridges

This is basically the approach we want.  A ContentChild (TestRecursiveBridgeMainChild) asks the ContentParent (TestRecursiveBridgeMainParent) to create a new ContentParent and bridge them with a ContentBridge (TestRecursiveBridgeSub).  The two ContentChild's allocate the two halves of the bridge.  We can then use the bridge singleton to create and manage PBrowsers.

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 765802 [details] [diff] [review]
Part 1: Teach the compiler that protocols can bridge two of the same protocol

diff --git a/ipc/ipdl/ipdl/type.py b/ipc/ipdl/ipdl/type.py
index 07a6abd..fff695a 100644
--- a/ipc/ipdl/ipdl/type.py
+++ b/ipc/ipdl/ipdl/type.py

@@ -1776,11 +1776,12 @@ class BuildProcessGraph(TcheckVisitor):
             if pproc == cproc:
                 if parentSideActor is not None:
-                    self.error(bridges.loc,
-                               "ambiguous bridge `%s' between `%s' and `%s'",
-                               bridgeProto.name(),
-                               parentSideProto.name(),
-                               childSideProto.name())
+                    if parentSideProto != childSideProto:
+                        self.error(bridges.loc,
+                                   "ambiguous bridge `%s' between `%s' and `%s'",
+                                   bridgeProto.name(),
+                                   parentSideProto.name(),
+                                   childSideProto.name())

if parentSideActor is not None and parentSideProto != childSideProto

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 766530 [details] [diff] [review]
Part 2: Add an IPDL test for recursive bridges

I don't pretend to know what's going on here, but I presume you copy-pasted some existing test and changed it so as to cover recursive bridging.  That seems fine...

Comment 7

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #5)
> Comment on attachment 765802 [details] [diff] [review]
> Part 1: Teach the compiler that protocols can bridge two of the same protocol
> 
> diff --git a/ipc/ipdl/ipdl/type.py b/ipc/ipdl/ipdl/type.py
> index 07a6abd..fff695a 100644
> --- a/ipc/ipdl/ipdl/type.py
> +++ b/ipc/ipdl/ipdl/type.py
> 
> @@ -1776,11 +1776,12 @@ class BuildProcessGraph(TcheckVisitor):
>              if pproc == cproc:
>                  if parentSideActor is not None:
> -                    self.error(bridges.loc,
> -                               "ambiguous bridge `%s' between `%s' and
> `%s'",
> -                               bridgeProto.name(),
> -                               parentSideProto.name(),
> -                               childSideProto.name())
> +                    if parentSideProto != childSideProto:
> +                        self.error(bridges.loc,
> +                                   "ambiguous bridge `%s' between `%s' and
> `%s'",
> +                                   bridgeProto.name(),
> +                                   parentSideProto.name(),
> +                                   childSideProto.name())
> 
> if parentSideActor is not None and parentSideProto != childSideProto

That's not the same as what I did, and in fact causes us to not generate the AllocPContentBridge functions because we go through the pproc == cproc case twice and overwrite the results of the first one the second time.

Comment 8

[Justin Lebar (not reading bugmail)]

Gah, you're right, there's an else down there.  Sorry.

Comment 9

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/395ba08436fd

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/395ba08436fd