Closed Bug 1580448 (JSProcessActor) Opened 1 year ago Closed 7 months ago

Consider introducing JSProcessActors (JSWindowActor for PContent)

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M6
Tracking Status
firefox77 --- fixed

People

(Reporter: nika, Assigned: Yoric)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

This would be like JSWindowActor, but for content processes, and would eliminate one of the remaining usecases for the old MessageChannel APIs.

Fission Milestone: --- → ?
Depends on: 1580766
No longer depends on: 1580766
Fission Milestone: ? → Future

kmag says this bug should block enabling Fission in Nightly (M6) because bug 1587058 blocks M6.

Fission Milestone: Future → M6

I believe this should be moved ahead to M5.

Unlike other (most of the?) work the Fission team is doing to replace features and primitives with OOPIF-compatible ones, this is introducing a new API that other teams (at least WebExt and DevTools, possibly others) need to build on top of.

Since we're working in parallel toward the same M6 goal, and since several of the things we need to build on top of this api are themselves non-trivial, I don't think it's reasonable for all of that to happen in the same milestone.

New APIs need to "bake" for some time before they could be reliable used. After landing, they need to be documented, learned, tried, used and misunderstood, crashes found and fixed -- which I don't think is possible in the span of a couple of months (as history has shown).

Fission Milestone: M6 → ?

New APIs need to "bake" for some time before they could be reliable used. After landing, they need to be documented, learned, tried, used and misunderstood, crashes found and fixed -- which I don't think is possible in the span of a couple of months (as the history has shown).

*If left in M6, this all needs to happen after designing, writing, reviewing, testing and landing the API itself during the same milestone.

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

I believe this should be moved ahead to M5.

In that case, moving to M5.

Fission Milestone: ? → M5

I can take a look at this. What's the use case, exactly?

Assignee: nobody → dteller
Flags: needinfo?(nika)
Flags: needinfo?(cpeterson)

I'm not sure I understand why this is considered to block M5. My impression was that we weren't planning to add per-process JS actors until after Fission, and had no intention to make them block Fission. The existing per-process MessageManager system should work as-usual with Fission.

:zombie, why does this block the work you're doing?

Flags: needinfo?(nika) → needinfo?(tomica)

Because we need to very thoroughly combine frame-level and process-level messaging in a lot of places in the webextensions framework, and for those two to be using similar yet different APIs with different capabilities and guarantees is gonna be messy, and require keeping around thousands of lines of (old, crufty, special-case) code to deal with message managers.

If there was a change of plans regarding M5, I didn't see that communicated anywhere.

And more generally, I don't understand the point of adding process level actors after fission is "complete". It seems unlikely people are gonna have the opportunity to go back just to "improve" things after they get it working once (code complexity and cruft be damned).

Overall, it seems like a very inefficient way to coordinate our work.

Flags: needinfo?(tomica)

Tracking for Fission Nightly (M6). JSContentActors do not need to block the Fission dogfooding milestone (M5).

Assignee: dteller → nobody
Fission Milestone: M5 → M6
Flags: needinfo?(cpeterson)

JSWindowActors, as currently implemented, are not a complete/finished fission-compatible replacement for all the functionality provided by the family of MessageManagers. Several aspects are still missing, and workarounds using process-level MessageManagers are somewhere between very inefficient and hard to impossible.

A few examples off the top of my head:

  • Ability to send Queries in addition to one way messages
  • Ability to efficiently broadcast the exact same message to the whole BrowsingContext subtree
  • Ability to enumerate, identify and reference content processes from the parent
  • Ability to link which processes a JSWindowActors belongs to, and vice versa
  • A persistent top-level BC "id" (bug 1580766)

Though I'm sure there will be others to discover, since I didn't expect we'll need to make parts of the web extensions framework fission compatible without ProcessActors.

Alias: JSContentActor

I'm working on bug 1316748, which should be least impacted by the above issues, and came across one more potential pitfall:

  • message ordering between JSWindowActors and process level MessageManagers might not be guaranteed in all cases (specifically, for the in-parent documents, according to Nika)

Unfortunately, we still need to support GeckoView, which doesn't have support for out-of-process extensions, so all extensions pages run in the parent process on android.

Assignee: nobody → dteller
Attachment #9129505 - Attachment description: Bug 1580448 - JSContentActor API → Bug 1580448 - JSContentActor API;r?nika
Status: NEW → ASSIGNED
Attachment #9129505 - Attachment description: Bug 1580448 - JSContentActor API;r?nika → Bug 1580448 - JSContentActor API;r?nika!

Could we find a better name than JSContentActor{...}? Semantically, Content and Window are near-synonyms.

Can we rename them JSProcessActor, as someone suggested previously?

