679479 - Document how to communicate between content scripts and page scripts

Status: RESOLVED FIXED

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

Description

[Will Bamberg [:wbamberg]]

Bug 666547 adds the ability for content scripts to communicate with page scripts using document.defaultView.postMessage.

We ought to document this. I think it's best to dismember the content scripts guide into chapters, since it's quite long now, and add a chapter to address this bug.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Dismember content script guide; add stuff on page scripts and proxies

I've split the content script guide into chapters, as it was getting a bit long, and in a chapter on what sorts of things content scripts get access to have added two new sections:

* accessing DOM through the proxy, and a bit on the implications of this
* accessing page scripts using document.defaultView.postMessage

Comment 2

[Will Bamberg [:wbamberg]]

Attachment: Updated patch

Updated patch: dismember content script guide, add sections on proxies and page script access

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(It feels scary to be taking such a large patch on the stabilization branch, especially after the initial two-week period for landing docs changes on that branch that we have defined in the development process.  Nevertheless, I can imagine making an exception in this case so that we document the new feature, which is why drivers left this targeted to 1.1 during our triage this morning.)

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 553800 [details] [diff] [review]
Updated patch

Review of attachment 553800 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this documentation, it really gives a great value on our dark-internal-low-level developments :)
My main concern is my first comment about a false statement, otherwise it looks good.

::: static-files/md/dev-guide/addon-development/content-scripts/access.md
@@ +11,5 @@
> +Content scripts need to be able to access DOM objects in arbitrary web
> +pages, but this gives rise to two potential security problems:
> +
> +1. changes the add-on made to the DOM would be visible to the page, making
> +it obvious to the page that an add-on was modifying it.

