Closed
Bug 1204583
Opened 9 years ago
Closed 9 years ago
Implement runtime.connect for the background and tab ExtensionPage
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox44 fixed)
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: rpl, Assigned: rpl)
Details
Attachments
(3 files, 6 obsolete files)
5.02 KB,
patch
|
Details | Diff | Splinter Review | |
2.32 KB,
patch
|
Details | Diff | Splinter Review | |
5.95 KB,
patch
|
Details | Diff | Splinter Review |
Currently only the content scripts have access to a "browser.runtime.connect" method, it should be available to the other extension page types (e.g. background and tab)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → luca.greco
Assignee | ||
Comment 1•9 years ago
|
||
Comment on attachment 8660831 [details] [diff] [review]
addBrowserRuntimeConnect.patch
This patch implements the connect function for the other ExtensionPage types.
Attachment #8660831 -
Flags: review?(gkrizsanits)
Comment 2•9 years ago
|
||
Comment on attachment 8660831 [details] [diff] [review]
addBrowserRuntimeConnect.patch
Review of attachment 8660831 [details] [diff] [review]:
-----------------------------------------------------------------
I think we should add a test for this before landing it.
::: toolkit/components/extensions/ext-runtime.js
@@ +22,4 @@
>
> onConnect: context.messenger.onConnect("runtime.onConnect"),
>
> + connect: function(extensionId, connectInfo) {
extensionId is optional, please handle the case when it is not given (that should be the common case)
Attachment #8660831 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 3•9 years ago
|
||
In the new version of the attached patch, I've added a small testcase which test the connection originated from a tab and received from the background page.
About the optional extensionId:
In the new runtime.connect method (the one which will be available to the tab and background pages), the extensionId is actually optional (and it is filled with the current extension id if missing, just like in the current content script "runtime.connect" implementation),
nevertheless from a deeper look into the chrome API doc pages, it seems that our current implementation has to be fixed to work as expected:
- https://developer.chrome.com/extensions/messaging#external
- https://developer.chrome.com/extensions/manifest/externally_connectable
Reading the the doc pages (but I'm going to put together a small addon to directly check it), it seems to me that if we specify an extensionId which is different from the current one, the connect message should be received from the runtime.onConnectExternal listeners instead of being receive from the runtime.onConnect, and the addon have to specify that they like to receive messages from the other addons or enabled web pages in the externally_connectable attributes (or explicitly accept any using "*").
Probably we should put this broader change (the implementation of runtime.onConnectExternal and the externally_connectable manifest attribute, then change the runtime.connect accordingly) in a followup, mostly because we need to fix both this new implementation as well as the runtime.connect available in the content scripts (and currently implemented in the ExtensionContent.jsm).
Attachment #8660831 -
Attachment is obsolete: true
Attachment #8664223 -
Flags: review?(gkrizsanits)
Comment 4•9 years ago
|
||
(In reply to Luca Greco from comment #3)
> Created attachment 8664223 [details] [diff] [review]
> bug-1204583-v2.patch
>
> In the new version of the attached patch, I've added a small testcase which
> test the connection originated from a tab and received from the background
> page.
Thanks for the test!
>
> About the optional extensionId:
>
> In the new runtime.connect method (the one which will be available to the
> tab and background pages), the extensionId is actually optional
Is it? It seems to me that if one calls it like |connect(connectInfo)| it will not work.
Sorry, I think I was a bit too vague earlier, what I meant is something like the patch does
under Bug 1202486.
> nevertheless from a deeper look into the chrome API doc pages, it seems that
> our current implementation has to be fixed to work as expected:
Yes, that is a missing feature, and totally is worth to file a bug for this (I think
we don't have it but Bill might have filed something already that covers it I wouldn't know).
But I only wanted to ensure here to make the most common use cases to work.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> >
> > About the optional extensionId:
> >
> > In the new runtime.connect method (the one which will be available to the
> > tab and background pages), the extensionId is actually optional
>
> Is it? It seems to me that if one calls it like |connect(connectInfo)| it
> will not work.
> Sorry, I think I was a bit too vague earlier, what I meant is something like
> the patch does
> under Bug 1202486.
ah!I see now, I wasn't aware of the changes related to Bug 1202486, I'm going to update the patch and the testcase accordingly.
Thanks for the clarification.
Assignee | ||
Comment 6•9 years ago
|
||
I've updated the patch, in this third revision if we pass only one argument to the runtime.connect method it becomes the connectionInfo.
I preferred "arguments.length < 2" to check the parameter number, because if we check only if the connectInfo defined:
connect: function(extensionId, connectInfo) {
if (!connectInfo) {
connectInfo = extensionId;
extensionId = null;
}
...
}
then if we pass two parameters but the connectionInfo is a null object by mistake, it will end to override the extensionId, e.g.:
var connectInfo = null;
...
browser.runtime.connect(extensionId, connectInfo)
how it looks to you?
should we update the Bug 1202486 patch as well?
Attachment #8664223 -
Attachment is obsolete: true
Attachment #8664223 -
Flags: review?(gkrizsanits)
Attachment #8664278 -
Flags: review?(gkrizsanits)
Comment 7•9 years ago
|
||
(In reply to Luca Greco from comment #6)
> I preferred "arguments.length < 2" to check the parameter number, because if
> we check only if the connectInfo defined:
I would like to check the arguments.length too but, instead the idea is that we will do argument checking a bit higher level, something like webidl dictionary and then it will throw for null or any other invalid arguments. We should not bother to add any more arg checking code before that, or at least that was the consensus.
On the other hand your solution is I think broken for the |connect(extensionId)| use case so I would prefer to just go with Bill's version.
Assignee | ||
Comment 8•9 years ago
|
||
I've updated the patch as suggested.
I agree completely, as it is still broken on that use case then it is better to have the same bug on both the implementations and fix both of them in a followup.
Attachment #8664278 -
Attachment is obsolete: true
Attachment #8664278 -
Flags: review?(gkrizsanits)
Attachment #8664309 -
Flags: review?(gkrizsanits)
Comment 9•9 years ago
|
||
Comment on attachment 8664309 [details] [diff] [review]
bug-1204583-v4.patch
Review of attachment 8664309 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry I was too tired and jet-lagged yesterday evening to finish this up. I was thinking about this, maybe the right thing to do here would be to do an additional instanceof/typof check to decide which arg we've got, since both are optional (unlike in the other bug I mentioned). We can fix this in a follow up or you can add the addional check now. Note that some patches were backed out so your patch might be affected, and have to wait...
Attachment #8664309 -
Flags: review?(gkrizsanits) → review+
Comment 10•9 years ago
|
||
In the meanwhile let's ask Bill what he thinks. Bill, do you think in case of two optional args we should do some additional type checking on the args to be able to decide which arg we've got if the function was called with only one arg? Also, what do you think about Luca's proposal about checking argument.length (comment 6) in general?
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 11•9 years ago
|
||
In the attached patch there is a new processRuntimeConnectParams helper which processes and validates the runtime.connect params.
It tests the arguments using typeof (and remove from the arguments list when it's recognized) and raises an exception on invalid cases.
If we think this approach is fine I can move this helper function in one of the shared module, add test units directly on the processRuntimeConnectParams helper and share it between the two runtime.connect implementations.
Flags: needinfo?(gkrizsanits)
Comment on attachment 8664807 [details] [diff] [review]
bug-1204583-preprocess-and-validate-connect-params.patch
Review of attachment 8664807 [details] [diff] [review]:
-----------------------------------------------------------------
I don't think it matters too much what we do here as long as typical extensions work. We know that we need to make a general argument parsing layer (bug 1190677) and this will all be subsumed by that.
Attachment #8664807 -
Flags: review+
Luca, do you need help landing this stuff?
Flags: needinfo?(wmccloskey)
Comment 14•9 years ago
|
||
Clearing needinfo as Bill answered everything I think.
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 15•9 years ago
|
||
bug-1204583-v4.patch rebased
Attachment #8664309 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
rebased bug-1204583-preprocess-and-validate-connect-params.patch
Attachment #8664807 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
I wasn't completely satisfied by the test coverage of the valid/invalid connect params of the new connect params preprocessor helper.
In the attached patch I added a new testcase (in the toolkit mochitests testsuite) which test invalid connect params.
With the above testcase I encountered a bug in the previous patch and introduced a fix (which is in this new attached patch):
- if the Error is not created from the extension's contentWindow.Error prototype, the extension code will not be able to access the object properties (due to security error on accessing a property of a privileged object)
Attachment #8666374 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 18•9 years ago
|
||
Follows the trybuild of the above three patches:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee04f234789b
Attachment #8666374 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Final updates on the last patch:
- tweak commit comment
- rebased last patch on the changes introduced by Bug 1204072
Attachment #8666374 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
I tried to build the rebased patches on try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=25a11d46c9e7
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=06428d60f03e
There are a bunch of known (and unrelated) intermittent tests and the one of the test (unrelated to this change) group takes a lot of time to timeout.
I checked one more time the patches are correctly rebased on a recent mozilla-central tip, so I'm adding the checkin-needed keyword.
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e88120992f73
https://hg.mozilla.org/integration/fx-team/rev/d5c8007ea568
https://hg.mozilla.org/integration/fx-team/rev/a39c7c2300f3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e88120992f73
https://hg.mozilla.org/mozilla-central/rev/d5c8007ea568
https://hg.mozilla.org/mozilla-central/rev/a39c7c2300f3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•