Flags: needinfo?(nika)
Attachment #9129505 - Attachment description: Bug 1580448 - JSContentActor API;r?nika! → Bug 1580448 - JSContentActor API;r?nika
Attachment #9138470 - Attachment description: Bug 1580448 - nsIContent{Child, Parent}::getActor;r?nika! → Bug 1580448 - nsIContent{Child, Parent}::getActor;r?nika
Attachment #9138472 - Attachment description: Bug 1580448 - ChromeUtils.{register, unregister}ContentActor;r?nika! → Bug 1580448 - ChromeUtils.{register, unregister}ContentActor;r?nika
Attachment #9129505 - Attachment description: Bug 1580448 - JSContentActor API;r?nika → Bug 1580448 - JSContentActor API;r?nika!
Attachment #9138470 - Attachment description: Bug 1580448 - nsIContent{Child, Parent}::getActor;r?nika → Bug 1580448 - nsIContent{Child, Parent}::getActor;r?nika!
Attachment #9138472 - Attachment description: Bug 1580448 - ChromeUtils.{register, unregister}ContentActor;r?nika → Bug 1580448 - ChromeUtils.{register, unregister}ContentActor;r?nika!

As per the vote, we're renaming them JSProcessActor.

Summary: Consider introducing JSContentActors (JSWindowActor for PContent) → Consider introducing JSProcessActors (JSWindowActor for PContent)
Attachment #9129505 - Attachment description: Bug 1580448 - JSContentActor API;r?nika! → Bug 1580448 - JSProcessActor API;r?nika!
Attachment #9138472 - Attachment description: Bug 1580448 - ChromeUtils.{register, unregister}ContentActor;r?nika! → Bug 1580448 - ChromeUtils.{register, unregister}ProcessActor;r?nika!
Attachment #9138538 - Attachment description: Bug 1580448 - JSContentActor tests;r?nika! → Bug 1580448 - JSProcessActor tests;r?nika!

This is a fixup to Bug 1580447, which landed some time ago.

Depends on D69819

Attachment #9144258 - Attachment description: Bug 1580448 - JSProcessActorParent::AfterDestroy;r?nika! → Bug 1580448 - JSProcessActor{Child, Parent}::AfterDestroy;r?nika!
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/358f764ae48b
Trivial unified build fixups;r=nika
https://hg.mozilla.org/integration/autoland/rev/9b548bd38671
Rename JSWindowActor into JSActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/2a1701831a6a
JSProcessActor API;r=nika
https://hg.mozilla.org/integration/autoland/rev/dafa80c63322
nsIContent{Child, Parent}::getActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/bfbd3330b0a5
ChromeUtils.{register, unregister}ProcessActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/fd7527c86239
Renaming Test{Child, Parent} => TestWindow{Child, Parent};r=nika
https://hg.mozilla.org/integration/autoland/rev/6db8de5fc125
JSProcessActor tests;r=nika
https://hg.mozilla.org/integration/autoland/rev/677257a41457
Add NS_DECL_NSICONTENTPARENT to ContentParent;r=nika
https://hg.mozilla.org/integration/autoland/rev/6b4db1a501df
JSProcessActor{Child, Parent}::AfterDestroy;r=nika

Merge error. Fixed, will reland once I've confirmed that the fix is sufficient.

Flags: needinfo?(dteller)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68a900996016
Trivial unified build fixups;r=nika
https://hg.mozilla.org/integration/autoland/rev/b49b7bbd25b9
Rename JSWindowActor into JSActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/9c7322618b18
JSProcessActor API;r=nika
https://hg.mozilla.org/integration/autoland/rev/08f0ecf2f308
nsIContent{Child, Parent}::getActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/7e0c0eacd079
ChromeUtils.{register, unregister}ProcessActor;r=nika
https://hg.mozilla.org/integration/autoland/rev/4de7ed8efac0
Renaming Test{Child, Parent} => TestWindow{Child, Parent};r=nika
https://hg.mozilla.org/integration/autoland/rev/2ba000294288
JSProcessActor tests;r=nika
https://hg.mozilla.org/integration/autoland/rev/e62bfdb50e18
Add NS_DECL_NSICONTENTPARENT to ContentParent;r=nika
https://hg.mozilla.org/integration/autoland/rev/c51bedeb809f
JSProcessActor{Child, Parent}::AfterDestroy;r=nika

David, are you going to document that at https://firefox-source-docs.mozilla.org/dom/Fission.html?

Flags: needinfo?(dteller)

Filed as bug 1635061. I think I'll wait a few days until I have a better idea of where bug 1633379 is heading, though.

Regressions: 1635024
Alias: JSContentActor → JSProcessActor
Flags: needinfo?(nika)
Flags: needinfo?(dteller)
You need to log in before you can comment on or make changes to this bug.