1633379 - Consider in-process JSProcessActors

Status: RESOLVED FIXED

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

Description

[David Teller [:Yoric] - still alive but not very active]

Apparently we may need in-process JSProcessActor in some cases.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Extract from a discussion on phabricator

nika: ContentChild is not a ProcessActor, we'll need to add a new [data structure] for that.

nika: When we're adding in-process support, we will need to add a brand new protocol called PDOMProcess, which will have the two sides DOMProcessChild and DOMProcessParent. The type ProcessActor will be the base class of this brand-new type.

We can do this in a follow-up, but it's a bit misleading to say that ContentChild is a ProcessActor, and it won't be in the future.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

So, zombie, if you could detail what you're planning to do with this, this would help my wrap my head around the idea of doing an in-process per-process actor :)

Comment 3

[David Teller [:Yoric] - still alive but not very active]

In particular, I'm interested in:

• understanding the expected lifetime of such in-process actors;
• understanding whether the objective is to share actor code between Fission platforms and non-Fission platforms or something else entirely.

Comment 4

[Tomislav Jovanovic :zombie]

I don't have specific requirements, other than how we use Message Managers for which we need Fission-compatible replacement:

Message managers are arranged in a hierarchy, each tab's MM has a .processMessageManager, including in-parent-process tabs.

We use those for "group" or "broadcast" type messages to reduce IPC, and as a security check to confirm all extension's message senders are from the same (expected) process. Additionally, we have some "messaging subjects" whose lifetime isn't tied to a specific window/tab, so they use process-level message managers directly.

The need for in-parent-process JSProcessActors is the same, so that we wouldn't need two separate code paths for each of the subjects (either tied to a JSWindowActor or not) that might live in the parent process.

As for the lifetime, I think the simplest possible solution should be enough. Probably created lazily first time the named in-process actor is accessed, and lives as long as the JS runtime (without the need for willDestroy/didDestroy events).

Comment 5

[Chris Peterson [:cpeterson]]

Fission M6a
P2

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tomislav Jovanovic :zombie from comment #4)

The need for in-parent-process JSProcessActors is the same, so that we wouldn't need two separate code paths for each of the subjects (either tied to a JSWindowActor or not) that might live in the parent process.

Do you have any example?

I still have difficulties wrapping my head around the concept. This would be one child/parent pair per... what?

Comment 7

[Tomislav Jovanovic :zombie]

Do you have any example?

Each extension context is tied to a Window, and needs to participate in extension messaging. Normally, JSWindowActor would be enough for that, but some messages need to target all contexts in a process, so we need a process-level thing to send it once.

So that currently works by sending a message via Services.ppmm from the parent, and subscribing to Services.cpmm on the "receiver" side, which currently also works when Services.cpmm is accesses from the parent process (android runs extension pages in the parent still).

Not sure if that's what you asked for, or want something more specific, like pointing at lines of code (not easy, since we're in the middle of transitioning to JSWindowActors).

I still have difficulties wrapping my head around the concept. This would be one child/parent pair per... what?

One pair per named process actor, just like in any other process.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Thanks, that's exactly what I need. I still don't entirely understand, but I'm getting closer :)

Each extension context is tied to a Window, and needs to participate in extension messaging. Normally, JSWindowActor would be enough for that, but some messages need to target all contexts in a process, so we need a process-level thing to send it once.

I don't understand why it doesn't work with JSWindowActor or how JSProcessActor could help. Is the problem that you need to target all the JSWindowActors (of a given name) in a specific process and only wish to send a single message? Is it something that is not possible with JSWindowActor or just that using the current version of JSWindowActor would make it slow or inconvenient?

I still have difficulties wrapping my head around the concept. This would be one child/parent pair per... what?

One pair per named process actor, just like in any other process.

Let me rephrase: as of bug 1580448, it's one child/parent pair per named actor per child process. What would replace the "per child process" in your case? Just the parent process?

Comment 9

[Tomislav Jovanovic :zombie]

I don't understand why it doesn't work with JSWindowActor or how JSProcessActor could help. Is the problem that you need to target all the JSWindowActors (of a given name) in a specific process and only wish to send a single message? Is it something that is not possible with JSWindowActor or just that using the current version of JSWindowActor would make it slow or inconvenient?

Yes, that example was about performance, instead of sending N copies of the same message to N different JSWindowActors (in the same process), send it once via the ProcessActor and distribute it inside the process.

One pair per named process actor, just like in any other process.

Let me rephrase: as of bug 1580448, it's one child/parent pair per named actor per child process. What would replace the "per child process" in your case? Just the parent process?

Yes. The result after this bug would be simpler description of the situation: "one per named actor per (any) process".

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Ok, I think I'm getting closer to understanding :)

What is not entirely clear to me at that stage is whether the best solution is to modify JSProcessActor or JSWindowActor.

