Last Comment Bug 612726 - bypass content scripts for an addon's own local content
: bypass content scripts for an addon's own local content
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 enhancement with 1 vote (vote)
: ---
Assigned To: Alexandre Poirot [:ochameau]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-16 15:45 PST by Myk Melez [:myk] [@mykmelez]
Modified: 2012-02-07 12:56 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP - inject `self`in all resource: documents and deprecate contentScript(File) (2.91 KB, patch)
2011-06-20 07:22 PDT, Alexandre Poirot [:ochameau]
myk: feedback+
Details | Diff | Splinter Review
Keep contentScript(File) up and running (3.90 KB, patch)
2011-11-21 08:10 PST, Alexandre Poirot [:ochameau]
myk: review-
Details | Diff | Splinter Review
Pull request 277 (165 bytes, text/html)
2011-11-29 08:59 PST, Alexandre Poirot [:ochameau]
no flags Details

Description Myk Melez [:myk] [@mykmelez] 2010-11-16 15:45:55 PST
When an addon's own local content is loaded into a page (panel, widget, page-worker, etc.), the content is just as trustworthy to the addon's main code as a script loaded as a content script that mediates interaction between the content and the main code.

So it is unnecessary to require addons to use a content script to mediate such interaction, and we should allow addons to communicate directly with such pages, injecting the same WebWorker-like API directly into the page that we currently inject into the content script for the page.

For pages into which something other than an addon's own local content is loaded, on the other hand, content scripts remain useful to mediate interaction between the untrusted content and the addon's main code, and we should continue to require them.
Comment 1 Alexandre Poirot [:ochameau] 2011-03-24 07:41:19 PDT
Definitively an important feature!
For example:
I believe that first developers thought would be that postMessage is going to work from Panel or Widget js scripts. But, for now, postMessage is the standard one that require two arguments.
And this situation is not so easy to understand for new comers.
Comment 2 Myk Melez [:myk] [@mykmelez] 2011-06-15 12:54:54 PDT
(automatic reprioritization of 1.0 bugs)
Comment 3 Alexandre Poirot [:ochameau] 2011-06-20 07:22:14 PDT
Created attachment 540456 [details] [diff] [review]
WIP - inject `self`in all resource: documents and deprecate contentScript(File)

This is one of many way to implement such feature. Here is some questions to debate:
1/ How to enable this injection?
- Here I just matched all resource:* documents, we may match others like chrome, data ? And/or we may restrict to a subset of resource: URLs.
- We can use a flag instead ?

