1475415 - JS IPDL API

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, enhancement, P5)

Description

[Tristan Bourvon]

This bug tracks the development of a JS IPDL API.

This API is intended for use from chrome JS to send inter-process messages using IPDL in a similar fashion to how C++ does it right now.

The IPDL API allows more structure and correctness compared to the message manager, and should also enable JS-to-C++ IPC communication in the future.

More information will be added with each patch building that API.

Comment 1

[Tristan Bourvon]

Attachment: Bug 1475415 - Add first version of IPDL-JS API

Add the first version of the IPDL-JS API, which allow chrome JS to load
IPDL files and use them to communicate accross Content processes.
See IPDLProtocol.h for more information regarding how to use the API.

Comment 2

[Tristan Bourvon]

So here is a description of what this first patch does and how.

An IPDL global JS object is exposed through the window object to Chrome only. It is defined using WebIDL with IPDL.webidl and implemented in IPDL.h/.cpp.
This IPDL JS object resolves all of its properties dynamically, except `registerTopLevelClass` and `getTopLevelInstance-s`, which are used respectively to indicate the JS class used for the top level protocol, and to retrieve the instances of this protocol which have been created for the content parents/children.
The property resolution is used to dynamically and lazily load IPDL protocols.

To implement a protocol in JS, simply subclass the abstract protocol class, just as in C++:

`class TestParent extends IPDL.PTestParent {...}`

Like with C++, the recvXXX methods have to be overriden, as well as the allocXXX methods. All the other methods are optional, and the sendXXX methods are fully implemented. Promises are used for async messages.

How this works under the cover is that each IPDL protocol is represented using the IPDLProtocol.h/cpp class. This represents the protocol definition/class, and provides the constructor and prototype for the abstract class. It also uses JSClass to include itself in the private data field of the abstract JS class prototype.

When creating a new instance of a subclass of the protocol class, the constructor will be called and will take care of creating a new JS object with the proper JSClass, and it will also create a new IPDLProtocolInstance.h/cpp object and attach it to the IPDL protocol JS instance using the private data field.

When sending a message, the dispatch methods of the protocol JSClass take care of unwrapping the IPDLProtocolInstance object from the JS object and forwarding the call properly.

The message is then send to the appropriate IPCInterface.h/cpp, which takes care of sending the message through AsyncMessageIPDL/SyncMessageIPDL defined in PContent.

On the receiving side, the IPCInterface pulls up the appropriate IPDLProtocolInstance depending on the protocol name and channel id, and forwards the message to it. The IPDLProtocolInstance then calls the recvXXX handler, before sending back the return value.

When creating a sub protocol, the behavior is also similar to C++ with the sendXXXConstructor/allocXXX system.
However, for deletion, we have no deallocXXX since JS is GC'ed. Cleanups are still possible in recv__delete__.
(...aaaaaand I just realized I forgot to make sure we get rid of our internal reference to the IPDL protocol instance... gotta fix that in a follow-up patch).

It's forbidden to send messages in the `constructor()` of the IPDL class, but recvXXXConstructor allows you to send messages after the object is constructed on the receiving end.

The correct ContentParent is magically attached to each protocol instance because they are always constructed in a C++-called environment (top level protocols when a new ContentParent is created, and subprotocols when calling allocXXX).

If you have any additional design questions, feel free to ask away.

Comment 3

[Andrew McCreight [:mccr8]]

I haven't had time to look into this patch in detail, but I thought I'd point out that you shouldn't actually add in all of those IPDL tests, as they are all already in the tree. :)

Comment 4

[Tristan Bourvon]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> I haven't had time to look into this patch in detail, but I thought I'd
> point out that you shouldn't actually add in all of those IPDL tests, as
> they are all already in the tree. :)

I kind of hesitated on doing that. Since we plan to replace the IPDL package altogether (and C++ codegen is well underway for the Rust parser), I was thinking that it would be better to make the two packages completely independent? But yeah, it's definitely duplication, maybe we can just move the stuff when we retire the Python part. What do you think?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Tristan Bourvon from comment #4)
> (and C++ codegen is well underway for the Rust parser),

Who is working on that, out of curiosity?

