1298810 - onMessage and onDisconnect callbacks should receive their Port objects as arguments

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Benjamin Smedberg]

Bug report from a partner who is transitioning away from NPAPI:

As we code with extension APIs, it works fine but we found there are some differences between Chrome extension APIs & Firefox extension APIs:
At OnMessage or OnDisconnect, we expert parameter nativePort has values same as Chrome, but at Firefox it is NULL.  It is very important for us, we use it to identify which secession it call back, for Chrome we can support every page has it own communicates  with native session, but for Firefox, especially at OnDisconnect, for parameter nativePort is NULL, our codes can't distinguish which session closed. 
And now, we can only support single session now, it is limit and user experience not like Chrome. 
Could you support them like Chrome?  Could you give us some advices about how to support multi sessions between extension and native ?

Comment 1

[Benjamin Smedberg]

I'm not sure about the severity of this yet, but I'm requesting tracking because this is a defect in a new feature we're planning on shipping in 51 and I don't want it to get lost. I have sample code from the partner but I don't know if I have permission to post it publicly yet: Mozilla employees ping me if you want it.

Comment 2

[Kev Needham [:kev]]

Fwd'd to aswan on this as well, who is out this week. Something to pick up on his return.

Comment 3

[Marcia Knous [:marcia]]

Tracking 51+ for this feature.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #0)
> At OnMessage or OnDisconnect, we expert parameter nativePort has values same
> as Chrome, but at Firefox it is NULL.

This isn't documented. Can you please explain its purpose and its contents?

> It is very important for us, we use it to identify which secession it call
> back, for Chrome we can support every page has it own communicates  with
> native session, but for Firefox, especially at OnDisconnect, for parameter
> nativePort is NULL, our codes can't distinguish which session closed. 

Each port has its own onDisconect listeners, so the listener should be able to
tell which port it's attached to.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> I have sample code from the partner but I don't know if I have permission to
> post it publicly yet: Mozilla employees ping me if you want it.

Please email me a copy.

Thanks.

Comment 5

[Kris Maglione [:kmag]]

OK, I understand the problem now. It should be simple enough to fix, and we should probably uplift to 50.

Comment 6

[Andy McKay]

Looks like Kris has got this one.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1298810: Pass Port object to listeners on native messaging ports.

Review commit: https://reviewboard.mozilla.org/r/75036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75036/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8786076 [details]
Bug 1298810: Pass Port object to listeners on native messaging ports.

https://reviewboard.mozilla.org/r/75036/#review73118

r+ with (minor) comments.

::: toolkit/components/extensions/NativeMessaging.jsm:396
(Diff revision 1)
>          this.send(msg);
>        },
>  
>        onDisconnect: new ExtensionUtils.SingletonEventManager(this.context, "native.onDisconnect", fire => {
>          let listener = what => {
> -          this.context.runSafe(fire);
> +          this.context.runSafeWithoutClone(fire, port);

It looks like here (and on line 407) we needed to change `runSafe` to `runSafeWithoutClone` because we need to clone the `port` object with the `cloneFunction` option enabled, how about adding a small inline comment which states this explicitly?

e.g. // The port object has been already cloned manually...

::: toolkit/components/extensions/NativeMessaging.jsm:416
(Diff revision 1)
>            this.off("message", listener);
>          };
>        }).api(),
>      };
>  
> -    return Cu.cloneInto(api, this.context.cloneScope, {cloneFunctions: true});
> +    port = Cu.cloneInto(port, this.context.cloneScope, {cloneFunctions: true});

and/or we can add an inline doc here to explicitly state that we are cloning the `port` object with the `cloneFunctions` option to pass it  into the onDisconnect/onMessage native messaging events as an object with a set of API methods.

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d981eb1bef3c5815a269a1402b13083a07fcee28
Bug 1298810: Pass Port object to listeners on native messaging ports. r=rpl

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8786076 [details]
Bug 1298810: Pass Port object to listeners on native messaging ports.

Approval Request Comment
[Feature/regressing bug #]: Bug 1190682
[User impact if declined]: This bug is a compatibility issue for extensions ported from Chrome, and could prevent those extensions from working as expected in Firefox.
[Describe test coverage new/current, TreeHerder]: The affected methods are thoroughly tested, and this patch adds additional tests for the change.
[Risks and why]: Low. The patch adds an additional argument to two callback functions, but does not otherwise change behavior in a way that should affect existing code. Code that was written to run in Chrome will already have been exposed to this behavior.
[String/UUID change made/needed]: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d981eb1bef3c

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8786076 [details]
Bug 1298810: Pass Port object to listeners on native messaging ports.

Ensures extensions ported over from Chrome run as expected, Aurora50+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fde69933d9cc