1280482 - Give content scripts access to export helpers

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Kris Maglione [:kmag]]

This should be easy enough to do, and would be extremely useful for content scripts that are trying to interact with web content.

It should be as simple as adding `wantExportHelpers: true` to the Sandbox created in ExtensionContent.jsm[1], and then adding a test to make sure that createObjectIn(), evalInWindow(), and exportFunction() are actually available.


[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#378

Comment 1

[John Kang]

Hello, may I work on this bug?

Comment 2

[Andrew Swan [:aswan]]

Sure, thanks for your interest!
If you haven't built firefox before, that would be a good place to start: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Also take a look at the notes about hacking on webextensions here: https://wiki.mozilla.org/WebExtensions/Hacking

Kris outlined the steps that should be taken in the bug description, if you need more detail or have other questions feel free to ask here or in #webextensions on IRC.

Comment 3

[John Kang]

Thanks! I built Firefox and made the required change but mochitest doesn't seem to work. I tried to just run any old mochitest to see if it runs okay but I get this output.
http://pastebin.com/Xf5RLj6L

Comment 4

[Andrew Swan [:aswan]]

What was the command you ran to start the tests?  Also, if you run again an unmodified mozilla-central tree, do you still see the failures?  (ie, commit your changes, then run "hg up central", then build, then run tests again).  One other thing to try would be to pull (and update) to the latest version at mozilla-central, then re-build and run again.

And, it looks like you're doing a full build, artifact builds should work fine for this bug and save you some time:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Comment 5

[John Kang]

I ran ./mach mochitest and ./mach mochitest [filepath] on a fresh clean build and I still get errors :/

I've ran this fine before on my ubuntu laptop but I've been running into obstacles on my macbook.

Comment 6

[Kris Maglione [:kmag]]

I'd advise trying an artifact build rather than doing your own debug build. If that doesn't work, please attach your mozconfig file along with the test output.

Comment 7

[:shell escalante]

do you have time to work on the solution in the next week or two?

Comment 8

[John Kang]

Yes, I will be working on this. Is there a specific artifact build best suited for this particular bug or will they all work fine?

Comment 9

[Andrew Swan [:aswan]]

See the link in comment 4.  Artifact builds aren't something that you download, they are a method for doing a local build much more quickly (compiled object files for your platform are downloadeded but that all happens automatically, you don't need to do anything other than add a line to your mozconfig).

This bug is targetted for Firefox 50 which means it needs to land by the end of next week.  If you're not sure if you'll be able to get it done by then, we could help you find a different good first bug to work on?  (Though in any case, getting comfortable with the process of building Firefox and using artifact builds is a good first step).

Comment 10

[John Kang]

I just did an artifact build and mochitest is still broken for me. I'm going to attempt to fix that issue with my device in the mean time so I will leave this bug to someone else so that I don't hold up this issue. Thanks!

Comment 11

[Rob Wu [:robwu]]

evalInWindow appears to be dead: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.evalInWindow

I'll add cloneInto, exportFunction and createObjectIn instead.

Comment 12

[Rob Wu [:robwu]]

Attachment: Bug 1280482 - Add export helpers to content scripts

Review commit: https://reviewboard.mozilla.org/r/64738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64738/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