> I was thinking that it would be better to make the two packages completely
> independent? But yeah, it's definitely duplication, maybe we can just move
> the stuff when we retire the Python part. What do you think?

I want to leave the existing tests where they are, so we keep the history. It is convenient if you want to see the commit that added a certain feature from the test. I guess that matter less if the code is removed, but it is still nice to be able to see the original bug, which can contain examples and motivation for a feature.

I guess if the idea is to have the stuff in m-c be a mirror of a git repo, it doesn't hurt to have a second copy of the tests around for now.

I also feel like these files should be under ipc/ipdl/new or something, rather than a new separate top level directory under ipc, but maybe that's just me. It can get moved later, I suppose.

At some point your parser changes should be put into the Rust IPDL parser repo, but I can look over the changes and do that myself later.

Comment 6

[Tristan Bourvon]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> (In reply to Tristan Bourvon from comment #4)
> > (and C++ codegen is well underway for the Rust parser),
> 
> Who is working on that, out of curiosity?

I am!

> > I was thinking that it would be better to make the two packages completely
> > independent? But yeah, it's definitely duplication, maybe we can just move
> > the stuff when we retire the Python part. What do you think?
> 
> I want to leave the existing tests where they are, so we keep the history.
> It is convenient if you want to see the commit that added a certain feature
> from the test. I guess that matter less if the code is removed, but it is
> still nice to be able to see the original bug, which can contain examples
> and motivation for a feature.
> 
> I guess if the idea is to have the stuff in m-c be a mirror of a git repo,
> it doesn't hurt to have a second copy of the tests around for now.

I'll change the test to use the files from the ipdl/ directory instead of duplicating them then, you make a good point.

> I also feel like these files should be under ipc/ipdl/new or something,
> rather than a new separate top level directory under ipc, but maybe that's
> just me. It can get moved later, I suppose.

IMO it's better to keep it as a top level directory because it's meant as a replacement of the other folder. It feels weird to include all that Rust/C++ stuff as a sub-directory of the Python stuff. I guess it's a matter of taste, so I'll leave the final decision to Blake on this one.

> At some point your parser changes should be put into the Rust IPDL parser
> repo, but I can look over the changes and do that myself later.

Yes, that's planned, but the parser is still evolving with the cpp codegen stuff, so I want to wait before doing that (also, it's slightly less of a priority than the work I've been doing up until now).

Thank you for your comments!

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Tristan Bourvon from comment #6)
> IMO it's better to keep it as a top level directory because it's meant as a
> replacement of the other folder. It feels weird to include all that Rust/C++
> stuff as a sub-directory of the Python stuff. I guess it's a matter of
> taste, so I'll leave the final decision to Blake on this one.

I don't feel strongly about this. I actually think a sibling directory of ipdl makes more sense because this is a ground-up replacement and not building on top of the existing code. The hope will be to eventually move ipdl_new into ipdl and nuke the existing stuff (except for the tests and sync-message.ini, etc).

> Yes, that's planned, but the parser is still evolving with the cpp codegen
> stuff, so I want to wait before doing that (also, it's slightly less of a
> priority than the work I've been doing up until now).

How much of an impediment would it be for you to do incremental pull requests on the parser changes? If Andrew is OK waiting until you're done changing it, I won't push for it, but I think it might be good to get incremental feedback and early merges into the tree for changes that might not be perfect but that stand alone (i.e. pass all the tests and don't break things). That'll also hopefully reduce any review time for changes on top of that.

Comment 8

[Tristan Bourvon]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> How much of an impediment would it be for you to do incremental pull
> requests on the parser changes? If Andrew is OK waiting until you're done
> changing it, I won't push for it, but I think it might be good to get
> incremental feedback and early merges into the tree for changes that might
> not be perfect but that stand alone (i.e. pass all the tests and don't break
> things). That'll also hopefully reduce any review time for changes on top of
> that.

It's not that big of a deal, I can do that.

Comment 9

[Phabricator Automation]

Comment on attachment 8991783 [details]
Bug 1475415 - Add first version of IPDL-JS API

Blake Kaplan (:mrbkap) has approved the revision.

https://phabricator.services.mozilla.com/D2116

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8991783 [details]
Bug 1475415 - Add first version of IPDL-JS API

Andrew McCreight [:mccr8] has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58f0722012cd
Add first version of IPDL-JS API r=mrbkap,mccr8

Comment 12

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 58f0722012cd (Bug 1475415) for force-cargo-library-build bustages CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&fromchange=787f9095543f9b60d22301d6c3ef06c4b47b1173&selectedJob=201209163

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201209163&repo=autoland&lineNumber=25911

Comment 13

[Tristan Bourvon]

Nathan, do you have any idea what could cause this LLVM OOM error? It doesn't seem directly related to my patch and it sounds more like a threshold my patch made us hit...

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Tristan Bourvon from comment #13)
> Nathan, do you have any idea what could cause this LLVM OOM error? It
> doesn't seem directly related to my patch and it sounds more like a
> threshold my patch made us hit...

Not sure offhand.  Do you generate extremely large Rust files as part of the build process, or are any of your Rust files unreasonable to compile?  Maybe measure peak memory usage on your local machine, and report a Rust bug if it's excessive?

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/cc77004653fc
Backed out changeset 58f0722012cd for force-cargo-library-build bustages CLOSED TREE

Comment 16

[John Dai[:jdai]]

(In reply to Tristan Bourvon from comment #13)
> Nathan, do you have any idea what could cause this LLVM OOM error? It
> doesn't seem directly related to my patch and it sounds more like a
> threshold my patch made us hit...

See bug 1494907, they're considering disable static analysis with 32-bit windows for TaskCluster.

Comment 17

[John Dai[:jdai]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a49b1f0a63173b800ad53aeb4fb9dffd44626e

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Hey jdai, any updates here? Your try push does indeed look good.

Comment 19

[John Dai[:jdai]]

Thanks for reminding me. I'll send out the patches now.

Comment 20

[John Dai[:jdai]]

Attachment: Bug 1475415 - Part 1: Add IPDL-JS API.

Comment 21

[John Dai[:jdai]]

Attachment: Bug 1475415 - Part 2: IPDL-JS ipc.

Depends on D9682

Comment 22

[John Dai[:jdai]]

Attachment: Bug 1475415 - Part 3: IPDL parser in rust.

Depends on D9683

Comment 23

[John Dai[:jdai]]

Attachment: Bug 1475415 - Part 4: Test IPDL-JS API.

Depends on D9684

Comment 24

[Andrew McCreight [:mccr8]]

Thanks for working on this. How do these patches differ from the patch that Blake already reviewed? If they are mostly the same, only the parts that changed should really require review. You flagged three reviewers on these patches. What are you expecting each of us to review?

Comment 25

[John Dai[:jdai]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> Thanks for working on this. How do these patches differ from the patch that
> Blake already reviewed? If they are mostly the same, only the parts that
> changed should really require review. You flagged three reviewers on these
> patches. What are you expecting each of us to review?

Sorry, I didn't notice that the review flag has been set. I canceled all of Blake and your review requests. I'll provide a diff only for Nika. Thank you.

Comment 26

[Christian Holler (:decoder)]

Nika, can you help us figure out what the status of this bug is? Having an IPDL-like system for JS actors would be great for security and I would like to help unblock this.

Comment 27

[Nika Layzell [:nika] (ni? for response)]

As I mentioned to you on matrix, there are a lot of potential paths forward here depending on what exactly we're looking for from JS actors.

For improving JSWindowActors and validation there, we can perhaps use a schema system for IPC messages in the WindowActorSidedOptions, which specifies what messages are allowed to be sent, and the shape of valid messages, in some way. We could include a legacy opt-out for the current behaviour and slowly transition things over to specifying the types precisely.

Comment 28

[Andrew McCreight [:mccr8]]

I think a big issue here is figuring out exactly what kind of type system you want for these messages. You'd want to talk to people who are actually figuring out what kind of system makes sense for them. I think that given a type system, it wouldn't be too hard to dynamically infer a type for a message, so once something like that was set up we could do a try run and automatically generate specifications for many of the messages. We'd still probably want to look over the results, so there might still be a lot of work, but at least the boiler plate would be taken care of.

Comment 29

[Andrew McCreight [:mccr8]]

This hasn't been actively worked on in years, so I'm going to close it. Hopefully bug 1885221 will make our JS IPC a bit more controlled.