Closed Bug 1204583 Opened 4 years ago Closed 4 years ago

Implement runtime.connect for the background and tab ExtensionPage

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: rpl, Assigned: rpl)

Details

Attachments

(3 files, 6 obsolete files)

Attached patch addBrowserRuntimeConnect.patch (obsolete) — 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: nobody → luca.greco
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 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)
Attached patch bug-1204583-v2.patch (obsolete) — Splinter Review
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)
(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.
(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.
Attached patch bug-1204583-v3.patch (obsolete) — Splinter Review
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)
(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.
Attached patch bug-1204583-v4.patch (obsolete) — Splinter Review
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 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+
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)
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)
Clearing needinfo as Bill answered everything I think.
Flags: needinfo?(gkrizsanits)
bug-1204583-v4.patch rebased
Attachment #8664309 - Attachment is obsolete: true
rebased bug-1204583-preprocess-and-validate-connect-params.patch
Attachment #8664807 - Attachment is obsolete: true
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)
Follows the trybuild of the above three patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee04f234789b
Attachment #8666374 - Flags: review?(wmccloskey) → review+
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
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.