https://reviewboard.mozilla.org/r/64738/#review62088

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:31
(Diff revision 1)
> +    let s = document.createElement("script");
> +    s.textContent = `(${function() {
> +      let result1 = "unknown 1";
> +      let result2 = "unknown 2";
> +      try {
> +        result1 = +precisePi();

Why the `+`?

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:40
(Diff revision 1)
> +      try {
> +        result2 = -window.precisePi();
> +      } catch (e) {
> +        result2 = "err:" + e;
> +      }
> +      document.currentScript.dispatchEvent(new CustomEvent("customevt", {

It would be better to use `window.postMessage` for this.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:52
(Diff revision 1)
> +    browser.test.assertTrue(result);
> +    browser.test.assertTrue(Array.isArray(result));
> +    browser.test.assertEq(2, result.length);

No need for these three tests.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:55
(Diff revision 1)
> +    browser.test.assertEq(3.14, result[0]);
> +    browser.test.assertEq(-3.14, result[1]);

Please add descriptions for these tests.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:58
(Diff revision 1)
> +    browser.test.assertTrue(Array.isArray(result));
> +    browser.test.assertEq(2, result.length);
> +    browser.test.assertEq(3.14, result[0]);
> +    browser.test.assertEq(-3.14, result[1]);
> +
> +    browser.test.notifyPass("Content script done");

Please make the message more specific to this test.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64738/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

https://reviewboard.mozilla.org/r/64738/#review62376

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_exporthelpers.html:40
(Diff revision 2)
> +      try {
> +        result2 = window.precisePi();
> +      } catch (e) {
> +        result2 = "err:" + e;
> +      }
> +      document.currentScript.dispatchEvent(new CustomEvent("customevt", {

This still needs to be updated to use `window.postMessage` rather than a custom event.

Comment 16

[Rob Wu [:robwu]]

https://reviewboard.mozilla.org/r/64738/#review62088

> Why the `+`?

Opposed to -window.precisePi(). I've now removed the + and - and used the description to distinguish between the two assertions.

> It would be better to use `window.postMessage` for this.

postMessage is asynchronous. I want to have at least one unit test that confirms that an inline script from a content script runs synchronously. It is not very likely to break, but better safe than sorry.

Comment 17

[Rob Wu [:robwu]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

(I forgot to publish the replies to your previous comments, please take another look. I haven't updated the patch since your last review)

Comment 18

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/64738/#review62088

> Opposed to -window.precisePi(). I've now removed the + and - and used the description to distinguish between the two assertions.

Unary plus coerces the type of the result, so it's best to avoid here.

> postMessage is asynchronous. I want to have at least one unit test that confirms that an inline script from a content script runs synchronously. It is not very likely to break, but better safe than sorry.

Testing synchronous execution of scripts seems orthogonal to this test. If you really think it's that important, though, then please export another function and have the content script call it with its results as an argument. That has the benefit of adding some additional tests for `exportFunction` functionality.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

https://reviewboard.mozilla.org/r/64738/#review62422

Comment 20

[Rob Wu [:robwu]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64738/diff/2-3/

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

https://reviewboard.mozilla.org/r/64738/#review62718

Comment 22

[Rob Wu [:robwu]]

Do we need to publish some documentation about using these APIs? There are bits and pieces everywhere about exportFunction, etc., but I didn't find anything that says that e.g. window.eval runs code in the page's scope instead of the content script's (this is a significant difference with Chrome extensions and has potential security implications).

An up-to-date page that shows the state of art and warns about pitfalls seems useful.

Comment 23

[Kris Maglione [:kmag]]

Yes, just add the dev-doc-needed keyword.

As for `window.eval`, that shouldn't even exist, except via `wrappedJSObject`. The `eval` global does exist, though, and evaluates code in the context of the sandbox.

Comment 24

[Carsten Book [:Tomcat]]

does not apply cleanly:

patching file toolkit/components/extensions/ExtensionContent.jsm
Hunk #1 FAILED at 374
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/ExtensionContent.jsm.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 25

[Rob Wu [:robwu]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64738/diff/3-4/

Comment 26

[Rob Wu [:robwu]]

Rebased patch, please try again.

Comment 27

[Rob Wu [:robwu]]

[Tracking Requested - why for this release]: WebExtensions is going to stable in 48, and the ability to safely expose APIs to a web page is necessary.

Comment 28

[Rob Wu [:robwu]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

Approval Request Comment
[Feature/regressing bug #]: WebExtensions
[User impact if declined]: WebExtensions would not have access to APIs to safely interact between (privileged) content scripts and (hostile) web pages.
[Describe test coverage new/current, TreeHerder]: Passes all mochitests on Windows and Linux (opt & debug)
[Risks and why]: The export helpers for WebExtensions allow extension authors to safely interact with a web page. Without these helpers, WebExtension authors will probably resort to less efficient methods, and possibly not adopt the use of these helper functions when these become available.
[String/UUID change made/needed]: No

Comment 29

[Kris Maglione [:kmag]]

(In reply to Rob Wu [:robwu] from comment #28)
> [Risks and why]: The export helpers for WebExtensions allow extension
> authors to safely interact with a web page. Without these helpers,
> WebExtension authors will probably resort to less efficient methods, and
> possibly not adopt the use of these helper functions when these become
> available.

The risks of uplifting this are extremely small. Aside from added tests, this
is a one-line change that adds a single option to content script sandboxes.
That setting is already widely used by other content script sandboxes, so its
behavior is well-tested and well-understood.

It's also worth noting that this change makes it much easier for extension
authors to write secure extensions when they need to interact directly with
web pages. If this feature isn't available in Firefox 48, it will be much more
difficult to convince them to do so.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ee4f41614adc
Add export helpers to content scripts. r=kmag

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee4f41614adc

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8771636 [details]
Bug 1280482 - Add export helpers to content scripts

It's too late for 48 as we already built rc2 and it doesn't seem to be a super critical. If you want to mention this in release notes, please set the flag relnote-firefox.

Comment 33

[Gerry Chang [:gchang]]

Track 49 for WebExtension.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8afbaa70f16

Comment 35

[Rob Wu [:robwu]]

MDN needs to be updated. It can be a subsection of https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Communicating_with_the_web_page (make sure to mark it as Firefox-only, these helpers are an established concept in Firefox addons but non-existing in Chrome extensions).

To write documentation, you can look at the Addon SDK's docs: https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/Interacting_with_page_scripts#Expose_objects_to_page_scripts (NOTE: unsafeWindow is NOT supported in WebExtensions, see comment 11 and/or the patch to see what's implemented).

Once we have an article, please update the introduction at Addon SDK vs WebExtensions:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Comparison_with_the_Add-on_SDK#Content_scripts
"In the SDK, content scripts can share objects with page scripts, using techniques like unsafeWindow and createObjectIn. >>>>This is not possible in WebExtensions.<<<<<"

Comment 36

[Will Bamberg [:wbamberg]]

I've added a section to that page:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Sharing_objects_with_page_scripts

Please let me know if it makes sense to you.

As we discussed, I omitted createObjectIn, but have documented wrappedJSObject.

I've also written a little example WebExtension for this: if you think it's useful I'd be grateful for you to take a look at that as well. There will be a web page to go with it as well (although of course you can test this on any page using the Web Console.

Comment 37

[The 8472]

I think it might also make sense to expose Cu.{waive|unwaive}Xrays.

Comment 38

[Kris Maglione [:kmag]]

(In reply to Will Bamberg [:wbamberg] from comment #36)
> I've added a section to that page:
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Content_scripts#Sharing_objects_with_page_scripts
> 
> Please let me know if it makes sense to you.
> 
> As we discussed, I omitted createObjectIn, but have documented
> wrappedJSObject.
> 
> I've also written a little example WebExtension for this: if you think it's
> useful I'd be grateful for you to take a look at that as well. There will be
> a web page to go with it as well (although of course you can test this on
> any page using the Web Console.

Just one nit, to start:

  function onExecuted() {
    if (chrome.runtime.lastError) {
      console.log(chrome.runtime.lastError);
    }
  }

I don't think there's any need for this. The error will be reported to the
console if it's not explicitly checked, so there's no need for a callback at
all unless the caller needs the results.


That aside, I think this is a pretty good overview for beginners. I kind of
want to quibble over some of the details that are left out, but I'm really not
sure how many of them actually belong in the overview, as opposed to in the
detailed pages that you link to.

I think the one detail that we should probably at least mention is that
arguments passed to exported functions are wrapped in security wrappers, and
that objects obtained from the unwrapped window can, and generally should, be
explicitly rewrapped: `XPCNativeWrapper(window.wrappedJSObject.foo)`

I think maybe it would be helpful to just add a section with a quick overview
of how security wrappers work to this page, since the full Xray vision page
might be a bit overwhelming to newcomers.


(In reply to The 8472 from comment #37)
> I think it might also make sense to expose Cu.{waive|unwaive}Xrays.

That's easier said than done. It would require platform changes, and they
would probably have to ride the trains from 51. I agree that it probably makes
sense, though.

Even so, they can currently use XPCNativeWrapper() and
XPCNativeWrapper.unwrap() to the same effect (which should probably be
documented).

Comment 39

[The 8472]

(In reply to Kris Maglione [:kmag] from comment #38)
> That's easier said than done. It would require platform changes

I think it could be done in JS. Add Components to the sandbox, transplant the functions, delete Components.

Comment 40

[Kris Maglione [:kmag]]

(In reply to The 8472 from comment #39)
> I think it could be done in JS. Add Components to the sandbox, transplant
> the functions, delete Components.

It's not that simple. Only code running with the system principal has access to Components.utils, and the Components global is non-configurable when it's present.

Even if it were that simple, though, I'd rather handle it on the platform side with wantExportHelpers, which would be cleaner and no more complicated.

Comment 41

[Will Bamberg [:wbamberg]]

(In reply to Kris Maglione [:kmag] from comment #38)
> (In reply to Will Bamberg [:wbamberg] from comment #36)
> > I've added a section to that page:
> > 
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> > Content_scripts#Sharing_objects_with_page_scripts
> > 
> > Please let me know if it makes sense to you.
> > 
> > As we discussed, I omitted createObjectIn, but have documented
> > wrappedJSObject.
> > 
> > I've also written a little example WebExtension for this: if you think it's
> > useful I'd be grateful for you to take a look at that as well. There will be
> > a web page to go with it as well (although of course you can test this on
> > any page using the Web Console.
> 
> Just one nit, to start:
> 
>   function onExecuted() {
>     if (chrome.runtime.lastError) {
>       console.log(chrome.runtime.lastError);
>     }
>   }
> 
> I don't think there's any need for this. The error will be reported to the
> console if it's not explicitly checked, so there's no need for a callback at
> all unless the caller needs the results.
> 
> 
> That aside, I think this is a pretty good overview for beginners. I kind of
> want to quibble over some of the details that are left out, but I'm really
> not
> sure how many of them actually belong in the overview, as opposed to in the
> detailed pages that you link to.

This is the main page on content scripts, and this part is an advanced, browser-specific feature. I don't really want to go into too much detail on it here, and there is quite detailed documentation in the pages I link to.

> I think the one detail that we should probably at least mention is that
> arguments passed to exported functions are wrapped in security wrappers, and
> that objects obtained from the unwrapped window can, and generally should, be
> explicitly rewrapped: `XPCNativeWrapper(window.wrappedJSObject.foo)`

That's a good point.

> 
> I think maybe it would be helpful to just add a section with a quick overview
> of how security wrappers work to this page, since the full Xray vision page
> might be a bit overwhelming to newcomers.
> 

I have had a go at doing this, although I'm not sure how successful it is. One thing: the "Xray vision" page intentionally does not talk about "wrappers": this came from feedback I had when I wrote it that "wrappers" is an implementation detail that should not be mentioned. Having made this choice, I'd like to keep this consistent as far as possible (although having things like "wrappedJSObject" makes that difficult).

> (In reply to The 8472 from comment #37)
> > I think it might also make sense to expose Cu.{waive|unwaive}Xrays.

Yes, I agree.