Status

()

enhancement
P5
normal
Last year
23 days ago

People

(Reporter: tristanbourvon, Unassigned)

Tracking

(Blocks 6 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Reporter

Description

Last year
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.
Reporter

Comment 1

Last year
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.
Reporter

Comment 2

Last year
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.
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. :)
Reporter

Comment 4

11 months ago
(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?
(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.
Assignee: nobody → tristanbourvon
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 6

11 months ago
(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!
Flags: needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap)
Reporter

Comment 8

11 months ago
(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 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
Attachment #8991783 - Flags: review+
Comment on attachment 8991783 [details]
Bug 1475415 - Add first version of IPDL-JS API

Andrew McCreight [:mccr8] has approved the revision.
Attachment #8991783 - Flags: review+

Comment 11

9 months ago
Pushed by tristanbourvon@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58f0722012cd
Add first version of IPDL-JS API r=mrbkap,mccr8
Reporter

Comment 13

9 months ago
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...
Flags: needinfo?(tristanbourvon) → needinfo?(nfroyd)
(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?
Flags: needinfo?(nfroyd)

Comment 15

9 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/cc77004653fc
Backed out changeset 58f0722012cd for force-cargo-library-build bustages CLOSED TREE

Comment 16

9 months ago
(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.
Hey jdai, any updates here? Your try push does indeed look good.
Flags: needinfo?(jdai)

Comment 19

8 months ago
Thanks for reminding me. I'll send out the patches now.
Flags: needinfo?(jdai)

Comment 21

8 months ago
Depends on D9682

Comment 22

8 months ago
Depends on D9683

Comment 23

8 months ago
Depends on D9684
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?
Flags: needinfo?(jdai)

Comment 25

8 months ago
(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.
Flags: needinfo?(jdai)

Updated

8 months ago
Blocks: 1502960

Updated

8 months ago
Blocks: 1503285

Updated

8 months ago
Blocks: 1503290

Updated

8 months ago
Blocks: 1503291

Updated

8 months ago
Blocks: 1503513
No longer blocks: 1467212
Depends on: 1467212

Updated

3 months ago
Priority: -- → P1

Updated

23 days ago
Assignee: jdai → nobody
Mentor: mrbkap
Priority: P1 → P5
You need to log in before you can comment on or make changes to this bug.