It is not really the case. It is easy for the page to detect DOM modification (if the content script do modify the DOM).
I think you tried to said that JS values from the content script can be accessed by the page.
Key points to describe these proxy are: DOM and standards (also called "native").
So you can access to all DOM things (getAttribute, getElementById, querySelector),
but not only DOM, you have access to all web standards like: postMessage, localStorage, ...
Then what you try to say with 1. and 2. is that both sides are equals.
Same "visibility rule" apply from content script to page script and from page script to content script.
Finally, we just try to explain the benefit of these rules from both sides:
1. page script can't access/modify content script JS values.
=> the page can't steal data/priviledge method from the content script, nor trick it.
=> the page script will not conflict with content script (having a jQuery in the page won't mess anything in the content script)
2. content script can't access/modify page script JS values.
=> the content script can't leak data to the page script not be tricked by the page.
=> the content script won't pollute the page script (for ex, jQuery)

@@ +71,5 @@
> +The proxy is transparent to content scripts: as far as the content script
> +is concerned, it is accessing the DOM directly. But because it's not, some
> +things that you might expect to work, won't. For example, if the page includes
> +a library like [jQuery](http://www.jquery.com), or any other page script
> +adds any other objects to the window, they won't be visible to the content

It is not necessary only "objects to the window".
Some pages may set values to any DOM node.
But having said that, I think that your description is clear 
and giving too much details may be irrelevant.

@@ +80,5 @@
> +
> +If you really need direct access to the underlying DOM, you can use the
> +global `unsafeWindow` object. Try editing the example at [https://builder.addons.mozilla.org/addon/1013777/revision/4/](https://builder.addons.mozilla.org/addon/1013777/revision/4/)
> +so the content script uses `unsafeWindow.confirm()` instead of
> +`window.confirm()` to see the difference.

We have to create an account, give a mail, install addon builder, ...
It would be really cool to have another addon example with unsafeWindow.
Btw, your idea of publishing such examples on addon builder is really awesome!

@@ +94,5 @@
> +
> +Content scripts loaded into the same global execution context can interact
> +with each other directly as well as with the web content itself. However,
> +content scripts which have been loaded into different execution contexts
> +cannot interact with each other.

Wouldn't "document" be better than "global execution context"?

::: static-files/md/dev-guide/addon-development/content-scripts/using-port.md
@@ +42,5 @@
> +alt="Content script events">
> +
> +Events are asynchronous: that is, the sender does not wait for a reply from
> +the recipient but just emits the event and continues processing.
> +

To be extremelly precise, Events are sent asynchronously,
but the call to emit() doesn't wait for anything but release immedialy as it send the message asynchronously to the worker.

Comment 5

[Alexandre Poirot [:ochameau]]

canuckistani | alexp_: I'm a +1 for this patch, I think it improves things far more than it might potentially hurt
alexp_       | canuckistani: sure it's a huge benefit that would have help two people I tried to help using page-mod, just for today :o
canuckistani | exactly! ship it!

myk: I let you finally decide to take it or not in 1.1.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

This is a docs-only patch, and it's very helpful, to be sure.  But it's also a large patch whose latest status is r-, and it's a day before we plan to ship 1.1rc1.  So I'm skeptical that we should take it at this time.  But I'll cc: the other drivers, reset the milestone, and tag it [triage:followup] to prompt us to take another look at it.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Myk Melez [:myk] from comment #6)
> This is a docs-only patch, and it's very helpful, to be sure.  But it's also
> a large patch whose latest status is r-, and it's a day before we plan to
> ship 1.1rc1.  So I'm skeptical that we should take it at this time.  But
> I'll cc: the other drivers, reset the milestone, and tag it
> [triage:followup] to prompt us to take another look at it.

I'd agree that this should miss the 1.1 release now. Let's get it in for 1.2 a.s.a.p. though.

Comment 8

[Jeff Griffiths (:canuckistani) (:⚡︎)]

For the record, I don't agree - current docs will continue to do more harm between now and 1.2 than these changes possibly could, in terms of helping people understand how page-mod works.

As a mitigation step I'm going to prioritize another FAQ post around explaining how this works based on Will's work in this patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Various folks revisited this at yesterday's weekly meeting, after which drivers decided to pass on it for 1.1.

To mitigate the cost of not having these docs in the 1.1 timeframe, Jeff is going to prioritize another FAQ post, as described in comment 8; and to make sure we get these docs into 1.2, someone (Jeff?) is going to pick up the bug and finish the patch per Alex's review comments while Will is away.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I just published this blog post with an edited version of the content script access section of this patch:

http://blog.mozilla.com/addons/2011/09/01/add-on-sdk-faq-content-script-access/

I'm hoping to get some feedback for this, and should have a new patch posted to this bug tomorrow.

Comment 11

[Will Bamberg [:wbamberg]]

Attachment: Updated patch, again

(In reply to Alexandre Poirot (:ochameau) from comment #4)
> Comment on attachment 553800 [details] [diff] [review]
> Updated patch
> 
> Review of attachment 553800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for this documentation, it really gives a great value on our
> dark-internal-low-level developments :)
> My main concern is my first comment about a false statement, otherwise it
> looks good.
> 
> ::: static-files/md/dev-guide/addon-development/content-scripts/access.md
> @@ +11,5 @@
> > +Content scripts need to be able to access DOM objects in arbitrary web
> > +pages, but this gives rise to two potential security problems:
> > +
> > +1. changes the add-on made to the DOM would be visible to the page, making
> > +it obvious to the page that an add-on was modifying it.
> 
> It is not really the case. It is easy for the page to detect DOM
> modification (if the content script do modify the DOM).
> I think you tried to said that JS values from the content script can be
> accessed by the page.
> Key points to describe these proxy are: DOM and standards (also called
> "native").
> So you can access to all DOM things (getAttribute, getElementById,
> querySelector),
> but not only DOM, you have access to all web standards like: postMessage,
> localStorage, ...
> Then what you try to say with 1. and 2. is that both sides are equals.
> Same "visibility rule" apply from content script to page script and from
> page script to content script.
> Finally, we just try to explain the benefit of these rules from both sides:
> 1. page script can't access/modify content script JS values.
> => the page can't steal data/priviledge method from the content script, nor
> trick it.
> => the page script will not conflict with content script (having a jQuery in
> the page won't mess anything in the content script)
> 2. content script can't access/modify page script JS values.
> => the content script can't leak data to the page script not be tricked by
> the page.
> => the content script won't pollute the page script (for ex, jQuery)
> 

I've tried to fix this.

> > +
> > +If you really need direct access to the underlying DOM, you can use the
> > +global `unsafeWindow` object. Try editing the example at [https://builder.addons.mozilla.org/addon/1013777/revision/4/](https://builder.addons.mozilla.org/addon/1013777/revision/4/)
> > +so the content script uses `unsafeWindow.confirm()` instead of
> > +`window.confirm()` to see the difference.
> 
> We have to create an account, give a mail, install addon builder, ...
> It would be really cool to have another addon example with unsafeWindow.

Agreed that it's kind of painful. I've added a new example which is identical, except for unsafeWindow.

> 
> @@ +94,5 @@
> > +
> > +Content scripts loaded into the same global execution context can interact
> > +with each other directly as well as with the web content itself. However,
> > +content scripts which have been loaded into different execution contexts
> > +cannot interact with each other.
> 
> Wouldn't "document" be better than "global execution context"?

Yes, it would :)

> 
> ::: static-files/md/dev-guide/addon-development/content-scripts/using-port.md
> @@ +42,5 @@
> > +alt="Content script events">
> > +
> > +Events are asynchronous: that is, the sender does not wait for a reply from
> > +the recipient but just emits the event and continues processing.
> > +
> 
> To be extremelly precise, Events are sent asynchronously,
> but the call to emit() doesn't wait for anything but release immedialy as it
> send the message asynchronously to the worker.

Hm. I don't see how what you've said differs from what I have...?

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Will Bamberg [:wbamberg] from comment #11)
> > ::: static-files/md/dev-guide/addon-development/content-scripts/using-port.md
> > @@ +42,5 @@
> > > +alt="Content script events">
> > > +
> > > +Events are asynchronous: that is, the sender does not wait for a reply from
> > > +the recipient but just emits the event and continues processing.
> > > +
> > 
> > To be extremelly precise, Events are sent asynchronously,
> > but the call to emit() doesn't wait for anything but release immedialy as it
> > send the message asynchronously to the worker.
> 
> Hm. I don't see how what you've said differs from what I have...?

You are 101% right! I totally misread this sentence!

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 558648 [details] [diff] [review]
Updated patch, again

Review of attachment 558648 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I've just added two small "cherry on the cake's comments" ;)

::: static-files/md/dev-guide/addon-development/content-scripts/access.md
@@ +8,5 @@
> +
> +## Access to the DOM ##
> +
> +Content scripts need to be able to access DOM objects in arbitrary web
> +pages, but this gives rise to two potential security problems:

Isn't there a typo here? gives+rise?

@@ +83,5 @@
> +
> +To see the difference, try editing the example at [https://builder.addons.mozilla.org/addon/1013777/revision/4/](https://builder.addons.mozilla.org/addon/1013777/revision/4/)
> +so the content script uses `unsafeWindow.confirm()` instead of
> +`window.confirm()`. Alternatively, try out the example at
> +[https://builder.addons.mozilla.org/addon/1015979/revision/3/](https://builder.addons.mozilla.org/addon/1015979/revision/3/).

These long builder URLs disturb reading the whole sentence.

Comment 14

[Will Bamberg [:wbamberg]]

Thanks Alex!

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> Comment on attachment 558648 [details] [diff] [review]
> Updated patch, again
> 
> Review of attachment 558648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I've just added two small "cherry on the cake's comments" ;)
> 
> ::: static-files/md/dev-guide/addon-development/content-scripts/access.md
> @@ +8,5 @@
> > +
> > +## Access to the DOM ##
> > +
> > +Content scripts need to be able to access DOM objects in arbitrary web
> > +pages, but this gives rise to two potential security problems:
> 
> Isn't there a typo here? gives+rise?
> 

No, "give rise to" is a real expression meaning, more or less, "causes". But it is probably too idiomatic, so I've replaced it with "could cause".

> @@ +83,5 @@
> > +
> > +To see the difference, try editing the example at [https://builder.addons.mozilla.org/addon/1013777/revision/4/](https://builder.addons.mozilla.org/addon/1013777/revision/4/)
> > +so the content script uses `unsafeWindow.confirm()` instead of
> > +`window.confirm()`. Alternatively, try out the example at
> > +[https://builder.addons.mozilla.org/addon/1015979/revision/3/](https://builder.addons.mozilla.org/addon/1015979/revision/3/).
> 
> These long builder URLs disturb reading the whole sentence.

Fixed that too, and landed as: https://github.com/mozilla/addon-sdk/commit/779df576cd43826ee1de4f25d37e0583acf294e7