1600612 - Stop using message manager to communicate between FrameProxy and FrameTarget for remote frames

Status: RESOLVED DUPLICATE of bug 1618691

(DevTools :: Framework, task, P1)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1565200. We will start using JsWindowActors to communication cross process for remote frames. However as pointed out by :ochameau during the review, the FrameTargetActor still expects a MessageManager as constructor argument:

    // Create the actual target actor.
    const { messageManager } = this.docShell;
    const targetActor = new FrameTargetActor(connection, messageManager);

Looking at the FrameTargetActor "constructor" (rather its initialize method here), the argument is referred to as "chromeGlobal".

/**
 * Target actor for a frame / docShell in the content process.
 *
 * @param connection DebuggerServerConnection
 *        The conection to the client.
 * @param chromeGlobal
 *        The content script global holding |content| and |docShell| properties for a tab.
 * @param prefix
 *        the prefix used in protocol to create IDs for each actor.
 *        Used as ID identifying this particular target actor from the parent process.
 */
frameTargetPrototype.initialize = function(connection, chromeGlobal) {
  this._chromeGlobal = chromeGlobal;

And looking at the other call site for new FrameTargetActor(...), the value of the argument is indeed the global for the devtools/server/startup/frame.js framescript:

var chromeGlobal = this;

// ...

const {
  FrameTargetActor,
} = require("devtools/server/actors/targets/frame");
actor = new FrameTargetActor(conn, chromeGlobal);

This frame script is loaded via mm.loadFrameScript in frame-connector.js

    let mm = frame.messageManager || frame.frameLoader.messageManager;
    mm.loadFrameScript("resource://devtools/server/startup/frame.js", false);

https://searchfox.org/mozilla-central/rev/073b138dcba41cd3f858522e5f0a9ee73e39afa0/devtools/server/connectors/frame-connector.js#45

And the global of a frame script is a ContentFrameMessageManager. So this chromeGlobal is indeed always a MessageManager (we could rename it, but I rather suggest we get rid of it).

The goal of this bug is to cleanup the FrameTargetActor constructor so that it no longer depends on a message manager. If we look at the current usage of chromeGlobal in FrameTargetActor:

• passed to BrowsingContextTargetActor::initialize
• used to add/removeMessageListener (for FrameTargetActor's update)
• used to get the DocShell

For the first item we can ignore it, since BrowsingContextTargetActor::initialize actually doesn't use this argument.

For add/removeMessageListener, it could be handled by the connection, using transparently MessageManager or jsWindowActor depending on what is available. But that would mean yet another cross process communication on the server. I think the only call site for this is in BrowserTabList.prototype._getActorForBrowser, which is used when listing or getting tabs. From a client perspective this means rootFront's listTabs or getTab.

listTabs is only used by about:debugging, and it should switch to another API that watches for descriptors (something similar to target-list but that would return tab/frame descriptors). And getTab is used in various places, but by nature since it's only focused on one tab, it doesn't seem a problem if the client had to do both a request to a global actor and then to a target actor. In both cases, it seems like this code might change significantly once we fully use descriptors for tab targets.

For the docShell getter, maybe it could be explicitly passed as a constructor argument? I am not clear on the lifecycle of docShell for a given ContentFrameMessageManager though.

I really think we should only revisit this once Bug 1579042 (introduce descriptors for tabs) has been implemented.

Comment 1

[Julian Descottes [:jdescottes]]

I think we'll need to fix this sooner rather than later, at least to stop getting the docShell from the message manager. I am hitting weird failures with this.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1600612 - Create remote FrameTargetActor with the correct docShell

Comment 3

[Phabricator Automation]

Comment on attachment 9116812 [details]
Bug 1600612 - Create remote FrameTargetActor with the correct docShell

Revision D57684 was moved to bug 1605027. Setting attachment 9116812 [details] to obsolete.

Comment 4

[Julian Descottes [:jdescottes]]

(since the attached patch only fixes part of the described issue, moving this to a separate bug)

Comment 5

[Chris Peterson [:cpeterson]]

Tracking DevTools bugs for Fission Nightly (M6) milestone

Comment 6

[Julian Descottes [:jdescottes]]

Not working on this until TabDescriptors have landed. Split the landable parts to bug 1605027 (already landed)

Comment 7

[Julian Descottes [:jdescottes]]

So far, my patch in Bug 1618691 is removing all usage of the message manager in the FrameTargetActor.

Comment 8

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 9

[Julian Descottes [:jdescottes]]

Confirming that Bug 1618691 removed all usage of message manager in Frame Target Actor .

Comment 10

[Chris Peterson [:cpeterson]]

Clearing Fission Milestone for bugs resolved as duplicates. We don't need to track duplicates.