718666 - API exposed to content script should be in content principal

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Followup for bug 714110 and bug 679363.
Currently, all content script APIs are defined in worker.js module, which have system principal. So that `self`, `postMessage`, `self.port.*` are COW!

In previous bugs we faced two issues:
- For security reason, `apply` throws an exception when being used on functions (One specific hotfix has been made in bug 679363 for self.postMessage),
- a regression has been introduced in Firefox 10 that break Arrays when being exposed as COW (bug 714110)

In order to avoid facing such issues, we should evaluate these APIs in the same principal than the content script. So in order to do that, I share the same sandbox than the one used for content proxy and implement all APIs exposed to the content script in this sandbox by using only two methods to communicate between both chrome and sandbox: One method to send events to the content from the chrome and another one to do the opposite.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 328

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 589198 [details]
Pull request 328

I'd like your feedback before asking for review.
I'm not completely satisfied by this patch, because of context-menu.
It requires some synchronous access to the content and it goes against one purpose of this patch. Do you think that I can refactor this part of context menu, so that content script access can be always asynchronous ? I've never looked into this module ...

https://github.com/mozilla/addon-sdk/pull/328/files#diff-0

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 589198 [details]
Pull request 328

I like overall direction. I'm concerned with:

1. That we introduce yet another global to the content-script scope.
2. That we implement event emitter in yet another place.

There was this thing pooping up in my mind for a while already, which may be relevant. What if we just used `window.postMessage` window.addEventListener('message', ... in order to implement all jetpack APIs.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> I'm not completely satisfied by this patch, because of context-menu.
> It requires some synchronous access to the content and it goes against one
> purpose of this patch. Do you think that I can refactor this part of context
> menu, so that content script access can be always asynchronous ? I've never
> looked into this module ...

So you meant "synchronous" instead. This is kind of goes against E10S, which is unfortunate. On the other hand though E10S is suspended and it will probably help fixing a bug 714181.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Is it possible to put a rush on this fix?  Bug 714891's temporary fix ends up creating a new XPConnect-wrapped JSCompartment for *every* call to ensureArgumentsAreJSON (when a window is provided).  Because the compartments get surfaced through XPConnect, it takes a while for them to get garbage collected.  I can easily see 10 live compartments with a 1Hz message to my content page...

Comment 6

[Alexandre Poirot [:ochameau]]

Sure. I'll try to make it for 1.6 release. It has to land 02/21.
As it already is a P1, I can dedicate time to this easily.

Thanks for watching SDK bottlenecks, and do not hesitate to points out our main perf/memory issues again!

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 589198 [details]
Pull request 328

With few nits.

Comment 8

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/cb341ff39e7a605dbe7a0dcddfce3af07deb7688