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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 2

[Peter Van der Beken [:peterv]]

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

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

This removes about 750 lines of Python, slightly more than state machine removal (bug 1316757).

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1316755, part 1 - Remove IPDL unit tests that use bridges, opens, and spawns.

Review commit: https://reviewboard.mozilla.org/r/107926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107926/

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Bug 1316755, part 2 - Eliminate spurious usage of Actor in lower.py.

Actor is used in the process checking code, which will get deleted
later.

Review commit: https://reviewboard.mozilla.org/r/107928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107928/

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1316755, part 2 - Remove bridges, opens and spawns from IPDL.

Review commit: https://reviewboard.mozilla.org/r/107930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107930/

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8831371 [details]
Bug 1316755, part 1 - Remove IPDL unit tests that use bridges, opens, and spawns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107926/diff/1-2/

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8831373 [details]
Bug 1316755, part 2 - Remove bridges, opens and spawns from IPDL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107930/diff/1-2/

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8831371 [details]
Bug 1316755, part 1 - Remove IPDL unit tests that use bridges, opens, and spawns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107926/diff/2-3/

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8831373 [details]
Bug 1316755, part 2 - Remove bridges, opens and spawns from IPDL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107930/diff/2-3/

Comment 17

[Andrew McCreight [:mccr8]]

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 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8831373 [details]
Bug 1316755, part 2 - Remove bridges, opens and spawns from IPDL.

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

Yay!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Andrew!

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92eff00a7ed2
https://hg.mozilla.org/mozilla-central/rev/1f16dda1409a