2/ How to inject `self`?
Here I've replaced self by the Worker's one, but developers may expect to be the same than before but only with new jetpack stuff added?
(It is not that simple to do as window.self === self in all cases, isn't ?)

3/ Do we deprecate contentScript/contentScriptFile when we enable injection?
Here, I do. It simplify implementation of such feature, but is there any usecase of injection + contentScripts?
Comment 4 Wes Kocher (:KWierso) 2011-09-08 12:09:48 PDT
(Pushing all open bugs to the --- milestone for the new triage system)
Comment 5 Myk Melez [:myk] [@mykmelez] 2011-09-08 16:37:26 PDT
Comment on attachment 540456 [details] [diff] [review]
WIP - inject `self`in all resource: documents and deprecate contentScript(File)

(In reply to Alexandre Poirot (:ochameau) from comment #3)

> This is one of many way to implement such feature.

Seems reasonable!


> Here is some questions to
> debate:
> 1/ How to enable this injection?
> - Here I just matched all resource:* documents, we may match others like
> chrome, data ? And/or we may restrict to a subset of resource: URLs.
> - We can use a flag instead ?

Just resource: is fine for now.  I can't think of a reason why we would *have* to restrict this behavior to a subset of resource: URLs, but it seems reasonable to start by restricting it to the subset of resource: URLs that load pages in the data/ directory, since that is the use case we intend to support.  We can always expand the set later if folks bring up valid use cases for doing this with resource: URLs that point to other resources.


> 2/ How to inject `self`?
> Here I've replaced self by the Worker's one, but developers may expect to be
> the same than before but only with new jetpack stuff added?
> (It is not that simple to do as window.self === self in all cases, isn't ?)

The third option is to call it something else in this case, like "addon".


> 3/ Do we deprecate contentScript/contentScriptFile when we enable injection?
> Here, I do. It simplify implementation of such feature, but is there any
> usecase of injection + contentScripts?

I know of no such use case, so deprecation seems reasonable, although we'll still need to support the existing API for a while, as it might not be trivial for existing consumers to update their code.
Comment 6 Wes Kocher (:KWierso) 2011-10-06 12:10:17 PDT
I'm adding this to bug 692539 which is a tracking bug for making contentScripts more consistent.

This bug doesn't quite fit the description, but I figure it's close enough. Feel free to break the dependency if you really disagree with this.
Comment 7 Myk Melez [:myk] [@mykmelez] 2011-10-06 13:42:36 PDT
Let's keep bug 692539 focused on the set of bugs for which we originally decided to file a meta bug, namely bugs on the various ways in which objects and their properties behave differently in content scripts than they do in web content because of imperfections in our content object wrappers or limitations of the content script model.  This bug, while certainly important to fix (P1 important, in fact!), doesn't quite fit into that bucket.
Comment 8 Alexandre Poirot [:ochameau] 2011-11-21 08:10:50 PST
Created attachment 575865 [details] [diff] [review]
Keep contentScript(File) up and running

Let's try to be concrete now, and try to land this for 1.4 release.
This patch seems inversely proportional to its benefits!

So I renamed `self` to `addon`, only when we inject in document.
I let `self` in regular content scripts.
I inject `addon` only when:
 - the document is from one of our addon folders. It is not really easy to ensure safely that it comes from one data folder of our addon. So I simply check if the URL starts with "uriPrefix". (It is not 100% correct, we can enable this behavior with an url starting with uriPrefix that is not a data folder. But I do not want to build a complex code just for that. Hopefully, bug 660629 should help us simplifying such test.)
 - no content scripts are defined. i. e. nor contentScript, nor contentScriptFile attributes are set to any non-empty values

So new addons that want to use `addon` will have to omit setting a content script, whereas old addons are still going to work with a content script.
Does that sounds ok?

I've explicitely avoided doing any documentation modification as it seems to require various changes in order to suggest using this simplier pattern for Panel, Widget and Page-worker! I'd like to ensure the final behavior before filling a documentation bug.
Comment 9 Myk Melez [:myk] [@mykmelez] 2011-11-23 15:24:03 PST
Comment on attachment 575865 [details] [diff] [review]
Keep contentScript(File) up and running

Thanks for picking this back up!  Yes, let's get this landed and into our users' hands.  I'm fine with landing the implementation first and the documentation later, but let's make label it experimental when we land the initial documentation for it, so we can experiment with it and make changes as appropriate before we start supporting it and guaranteeing it won't change.

As far as I can tell, the code looks good.  However, the tests are timing out against the latest nightly build:

error: TEST FAILED: test-content-symbiont.test:`addon` is not available when a content script is set (timed out)
.......error: TEST FAILED: test-content-symbiont.test:direct communication with trusted document (timed out)


Also, I think we should call the accessor object "program", because it is a more generic reference (like "self") to "the thing that created this content script", which might be an addon's main script but could also be a third-party module, a core SDK module, or even a module built into Firefox and accessed by Firefox code to implement core features (in the future, when we land some modules in Firefox and the Firefox core codebase uses them to implement features).
Comment 10 Alexandre Poirot [:ochameau] 2011-11-29 08:54:00 PST
(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> Comment on attachment 575865 [details] [diff] [review] [diff] [details] [review]
> Keep contentScript(File) up and running
>
> Also, I think we should call the accessor object "program", because it is a
> more generic reference (like "self") to "the thing that created this content
> script", which might be an addon's main _script_ but could also be a
> third-party _module_, a core SDK _module_, or even a _module_ built into Firefox
> and accessed by Firefox code to implement core features (in the future, when
> we land some modules in Firefox and the Firefox core codebase uses them to
> implement features).

By reading your description, it seems that `module` comes in every example you gave. Wouldn't it be a better name ? I don't see why shipping in Firefox would change the way jetpack is defined. It will always be about addons, even if we ship some features by using jetpack, it will still be addons ? Even if we ship Widget API in Firefox, main.js will still be an addon. Then if we ship main.js in firefox, this main.js will still be somekind of addon, but shipped by default and always enabled.
Comment 11 Alexandre Poirot [:ochameau] 2011-11-29 08:59:38 PST
Created attachment 577643 [details]
Pull request 277

It was failing because I forgot, again, to commit a new file!
Sorry about that.

Here is a pull request for the same patch, with new file.
I'd like to discuss, or at least highlight this global naming choice during today's meeting.
Comment 12 Connor Dunn 2011-11-29 09:02:11 PST
Just a quick comment on the naming, the fact that there currently is the self object in content scripts and the self module is already highly confusing. Adding a new reference to the self object with a different name seems to be pushing it a bit far.

Has any thought been given to either renaming the self object to {addon, program, module} (take your pick) everywhere, or naming the accessor object on local pages something that implies it links to self, i.e. addonSelf?
Comment 13 Wes Kocher (:KWierso) 2011-11-29 11:25:09 PST
To continue bikeshedding a bit more, why not just give them the "port" object directly? (So the extension just uses port.postMessage(), port.emit(), and port.on())
Comment 14 Myk Melez [:myk] [@mykmelez] 2011-11-29 11:31:39 PST
We used to give these scopes postMessage directly, and that caused some trouble (don't remember what it was, though), so we switched to namespacing it in `self` around the time we added `self.port`.  I'm not sure if `port.*` is adequately isolated, but it might be.
Comment 15 Alexandre Poirot [:ochameau] 2011-11-29 13:23:34 PST
We've added `port` in order to segregate events of the creator API from events emitted by the developer. I think it is only for context-menu API, with "click" event.
So if a content script do:
  self.on("click", function () {});
It will receive Context-menu API events only, not developer's one, like this:
  contextMenu.emit("click");

It would be really nice to simplify this story by avoiding a sub `port` attribute and access it directly. It makes sense to name this global by its usage. But then, if we keep the `sub` port attribute, we will have a duplicate name:
  Port.port
  Communicator.port
  Messages.port

Do you think that it makes sense to mix API events with developer ones, with proper documentation, in order to simplify our API?
Otherwise, `jetpack` may be a good fallback. I received negative feedback about `addon` and mitigated about `jetpack`.

Then, I'm wondering if it is reasonable to land one name now with experimental documentation, gather feedback during betas or may be during 1.4 release and then come up with a final name, which can be different ?
Comment 16 [github robot] 2011-11-29 13:47:23 PST
Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d3c84d55185608ebe674af65912ec627b24f6d97
Merge pull request #277 from ochameau/bypass-contentscript

fix bug 612726 - bypass content scripts for an addon's own local content - r=@mykmelez
Comment 17 Myk Melez [:myk] [@mykmelez] 2011-11-29 13:51:31 PST
(In reply to Alexandre Poirot (:ochameau) from comment #15)

> Do you think that it makes sense to mix API events with developer ones, with
> proper documentation, in order to simplify our API?

No, I think it's better to isolate the APIs built-in events from developer-defined ones, as we currently do.


> Otherwise, `jetpack` may be a good fallback. I received negative feedback
> about `addon` and mitigated about `jetpack`.

Indeed.  Both `addon` and `jetpack` are reasonable candidates.  They both have strengths and weaknesses.


> Then, I'm wondering if it is reasonable to land one name now with
> experimental documentation, gather feedback during betas or may be during
> 1.4 release and then come up with a final name, which can be different ?

It's reasonable to do so, although rapid-enough adoption would make it unpalatable.
Comment 18 Will Bamberg [:wbamberg] 2011-12-12 12:53:21 PST
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> Comment on attachment 540456 [details] [diff] [review]
> WIP - inject `self`in all resource: documents and deprecate
> contentScript(File)
> 
> (In reply to Alexandre Poirot (:ochameau) from comment #3)
> 
> > 3/ Do we deprecate contentScript/contentScriptFile when we enable injection?
> > Here, I do. It simplify implementation of such feature, but is there any
> > usecase of injection + contentScripts?
> 
> I know of no such use case, so deprecation seems reasonable, although we'll
> still need to support the existing API for a while, as it might not be
> trivial for existing consumers to update their code.

One possible use case is this: that you can currently specify a widget's content just using an icon:

require("widget").Widget({
  id: "my-widget",
  label: "My Widget",
  contentURL: "http://www.me.org/my-icon.ico"
});

...so there's no HTML you can use to include a script. Currently you can pass a content script in here, and this might be useful where you only want to display an icon, but, for example, to distinguish left and right clicks.

Should we also deprecate this, and say that if you want to run a script in your widget, then you must specify its content as HTML?
Comment 19 Alexandre Poirot [:ochameau] 2011-12-12 13:18:09 PST
Actually, the patch that landed don't deprecate anything.
We may have to tune the current behavior by displaying some deprecation messages, but we currently don't.
So here is how it works: if you want to have access to `addon` global, you have to omit setting contentScript and contentScriptFile properties.

Btw, do you have some thoughts about this global naming ?
Comment 20 Will Bamberg [:wbamberg] 2011-12-12 13:42:56 PST
(In reply to Alexandre Poirot (:ochameau) from comment #19)
> Actually, the patch that landed don't deprecate anything.
> We may have to tune the current behavior by displaying some deprecation
> messages, but we currently don't.
> So here is how it works: if you want to have access to `addon` global, you
> have to omit setting contentScript and contentScriptFile properties.

OK, so for the time being I'll document this just alongside the existing contentScript stuff, and not say anything about deprecation. But although it's painful for existing users, deprecating the old way is much better for new users.

Really I'm just pointing out that the new way isn't self-evidently better than the old way in all cases.

> Btw, do you have some thoughts about this global naming ?

Sorry, no useful ones. I feel pretty strongly that it should be the same name as it is in content scripts (i.e. "self"). Unfortunately I also feel pretty strongly that it should not be the same as the unrelated "self" module :-(.

I like "module" best, but understand that this clashes with CommonJS usage. I don't like "program", but agree with Myk that "addon" will look weird when SDK APIs are used to implement core Firefox features, rather than add-ons. Overall, though, I think "addon" is probably the best of the available options.
Comment 21 Will Bamberg [:wbamberg] 2011-12-13 10:25:55 PST
I've been working on the docs for bug 612726 "bypass content scripts for local content", and am thinking more about naming.

I think maybe we've spent too much energy worrying about what would be a "good" name for the page script's property, and not enough worrying about (1) consistency with the content script's property and (2) confusion with the "self" module. We should pay attention to Connor Dunn's comment 12.

Here are a couple of options.

(1) Call it "self". I think it's unfortunate that this is the same name as the self module, but consistency between the page script and the content script is more important than this. As it is, there's a bunch of documentation for using self.on/self.emit(). That can be shared between content script doc and page script doc, but if the property name is different it'll be a mess IMO.

This isn't just a documentation issue, it's an ease of use issue. Having a different name for the same thing will look gratuitously confusing, as Connor points out.

(2)  (a) call it "addon",
     (b) give content scripts a copy called "addon" while keeping their "self", and
     (c) deprecate content script's "self" in all documentation.

Which one we choose depends on whether we want to go through the extra disruption and work of (2) now, or not. I think eventually we should, but if we're not prepared to do it now, we should be prepared to live with the current situation for at least several months, and in that case consistency between page script and content script wins.
Comment 22 Alexandre Poirot [:ochameau] 2011-12-13 10:31:06 PST
I totally agree with Connor's comment!
I'm convinced that consistency is mandatory between content script and documents. In parallel, `self` brings many issues, even more in a web page, where `self` can be misused by web frameworks.

I see mutliple options:
- rename it to `self` and stick with it,
- rename it to `self` and try to deprecate to a better name in 1.5/1.6 release
- stick with "something else than self" and:
    - do not communicate about it until we deprecate self in content script
    (we can't deprecate self in content script for 1.4 release)
    - back it out for 1.4 ?

In any case, speaking about documentation, I expect this feature to simplify a lot Panel, Widget and Page-worker usage and example!

Note You need to log in before you can comment on or make changes to this bug.