Closed Bug 1316755 Opened 3 years ago Closed 3 years ago

Remove Bridges/Opens/Spawns from IPDL, replace with endpoints

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: billm, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

The Endpoint<> code is more powerful and easier to use than Bridge/Open. It's also getting a good amount of testing now that gfx code is using it. We should replace existing uses of Bridge/Open with endpoints. That will simplify replacement of lower.py.

Users of Bridge:
PContentBridge: This is kind of a b2g thing, but we probably want to keep supporting it.
PGMPContent
PPluginModule

Users of Open:
PBackground
PGMPContent
PGMPService
PProcessHangMonitor

I think we might be able to simplify the GMP code somewhat as well, but it might be simpler to do a direct translation at first.
(In reply to Bill McCloskey (:billm) from comment #0)
> The Endpoint<> code is more powerful and easier to use than Bridge/Open.
> It's also getting a good amount of testing now that gfx code is using it. We
> should replace existing uses of Bridge/Open with endpoints. That will
> simplify replacement of lower.py.
> 
> Users of Bridge:
> PContentBridge: This is kind of a b2g thing, but we probably want to keep

It's now also used in the jsplugin process and maybe remote iframe in the future.

> supporting it.
> PGMPContent
> PPluginModule
> 
> Users of Open:
> PBackground
> PGMPContent
> PGMPService
> PProcessHangMonitor
> 
> I think we might be able to simplify the GMP code somewhat as well, but it
> might be simpler to do a direct translation at first.
(In reply to Bill McCloskey (:billm) from comment #0)
> The Endpoint<> code is more powerful and easier to use than Bridge/Open.
> It's also getting a good amount of testing now that gfx code is using it. We
> should replace existing uses of Bridge/Open with endpoints. That will
> simplify replacement of lower.py.

Yeah, so I'm going to be adding a new consumer of Bridge for the Pepper plugins work. If I understand correctly, if I have two child processes (A and B) with corresponding protocols A' and B' from chrome process to child process, and I want to bridge the child sides of A' and B' with a protocol C, then I would need to send an Endpoint for C over A' to the chrome process and from there over B' to B?
Flags: needinfo?(wmccloskey)
(In reply to Peter Van der Beken [:peterv] from comment #2)
> Yeah, so I'm going to be adding a new consumer of Bridge for the Pepper
> plugins work. If I understand correctly, if I have two child processes (A
> and B) with corresponding protocols A' and B' from chrome process to child
> process, and I want to bridge the child sides of A' and B' with a protocol
> C, then I would need to send an Endpoint for C over A' to the chrome process
> and from there over B' to B?

See here for a description:
http://searchfox.org/mozilla-central/source/ipc/glue/ProtocolUtils.h#485

You can create the endpoints anywhere and send them anywhere. The tricky part is that you need to know the final pids of both sides when you create the endpoints. So generally it's easiest if the chrome process creates the endpoints since it creates the processes. However, the only time we use the pids is for OtherPid(), which is only for logging and for the protocol's own use. So maybe we could get rid of the need for pids, or change how they get set or something.
Flags: needinfo?(wmccloskey)
Depends on: 1319595
Depends on: 1321052
Depends on: 1321871
There's a comment in PContent.ipdl that starts with "This is pretty ridiculous" that could likely be removed once bridges and opens are removed.
I realized that we can probably remove the syntax for this from IPDL before we do anything else. We could just generate the Bridge and Open methods on all protocols.
Depends on: 1329797
Another thing that will need to be removed is "spawns": http://searchfox.org/mozilla-central/search?q=spawns&path=.ipdl

There are two of those. My patches in bug 1321871 removes the PGMPService one. The other one is in PContent, and is for PPluginModule.
Summary: Removing Bridge/Open from IPDL, replace with endpoints → Removing Bridge/Open/Spawns from IPDL, replace with endpoints
Depends on: 1333915
Depends on: 1333917
There are now bugs blocking this bug for every remaining use of bridges, opens, and spawns. So, there aren't too many. We can even start getting rid of eg opens when it is gone (bug 1329797) though maybe that's not worth the hassle.
Assignee: nobody → continuation
Summary: Removing Bridge/Open/Spawns from IPDL, replace with endpoints → Remove Bridges/Opens/Spawns from IPDL, replace with endpoints
This removes about 750 lines of Python, slightly more than state machine removal (bug 1316757).
Depends on: 1335103
try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80316dfcd2616876643defdb59b521289b0c3317

I'll wait until the blocking bugs are closer to landing before putting my patches up for review.
Attachment #8831372 - Attachment is obsolete: true
I'm still waiting on reviews for bug 1333917, so there's no big hurry to review this. It just deletes a lot of code, so hopefully it won't be too hard to review.
Comment on attachment 8831371 [details]
Bug 1316755, part 1 - Remove IPDL unit tests that use bridges, opens, and spawns.

https://reviewboard.mozilla.org/r/107926/#review109708
Attachment #8831371 - Flags: review?(wmccloskey) → review+
Comment on attachment 8831373 [details]
Bug 1316755, part 2 - Remove bridges, opens and spawns from IPDL.

https://reviewboard.mozilla.org/r/107930/#review109712

Yay!
Attachment #8831373 - Flags: review?(wmccloskey) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92eff00a7ed2
part 1 - Remove IPDL unit tests that use bridges, opens, and spawns. r=billm
https://hg.mozilla.org/integration/autoland/rev/1f16dda1409a
part 2 - Remove bridges, opens and spawns from IPDL. r=billm
https://hg.mozilla.org/mozilla-central/rev/92eff00a7ed2
https://hg.mozilla.org/mozilla-central/rev/1f16dda1409a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.