(In reply to Tomislav Jovanovic :zombie from comment #9)

Yes, that example was about performance, instead of sending N copies of the same message to N different JSWindowActors (in the same process), send it once via the ProcessActor and distribute it inside the process.

How would you distribute it inside the process? Don't you still need JSWindowActor at that stage?

Comment 11

[Tomislav Jovanovic :zombie]

Yes, we need JSWindowActor for other uses as well, so this is additional, not replacement.

As an example, you can see a part of those messy requirements in this comment (though slightly outdated now, since we're mid transition):
https://searchfox.org/mozilla-central/rev/0688ffdef2/toolkit/components/extensions/ExtensionParent.jsm#388-396

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tomislav Jovanovic :zombie from comment #11)

Yes, we need JSWindowActor for other uses as well, so this is additional, not replacement.

Let me rephrase the question: what do you do after JSProcessActor has been used to send the message to each process?

If I understand correctly what you wrote above, you still need to send it to each window in the process, right?

Each extension context is tied to a Window

Comment 13

[Tomislav Jovanovic :zombie]

Possibly yes (that code doesn't exist yet), but not all users of messaging (will) have an associated window, so some will directly receive things through the ProcessActor.

This includes our internal classes like the Extension representation in each process, and future Background ServiceWorkers.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

So, for the moment, from what we discussed, I have the impression that the best API for your initial scenario would be

JSWindowActorParent {
  void broadcastAsyncMessage(DOMString messageName, optional any message);
}

which would minimize the number of IPC messages while ensuring distribution to all JSWindowActorChildren.

Would that work for you? If so, I can try and implement this.

Now, for Extension and Background ServiceWorkers, do I understand correctly that:

• they live outside of a window (so JSWindowActor doesn't work for them);
• they live both in the parent process and in child processes (so you do JSProcessActor and in-process JSProcessActor).

Right?

Comment 15

[Tomislav Jovanovic :zombie]

While it might be nice to have a broadcast api, the rules of which window would be targeted by which message are more complicated than that. It might be worth revisiting this idea as a further perf optimization after we have all our patterns identified and implemented, but we don't need that right now.

We've been going back and forth on this, but it seems to me the problem description is (in my mind) much simpler than this discussion. You implemented JSProcessActorParent/JSProcessActorChild with a specific API (mainly sendMessage and receiveMessage) for each of the child processes, we just need another one instance of those two classes to be instantiated for the parent process, with the exact same API (calling sendMessage on one instance would route to receiveMessage on the other, and vice versa).

No new API, from the JS point of view, I would just ask for a child JSProcessActor, and get one regardless if i'm in a child process or parent.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Well, if you're sure it's the best API, I'll try and find how I can implement it :)

Comment 17

[David Teller [:Yoric] - still alive but not very active]

nika, to confirm, what should the DOMProcess{Child, Parent} belong to? Should we have a DOMProcessParent in the ContentParent and a DOMProcessChild in each process (both child and parent)?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

nika, gentle reminder that you mentioned over chat that you wanted to change the description of this bug but I don't know exactly towards what :)

Comment 19

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

Attachment: Bug 1633379 - Part 1: Move PInProcess into dom/ipc,

This moves it near the cross-process PContent actor, and makes it more clear
that this actor is only intended to be used for DOM things.

Comment 20

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

Attachment: Bug 1633379 - Part 2: Add support for in-process JSWindowActors,

This switches the nsIContent{Parent,Child} interface to be
nsIDOMProcess{Parent,Child}, and also implements it on
InProcess{Parent,Child}, along with the ProcessActor interface.

Comment 21

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

I threw together a quick implementation of making this with with in-process JSProcessActors. I haven't tested this a ton but this should work as a template at least.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efef0b59bcd8
Part 1: Move PInProcess into dom/ipc, r=kmag,Yoric
https://hg.mozilla.org/integration/autoland/rev/a26037f3225b
Part 2: Add support for in-process JSWindowActors, r=kmag,Yoric

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4958e4f8247
Backed out 2 changesets for windows build bustages on ContentChild.obj. CLOSED TREE

Comment 24

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=9d89fc475692b20011929c1dc8bb8f5ff8fe8809&selectedTaskRun=VNjlLf2lR8ucWAePG-3enA.0

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

Backout link: https://hg.mozilla.org/integration/autoland/rev/b4958e4f8247

[task 2020-06-25T17:45:31.647Z] 17:45:31     INFO -  /builds/worker/checkouts/gecko/dom/ipc/ContentChild.cpp(4215,45): error: function declared 'stdcall' here was previously declared without calling convention
[task 2020-06-25T17:45:31.647Z] 17:45:31     INFO -  NS_IMETHODIMP_(ContentChild*) ContentChild::AsContentChild() { return this; }
[task 2020-06-25T17:45:31.647Z] 17:45:31     INFO -                                              ^
[task 2020-06-25T17:45:31.647Z] 17:45:31     INFO -  /builds/worker/checkouts/gecko/dom/ipc/ContentChild.h(87,3): note: previous declaration is here
[task 2020-06-25T17:45:31.648Z] 17:45:31     INFO -    NS_DECL_NSIDOMPROCESSCHILD
[task 2020-06-25T17:45:31.648Z] 17:45:31     INFO -    ^
[task 2020-06-25T17:45:31.648Z] 17:45:31     INFO -  /builds/worker/workspace/obj-build/dist/include/nsIDOMProcessChild.h(82,40): note: expanded from macro 'NS_DECL_NSIDOMPROCESSCHILD'
[task 2020-06-25T17:45:31.648Z] 17:45:31     INFO -    virtual mozilla::dom::ContentChild * AsContentChild(void) override;
[task 2020-06-25T17:45:31.648Z] 17:45:31     INFO -                                         ^
[task 2020-06-25T17:45:31.649Z] 17:45:31     INFO -  1 error generated.
[task 2020-06-25T17:45:31.649Z] 17:45:31    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:748: ContentChild.obj] Error 1
[task 2020-06-25T17:45:31.649Z] 17:45:31     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/ipc'
[task 2020-06-25T17:45:31.649Z] 17:45:31     INFO -  make[4]: *** Waiting for unfinished jobs....

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96ddb1b29a80
Part 1: Move PInProcess into dom/ipc, r=kmag,Yoric
https://hg.mozilla.org/integration/autoland/rev/e82376ae2519
Part 2: Add support for in-process JSWindowActors, r=kmag,Yoric

Comment 26

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/96ddb1b29a80
https://hg.mozilla.org/mozilla-central/rev/e82376ae2519

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.