onMessage and onDisconnect callbacks should receive their Port objects as arguments

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: kmag)

Tracking

unspecified
mozilla51
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50- fixed, firefox51+ fixed)

Details

(Whiteboard: [nativeMessaging])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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 ?
Flags: needinfo?(amckay)
(Reporter)

Comment 1

2 years ago
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.
status-firefox50: --- → unaffected
status-firefox51: --- → affected
tracking-firefox51: --- → ?

Comment 2

2 years ago
Fwd'd to aswan on this as well, who is out this week. Something to pick up on his return.
Flags: needinfo?(aswan)
Tracking 51+ for this feature.
tracking-firefox51: ? → +
(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.
Flags: needinfo?(aswan)
OK, I understand the problem now. It should be simple enough to fix, and we should probably uplift to 50.
Assignee: nobody → kmaglione+bmo
status-firefox50: unaffected → affected
tracking-firefox50: --- → ?
Priority: -- → P1

Comment 6

2 years ago
Looks like Kris has got this one.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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.
Attachment #8786076 - Flags: review?(lgreco) → review+
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
Attachment #8786076 - Flags: approval-mozilla-aurora?
Summary: native messaging: OnMessage and OnDisconnect have null "nativePort" - different from Chrome → onMessage and onDisconnect callbacks should receive their Port objects as arguments
Whiteboard: [nativeMessaging]

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d981eb1bef3c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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+
Attachment #8786076 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
tracking-firefox50: ? → -
Needs rebasing for Aurora.
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/releases/mozilla-aurora/rev/fde69933d9cc
status-firefox50: affected → fixed
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.