1204583 - Implement runtime.connect for the background and tab ExtensionPage

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

Attachment: addBrowserRuntimeConnect.patch

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)

Comment 1

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

Comment on attachment 8660831 [details] [diff] [review]
addBrowserRuntimeConnect.patch

This patch implements the connect function for the other ExtensionPage types.

Comment 2

[Gabor Krizsanits (INACTIVE)]

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)

Comment 3

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

Attachment: 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.

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).

Comment 4

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 5

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

(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.

Comment 6

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

Attachment: bug-1204583-v3.patch

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?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 8

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

Attachment: bug-1204583-v4.patch

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.

Comment 9

[Gabor Krizsanits (INACTIVE)]

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...

Comment 10

[Gabor Krizsanits (INACTIVE)]

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?

Comment 11

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

Attachment: bug-1204583-preprocess-and-validate-connect-params.patch

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.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Luca, do you need help landing this stuff?

Comment 14

[Gabor Krizsanits (INACTIVE)]

Clearing needinfo as Bill answered everything I think.

Comment 15

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

Attachment: bug-1204583-v4.patch

bug-1204583-v4.patch rebased

Comment 16

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

Attachment: bug-1204583-preprocess-and-validate-connect-params.patch

rebased bug-1204583-preprocess-and-validate-connect-params.patch

Comment 17

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

Attachment: bug-1204583-test-connect-params-preprocess-and-fix-r.patch

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)

Comment 18

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

Follows the trybuild of the above three patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee04f234789b

Comment 19

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

Attachment: bug-1204583-test-connect-params-preprocess-and-fix-r.patch

Final updates on the last patch:
- tweak commit comment
- rebased last patch on the changes introduced by Bug 1204072

Comment 20

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

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.

Comment 21

[Pulsebot]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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