API exposed to content script should be in content principal

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 589198 [details]
Pull request 328
Assignee: nobody → poirot.alex

Updated

7 years ago
Priority: -- → P1
(Assignee)

Comment 2

7 years ago
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
Attachment #589198 - Flags: feedback?(rFobic)
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.
Attachment #589198 - Flags: feedback?(rFobic) → feedback-
> 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.
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...
(Assignee)

Comment 6

7 years ago
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!
(Assignee)

Updated

7 years ago
Blocks: 727854
(Assignee)

Updated

7 years ago
Attachment #589198 - Flags: review?(rFobic)
Comment on attachment 589198 [details]
Pull request 328

With few nits.
Attachment #589198 - Flags: review?(rFobic) → review+
(Assignee)

Comment 8

7 years ago
Landed:
https://github.com/mozilla/addon-sdk/commit/cb341ff39e7a605dbe7a0dcddfce3af07deb7688
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 709966

Updated

6 years ago
Depends on: 752775
You need to log in before you can comment on or make changes to this bug.