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

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
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?
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #765802 - Flags: review?(justin.lebar+bug)
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.
Attachment #765807 - Flags: review?(justin.lebar+bug)
Blocks: 879475
Depends on: 885804
Comment on attachment 765807 [details] [diff] [review]
Part 2: Use sets for channelOpenedActors

I don't think we need this.
Attachment #765807 - Attachment is obsolete: true
Attachment #765807 - Flags: review?(justin.lebar+bug)
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.
Attachment #766530 - Flags: feedback?(justin.lebar+bug)
Summary: Add support for recursive IPDL bridges → Add support for IPDL bridges that connect two of the same protocol
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
Attachment #765802 - Flags: review?(justin.lebar+bug) → review+
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...
Attachment #766530 - Flags: feedback?(justin.lebar+bug) → feedback+
(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.
Gah, you're right, there's an else down there.  Sorry.
https://hg.mozilla.org/mozilla-central/rev/395ba08436fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.