1456786 - Some comments in ProxyMessenger in ExtensionParent.jsm are incorrect

Status: RESOLVED WONTFIX

(WebExtensions :: General, enhancement)

Description

[Rob Wu [:robwu]]

There are three inaccurate comments:
1. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#194-196
2. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#197-198
3. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#270-271

Besides fixing the comments, there is also room for simplifying the code (see below).


1. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#194-196
    // Listen on the parent process message manager because `runtime.connect`
    // and `runtime.sendMessage` requests must be delivered to all frames in an
    // addon process (by the API contract).

This is incorrect. In the past such messages were indeed sent via the process manager, but noawadays all messages are sent via frame message managers, because of this commit: https://hg.mozilla.org/mozilla-central/rev/329102a4c682#l7.9


2. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#197-198
    // And legacy addons are not associated with a frame, so that is another
    // reason for having a parent process manager here.

This comment is incorrect too. Embedded WebExtensions don't support sending messages, only listening.
- Embedded WebExtensions
  * Messenger: https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/LegacyExtensionsUtils.jsm#65-68
  * The Messenger subscribes to messages on cpmm, and the ProxyMessenger will forward messages (originating from extension contexts) to the Embedded WebExtension via the process manager.


The only reason for subscribing to messages on the parent process manager is:
- ProxyScriptContext
  * context.messageManager = cpmm: https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ProxyScriptContext.jsm#361
  * Messenger: https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ProxyScriptContext.jsm#508

All other callers of messenger.sendMessage/connect pass the context's messageManager, which is a frame message manager, not a process message manager. So once the proxy API is removed (bug 1443259), it should be fine to only listen to the global frame message manager.



3. https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/toolkit/components/extensions/ExtensionParent.jsm#270-271
        apiManager.global.tabGetSender) {
      // From ext-tabs.js, undefined on Android.
Android supports the tabs API, so the check for apiManager.global.tabGetSender and comment can be removed, unless we want to support platforms without a tabs API. Probably never, so let's remove the comment?

Comment 1

[Kris Maglione [:kmag]]

I've told you repeatedly that this code is going away. Please stop spending time on it.

Comment 2

[Rob Wu [:robwu]]

The code going away implies that new code has to be written, presumably based on the knowledge of old code.

Misleading comments make that transition more difficult, so I have documented the inaccuracies (which I found while working on other stuff).

I understand that seeing "ProxyMessenger" might have have triggered you, but don't you think that it's worth 5 minutes of writing a sub-ten-line patch to fix the comments?
(if you do, please comment and I'll submit a patch; if you don't, then I'll leave it be since the inaccuracies are documented in this bug now)

Comment 3

[Kris Maglione [:kmag]]

No, I intend to write the new code without reference to the old code.

Comment 4

[Rob Wu [:robwu]]

Where (bugs?) can I read more about your intended changes? I'd like to know what I should avoid.

AFAIK messaging logic is rewritten to support out-of-process iframes, so code related to ProxyMessenger, Messenger, MessageChannel.jsm are going to change significantly.
Internal-only changes are clearly a no-go; correctness improvements are not as clear-cut.

E.g. I'd like to be able to answer whether I can work on bug 1223425 now (or later). This is a functional change, likely independent of the messaging implementation. But depending on the scope of your changes, it might make more sense to (de)prioritize it.

Comment 5

[Kris Maglione [:kmag]]

MessageChannel.jsm is going to be cut out of the loop entirely. Port endpoints are going to get actual IPDL actors.

(In reply to Rob Wu [:robwu] from comment #4)
> E.g. I'd like to be able to answer whether I can work on bug 1223425 now (or
> later). This is a functional change, likely independent of the messaging
> implementation. But depending on the scope of your changes, it might make
> more sense to (de)prioritize it.

As long as it doesn't deal with IPC code at all, that's probably fine.