1214658 - chrome object not available to scripts inside an iframe

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Maciej Ćwiek]

Attachment: iframetest.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

See the attached sample extension.
Steps to reproduce:
1. Unzip and install the unpacked extension.
2. Open any website.
3. Open JS console and check out the logs.


Actual results:

Chrome object is undefined for scripts loaded into the iframe.

Content script creates an iframe object and sets src to a html file, which belongs to the extension and appends it to DOM. The html loads a js file (also local to the ext) through script tag. The js requires chrome API, but the whole chrome object doesn't seem to be available whatsoever.


Expected results:

Chrome object should be available to scripts belonging to the extension, also if loaded into iframes, so then the consistency with Google Chrome for this case is preserved.

Comment 1

[Maciej Ćwiek]

The issue might actually be related to how script inclusions via script tags are treated, as I noticed that if I open the html file (attached extension -> html/iframe.html) in a separate tab, I get exactly the same error in the console. So it's not specific to the iframe I think.

Comment 2

[Gabor Krizsanits (INACTIVE)]

I think we just don't have a mechanism implemented yet to inject chrome/browser APIs into the global, when the windows URL belongs to an extension.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We do actually. But I excluded web_accessible_resources for security reasons. In the example in this bug it seems safe, but it would not be safe if you're injecting a <script> tag directly into a web page.

Comment 4

[Maciej Ćwiek]

(In reply to Bill McCloskey (:billm) from comment #3)
> We do actually. But I excluded web_accessible_resources for security
> reasons. In the example in this bug it seems safe, but it would not be safe
> if you're injecting a <script> tag directly into a web page.

Your example is not our intention though. The sample extension is exactly what we do in our target extension - Avira Browser Safety - and what we're eventually trying to achieve in FF, using WebExtensions API.
To me there should be no security concerns, as long as everything is done inside the extension scope, so when loading js via html, when both belong to the package.
So with relation to this use case, would you consider making it possible at all?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #3)
> We do actually. But I excluded web_accessible_resources for security
> reasons. In the example in this bug it seems safe, but it would not be safe
> if you're injecting a <script> tag directly into a web page.

Sure but I was talking about API injection for the case when the windows itself has a web accessible resource URL. For <script> tags we surely don't want to do any API injection I assume, but I might be missing something. Anyway, I assume we want this feature.

On the other hand I'm not sure if I get the principal story behind all this. I assume if a regular webpage has an iframe with a web accessible resource URL, they will have cross origin relationship. And if I add a <script> to the page with a resource URL, it will inherit the pages principal (as it's running in its scope) and will have cross origin relationship with the iframe as well. If this is the case then there should not be an issue, right?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've started by looking how Chrome behaves in the described scenario, 
to better evaluate which APIs we need to enable into such an iframe's window object.

It turned out that in Chrome, the iframe (created as in this issue description):
- gets the same APIs available to the content script (which has basically no direct access to the 
  privileged APIs)
- when the addon is disabled the iframe remains in its place but the chrome object's 
  properties are all set to undefined

During this tests I wondered what happens if the content script will create a new popup
window using window.open instead: 

Chrome creates a new extension tab page (with the expected privileged extension APIs available)
and the tab is immediately closed if the extension is disabled.

On Firefox it currently creates a new tab without the expected "privileged extension
APIs", so I'm going to open a separate issue to move on it the discussion about
the "window.open" scenario.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

This is a basic test case which reproduces the described behavior, 
it fails as expected on the current implementation and should pass once the fix is applied

(nevertheless, there are many good security-related reasons why webextension APIs are not enabled
by default in any possible window object, so this should be considered only the first
test, I want to create more test cases before landing any change to the current behavior,
at least the tests that ensures that we are not enabling any webextension API when/where
it shouldn't be available)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-enable-content-script-APIs-into-child-wi.patch

This is the patch which implements the first draft of the following proposed approach:

- When we are notified (in ExtensionContent) about a new window object available,
- If it is a window object which has "moz-extension/chrome-extension" as window.location.protocol
- And the window.location.hostname is the valid UUID of the same webextension addon which wraps
  the window.parent
- Then we immediately create an ExtensionContext for it (because the webextension APIs have to be available to the scripts that will be loaded in the page)
- And ExtensionContext detect the above scenario and define the context's sandbox as the
  window itself, instead of creating the isolated sandbox like in regular content script's
  ExtensionContext
- when the ExtentionContext is destroyed (e.g. the webextension addon has been disabled)
  the iframe remains in its place and the injected chrome and browser API objects are deleted (which is slightly different from
  what chrome does, by setting all the chrome object's properties to undefined)

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Here is a try launched on the two attached patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77fe79ee3bc4

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8695277 [details] [diff] [review]
0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

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

r+, but please don't land until it passes, obviously.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_create_iframe.html
@@ +10,5 @@
> +</head>
> +<body>
> +
> +<!-- WORKAROUND: this iframe hack is used to contain the html page source without escaping it -->
> +<iframe id="test-asset">

I guess this is fine, but please use a |<textarea>|, and don't include the tag name in the query below.

@@ +13,5 @@
> +<!-- WORKAROUND: this iframe hack is used to contain the html page source without escaping it -->
> +<iframe id="test-asset">
> +  <!DOCTYPE>
> +  <html>
> +    <script>

Please put this in a |<head>| element, and include |<meta charset="utf-8">| to prevent warnings.

@@ +54,5 @@
> +        "content_script_iframe.html"
> +      ]
> +    },
> +
> +    background: "(" + backgroundScript.toString() + ")()",

No need for the |.toString()|. Same below.

@@ +66,5 @@
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +
> +  let contentScriptIframeCount = 0;
> +  let contentScriptIframeCreatedPromise = new Promise(resolve => {
> +    extension.onMessage("content-script-iframe-loaded", () => { contentScriptIframeCount++; resolve(); });

One line per statement in the arrow function, please. I don't think there's any need for the count, though, since you resolve immediately after incrementing it, so it will never be more than 1. You should be able to just use |extension.awaitMessage("content-script-iframe-loaded")| below.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8695281 [details] [diff] [review]
0002-Bug-1214658-enable-content-script-APIs-into-child-wi.patch

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

This isn't a bad approach, but it's missing some pieces, and I think we can do better for detecting which documents to inject into.

So, as for the missing pieces:

 * We currently ignore web-accessible resources when asked for the add-on ID or a URL, in ExtensionManagement.jsm, in Service.extensionURIToAddonID. This is what prevents our APIs from being injected into documents loaded by web pages. I think we can turn off this behavior now that we're handling these differently. It *might* still be a good idea to only inject content script APIs into documents that are loaded by web pages, though.

 * In cases where we inject the content script API into a page, we need to make sure we're not also injecting the privileged API into them, and vice versa. I think that with your patch, there are cases where we'll inject both.

Once the first point above is addressed, we can use the |addonId| attribute of the window's principal to decide which windows to inject the APIs into, like we do in Extension.jsm's GlobalManager. And since, once we know that a window belongs to an add-on, we know that we need to inject one or the other API into the window, we should probably put that logic into a helper function in ExtensionManagement.jsm or ExtensionUtils.jsm.

I think the logic should probably go something like this:

 * If the browser is in a content process, inject the content script API.

 * If the browser is not a top-level window, inject the content script API. This may be a bit tricky to detect, and probably needs to be done via the docshell tree item's |sameTypeRootTreeItem| property to avoid issues with sandboxed iframes. (For which, feel free to ping me if you have questions)

 * If the browser was opened by a content window, inject the content script API. I think we can save this for a follow-up bug.

 * Otherwise, inject the full API.

This also means we can remove the |GlobalManager.injectInDocShell| function.

That's rather a lot... so feel free to ping me on IRC, or ask here, if you have any questions/concerns.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +247,5 @@
> +    // its sandbox is the contentWindow object itself
> +    this.sandbox = contentWindow;
> +  } else {
> +    this.sandbox = Cu.Sandbox(prin, {sandboxPrototype: contentWindow, wantXrays: true, isWebExtensionContentScript: true});
> +  }

I think perhaps it would be better to always create a sandbox to inject the APIs into, and then copy them into the content page, so that we can still nuke the sandbox when the extension is unloaded, and we don't have to worry about |sandbox| sometimes being DOM window.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch (v2)

Thanks Kris for the very detailed comments!

This updated testcase patch integrates the changes related to the review comments (and it adds a couple of asserts to check the APIs available in the created iframe, it is not to be considered enough and I'm going to add more scenarios into the tests, after that I've updated the fix patch as pointed out in the feedback comments)

as we will not land this patch until the fix is reach, I'm not setting the "r+" as a further precaution.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-enable-content-script-APIs-into-child-wi.patch

This an updated version of the patch with a number of changes based on the feedback
comments:

- in ExtensionManagement.jsm:
  - do not filter out "webAccessibleResources" in extensionURIToAddonID
  - export the "extensionURILoadableByAnyone" helper to use it in Extension.jsm
    and ExtensionContent.jsm
- in Extension.jsm:
  - added some comments about where and why we filter out a
    detected window object before turning it into a webextensions "tab" context
  - added a check that filter out "webAccessibleResources" from being
    turned into a webextensions "tab" context (Nevertheless we will probably
    discuss about changing this behavior in a followup, because during my tests 
    I found that Chrome behave differently)
- in ExtensionContent.jsm:
  - rewrite of the "checkIfContentScriptIframe" helper to be more strict about
    its checks:
    - the document have an associated addon id of an enabled addons
    - the document uri is a "webAccessibleResources"
    - the container DOM element is an iframe (and the container element retrieved
      by using the nsIDOMUtilUtils.containerElement, to get it across chrome boundaries)
    - the parent window (retrieved using nsIDocShellTreeItem.parent) is wrapped by a
      content script from the same addon id
  - in ExtensionContext:
    - do not create a sandbox if isContentScriptIframe (the APIs object
      created in the sandbox can't be copied and accessed from the contentWindow)
    - cloneScope getter returns the contentWindow if isContentScriptIframe
      (it is used from the api getManifest method to clone the manifest object into
      the context the api method is called from)
    - the close method set the api properties to undefined if isContentScriptIframe
      (which is how Chrome behaves in this scenario)    
  - in the observe listener
    - if the document has an addonId and checkIfContentScriptIframe(window)
      the content script APIs are injected into the detected window object
    
In this version I'm not using the "sameTypeRootTreeItem" to detect the iframe yet,
but if the new checks are not strict enough we can discuss about how the 
"sameTypeRootTreeItem" can be used to do it better and I will change the patch accordingly.

It seems that now the checks ensure that a window can't be injected with both the 
privileged APIs and content script APIs and I tried to trigger a new top level window 
using the window.open and check that it is not detected and content script APIs injected, 
but I want to turn it into a separate test case and put it into a new version of the testcase patch.

Moreover I'd like to discuss with you about other possible tricky scenarios 
that needs to be checked (put together a list of them to turn it into test cases)

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

try run on the updated patch:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed8c485bf713

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8696393 [details] [diff] [review]
0002-Bug-1214658-enable-content-script-APIs-into-child-wi.patch

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

Thanks. This is definitely better, but it still a few problems. There are a
lot of arcane specifics that are hard to get right in code like this.

Sorry if this is a bit disorganized. I'm trying to reply to specific things in
your comment, comment on lines of your patch, and comment on the summary, but
Splinter/Bugzilla does not make it easy to do this well...

(In reply to Luca Greco from comment #14)
>   - in ExtensionContext:
>     - do not create a sandbox if isContentScriptIframe (the APIs object
>       created in the sandbox can't be copied and accessed from the contentWindow)

It should be, as long as the Sandbox has the correct principal,
and you unwaive X-rays on both the sandbox and the window for
the copy.

>     - cloneScope getter returns the contentWindow if isContentScriptIframe
>       (it is used from the api getManifest method to clone the manifest
>        object into the context the api method is called from)

I think we should still return the sandbox here, for the reasons explained
below.

>     - the close method set the api properties to undefined if
>       isContentScriptIframe (which is how Chrome behaves in this scenario)

I do think that's a good idea, but I don't think that it's enough. I haven't
tested, but I'd be willing to bet that Chrome does more than just set the
clear those properties when an add-on is disabled. If an add-on stored a
reference to one of the APIs, e.g.,

  let cookies = chrome.cookies;
  thing.onclick = {
    cookies.remove({url, name});
  };

or,

  let deleteCookie = chrome.cookies.remove.bind(chrome.cookies);
  thing.onclick = {
    deleteCookie({url, name});
  };

then the APIs still need to stop working.

I think the easiest way to cut the bindings is to continue defining them in
the sandbox, and nuke it when we shut down, so any references to those objects
immediately become dead wrappers, and we know that no running code has
access/references to them.

The other option is to define the content script API using the mechanism added
in bug 1208257, and just disable the method invocation wrappers at shutdown.
But I don't think that's as good a solution.

> In this version I'm not using the "sameTypeRootTreeItem" to detect the
> iframe yet, but if the new checks are not strict enough we can discuss about
> how the "sameTypeRootTreeItem" can be used to do it better and I will change
> the patch accordingly.

Checking |sameTypeParent| is just as good a solution.

> It seems that now the checks ensure that a window can't be injected with
> both the privileged APIs and content script APIs and I tried to trigger a
> new top level window using the window.open and check that it is not detected
> and content script APIs injected, but I want to turn it into a separate test
> case and put it into a new version of the testcase patch.

I think that's probably true, but I'd still rather have the checks for both
cases centralized in one function that decides if we need a full API, a
content script API, or no injection at all. Trying to keep two separate sets
of checks in sync is going to lead to corner cases sooner or later.

> Moreover I'd like to discuss with you about other possible tricky scenarios
> that needs to be checked (put together a list of them to turn it into test
> cases)

That sounds like a good idea.


There are a few things I think we need to do differently:

- I'd rather aim for as close to the final expected behavior as possible,
  rather than saving too much for follow-up bugs. We can decide later whether
  we want windows/tabs opened by content pages to have the content script API,
  but aside from that we should implement this as discussed: Top-level pages
  in the main process should have the full extension API, iframes and other
  pages in the content process should have the content script API. We
  shouldn't check whether the URIs are web accessible at all.

- Don't bother checking the container element. It doesn't matter if it's an
  iframe, or not. It only matters that it's not a child of another
  content-type docshell. This is what checking |sameTypeParent| or
  |sameTypeRootTreeItem| tells us.

- Make the checks for full API and content API injection in the same place,
  using the same logic. The current API injection checks really need to be
  upgraded, anyway. For instance, they should be checking |sameTypeParent|
  rather than |window.top| too.

- I don't think we should be doing the API injection from |getContext|. It
  makes sense that |getContext| injects the APIs into the sandbox, because it
  creates/owns the sandbox, so there aren't any external side-effects. When
  we're injecting the APIs into existing windows, I think it needs to be more
  explicit.

  Since I still think we should be initially exporting the APIs into a sandbox
  before copying them to the content window, it would make sense to keep
  |getContext| as-is, and copy the APIs from the sandbox it creates to the
  content window we're injecting into.

- There are some quirks with X-ray wrappers and inner vs. outer windows that
  aren't quite handled correctly. There are some pretty good docs about this
  on MDN:

  * https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows
  * https://developer.mozilla.org/en-US/docs/Working_with_BFCache
  * https://developer.mozilla.org/en-US/docs/Xray_vision

  But they're not exactly complete (or sometimes up-to-date), so please ping
  me if you have any questions.

  See code comments for specifics.

::: toolkit/components/extensions/Extension.jsm
@@ +295,5 @@
>      let principal = contentWindow.document.nodePrincipal;
>      let id = principal.originAttributes.addonId;
> +
> +    // We don't inject privileged apis if the addonid is null
> +    // or doesn't exists

I think the comment above covers this.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +195,5 @@
>    }
>  }
>  
> +// Bug 1214658 - content script APIs should be available in windows which points to a web accessible
> +// webextension url and are child of a  window wrapped by a content script related to the same add-on uuid

There usually isn't a need to mention a bug number in comments like this. They're mostly useful in bugs about specific corner cases that people are likely to have questions about. Either way, it's usually better to mention it at the end, rather than the start, e.g., "(see bug 1214658 for rationale)"

@@ +205,5 @@
> +  let principal = window.document.nodePrincipal;
> +  let id = principal.originAttributes.addonId;
> +  let extension = ExtensionManager.get(id);
> +
> +  // do not inject if webextension id do not exist

Please always capitalize the first word of a comment and end with a period.

@@ +213,5 @@
> +
> +  let uri = window.document.documentURIObject;
> +
> +  // do not inject in webextension uri not loadable by anyone
> +  if (!ExtensionManagement.extensionURILoadableByAnyone(uri)) {

We shouldn't be checking this anymore.

@@ +220,5 @@
> +
> +  // get the container element across chrome boundaries
> +  let containerEl = window.QueryInterface(Ci.nsIInterfaceRequestor).
> +        getInterface(Ci.nsIDOMWindowUtils).
> +        containerElement;

This isn't necessary. The |sameTypeParent| checks tell us what we need to know for this.

@@ +230,5 @@
> +
> +  // get the parent window across chrome boundaries
> +  let parentWindow = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIWebNavigation)
> +        .QueryInterface(Ci.nsIDocShellTreeItem).parent

|sameTypeParent|.

You can also just use |window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell).sameTypeParent| to get here.

Also, note that this can be null (which is what we want to check for anyway), so don't try to |QueryInterface| it directly.

@@ +237,5 @@
> +
> +  // do not inject if the iframe's parent window is not wrapped by a content script
> +  // with the same addon id
> +  let contentScriptExtension = DocumentManager.getContext(extension.id, parentWindow);
> +  if (!contentScriptExtension) {

This will always be true. If a context doesn't already exist, one will be created.

I'm also not entirely sure what the point of this check is. Are we trying to restrict this to iframes of pages with content scripts created by a "content_script" manifest directive/tabs.executeScript?

@@ +286,5 @@
> +    // if the context is an iframe created from a content script
> +    // it has content script apis available into the content window
> +    let iframeChromeObj = Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
> +
> +    this.contentWindow.browser = iframeChromeObj;

This is already handled by |createObjectIn|.

@@ +287,5 @@
> +    // it has content script apis available into the content window
> +    let iframeChromeObj = Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
> +
> +    this.contentWindow.browser = iframeChromeObj;
> +    this.contentWindow.chrome = iframeChromeObj;

This shouldn't work without |waiveXrays|.

@@ +296,5 @@
> +    let chromeObj = Cu.createObjectIn(this.sandbox, {defineAs: "browser"});
> +
> +    // Sandboxes don't get Xrays for some weird compatibility
> +    // reason. However, we waive here anyway in case that changes.
> +    Cu.waiveXrays(this.sandbox).chrome = Cu.waiveXrays(this.sandbox).browser;

I know you didn't write this line, but please change |Cu.waiveXrays(this.sandbox).browser| to |chromeObj|.

@@ +306,5 @@
>  ExtensionContext.prototype = {
>    get cloneScope() {
> +    // clone scope is used to clone the manifest object into the
> +    // context the apis is running on (the content window in case of
> +    // a content script privileged iframe)

Please put this comment outside of the getter. Also, capitalize the first word, add a full stop to the end, and leave out the "clone scope" at the start ("Used to clone the manifest...")

@@ +340,5 @@
> +      // like chrome does in case the addon has been removed
> +      // and the iframe is still in the page
> +      for (let apiName in this.contentWindow.browser) {
> +        this.contentWindow.browser[apiName] = undefined;
> +        this.contentWindow.chrome[apiName] = undefined;

These are the same object. No need to do this twice. Also, |delete| would be preferable to assigning |undefined|.

Also, you need to be careful here. The window may be hidden in the bfcache (see https://developer.mozilla.org/en-US/docs/Working_with_BFCache and https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows), which means that |contentWindow| (which is an outer window) may actually point to a different inner window than you expect.

It's also possible for the content script to replace these properties (well, not in this case, since you didn't |unwaiveXrays|, so these aren't actually the properties available to content scripts) with other objects. Possibly objects it doesn't have privileges to access, but this script does. Which means this could lead to privilege escalation issues.

So if we actually need to clear these properties (aside from just nuking the sandbox and therefore the wrappers), we need to keep a safe copy somewhere, or just replace the |chrome| and |browser| globals with empty objects.

::: toolkit/components/extensions/ExtensionManagement.jsm
@@ +185,5 @@
>  this.ExtensionManagement = {
>    startupExtension: Service.startupExtension.bind(Service),
>    shutdownExtension: Service.shutdownExtension.bind(Service),
>  
> +  extensionURILoadableByAnyone:  Service.extensionURILoadableByAnyone.bind(Service),

I'd rather avoid exporting this here if it's not necessary. It's fine if we end up using it, but I don't think we need it for this patch.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch (v3)

In the new version of the patch there are the following changes (in response of the previous review comments and other issues I found by working on it and by running the testcase locally and during try builds):

- move the API privileges level checks into helpers functions (currently exported from ExtensionManagement.jsm, but I'm not exactly sure they are in their better position):
  - getAddonIdForWindow is just a simple helper to get the addon id currently associated to a window less verbosely
  - API_LEVELS in an enum of the available levels: NO_PRIVILEGES, CONTENTSCRIPT_PRIVILEGES and FULL_PRIVILEGES
  - getAPILevelForWindow is the helper which test if the window has an addon id and if is a sub-frame UI or a top level UI and return a API_LEVELS const as its result

- in the Extension.jsm observer, use the getAPILevelForWindow helper to test if we want to inject the FULL_PRIVILEGES APIs (and this test is now moved to the top of the function) 

- in the ExtensionContent.jsm there are the changes needed to inject the content script API into iframes that load a valid webextension urls

Follows a number of issues I found during the work on the last point of this patch and the solution applied in this patch.

## "Sharing API object between the sandbox and contentWindow" issues

### sandbox expanded principal

if the sandbox is created with expanded principal (e.g. [contentWindow])
object created into that sandbox could not be accessed from the contentWindow.

### copy the api object from the sandbox to the contentWindow

It seems that an object copied from the sandbox to the contentWindow
will silently deny access to the callable properties to the javascript code
running in the contentWindow, which is logged from here:

https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#354

### crash running testcase on debug build on cloning the sandbox object into the contentWindow

If instead of copying the object, I clone the object from the sandbox into the
contentWindow, all works fine but on a debug build the Firefox instance which is running
the test case crashes once the cloned method is called after the sandbox is nuked,
and the reason is the assertion at:

Assertion failure: js::GetObjectCompartment(obj) != js::GetContextCompartment(cx) (This should be invoked after entering the compartment but before wrapping the values), at /zfs-tardis/mozlab
/desktop/fx-team/js/xpconnect/src/ExportHelpers.cpp:296

This crash can be workarounded by disabling the cross compartment checks (the "allowCrossOriginArguments" 
options) but I consider it a non-option because it will open it to a much more permissive scenario than 
we want to expose it, so I tried to find a better one.

## Proxied Context Solution

Follows a rationale of the solution applied in the updated patch to solve the above issues:

- when the extension is unloaded we nuke the sandbox, set context.sandbox to null and
  if the currentWindow is still pointed to a webextension page we replace the APIs objects
  with empty objects.

- the APIs object injected in the contentWindow is created (Cu.createObjectIn) in the contentWindow itself
  (and this solve the issues of the "silent denied access" and the crash due to the
  debug assertion)

- The context associated to the APIs object injected in the contentWindow is a proxy
  object which wraps the context and it will present the following differences from the
  plain extension context object:
  
  - return the contentWindow itself as a cloneScope (which solve the issue with the
    "expeanded pricipal" issue and affects only the APIs injected in the contentWindow)

  - raise an exception with a better error message once the sandbox is nuked (which means
    that we don't want the APIs methods to work anymore, even if the javascript code still
    has a reference to the previously existent APIs method)

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch (v3)

This is an update version of the test case and in this new version I've added a bunch of checks related to what happens if we try to call the APIs object methods after the extension is unloaded.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

New patches version pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae25081cee2e

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

This new patch adds a new testcase which checks that the content script APIs are not injected into regular web pages (it is based on the existent one for the background pages and reuses its assets, and this patch renames one of these assets to reflect this sharing)

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

Fixed a small typo in the comments (s/this iframe/this textarea/)

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8699031 [details] [diff] [review]
0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch (v3)

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

This looks good for the most part, but I'm not convinced that your proxy solution is sufficient. If nothing else, it holds the context object alive after the add-on has been shut down.

I still think that creating the API bindings in a sandbox is the best solution. Or, at least, the best short term solution. We can probably do something better from the platform side in the future.

I did some testing, and as long as we create the sandbox with the correct principal, and don't enable X-rays in it, it seems to give the correct behavior. These are the changes I made on top of your patch (they could definitely use a bit more work):

diff --git a/toolkit/components/extensions/ExtensionContent.jsm b/toolkit/components/extensions/ExtensionContent.jsm
--- a/toolkit/components/extensions/ExtensionContent.jsm
+++ b/toolkit/components/extensions/ExtensionContent.jsm
@@ -226,80 +226,60 @@ function ExtensionContext(extensionId, c
                            .getInterface(Ci.nsIDOMWindowUtils);
   let outerWindowId = utils.outerWindowID;
   let frameId = contentWindow == contentWindow.top ? 0 : outerWindowId;
   this.frameId = frameId;
 
   let mm = getWindowMessageManager(contentWindow);
   this.messageManager = mm;
 
-  let prin = [contentWindow];
-  if (Services.scriptSecurityManager.isSystemPrincipal(contentWindow.document.nodePrincipal)) {
-    // Make sure we don't hand out the system principal by accident.
-    prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
+  if (ExtensionManagement.getAddonIdForWindow(this.contentWindow) == extensionId) {
+    // This has the unfortunate side-effect of changing the way we handle
+    // content scripts injected into extension documents. It would probably be
+    // better to create a separate ExtensionContext object for extension
+    // windows, rather than using the one returned by |getContext|.
+    this.sandbox = Cu.Sandbox(contentWindow, {sandboxPrototype: contentWindow, wantXrays: false, isWebExtensionContentScript: true});
+  } else {
+    let prin = [contentWindow];
+    if (Services.scriptSecurityManager.isSystemPrincipal(contentWindow.document.nodePrincipal)) {
+      // Make sure we don't hand out the system principal by accident.
+      prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
+    }
+
+    this.sandbox = Cu.Sandbox(prin, {sandboxPrototype: contentWindow, wantXrays: true, isWebExtensionContentScript: true});
   }
 
-  this.sandbox = Cu.Sandbox(prin, {sandboxPrototype: contentWindow, wantXrays: true, isWebExtensionContentScript: true});
-
   let delegate = {
     getSender(context, target, sender) {
       // Nothing to do here.
     },
   };
 
   let url = contentWindow.location.href;
   let broker = ExtensionContent.getBroker(mm);
   this.messenger = new Messenger(this, broker, {id: extensionId, frameId, url},
                                  {id: extensionId, frameId}, delegate);
 
-  let chromeObj = Cu.createObjectIn(this.sandbox, {defineAs: "browser"});
+  this.chromeObj = Cu.createObjectIn(this.sandbox, {defineAs: "browser"});
 
   // Sandboxes don't get Xrays for some weird compatibility
   // reason. However, we waive here anyway in case that changes.
-  Cu.waiveXrays(this.sandbox).chrome = chromeObj;
+  Cu.waiveXrays(this.sandbox).chrome = this.chromeObj;
 
-  injectAPI(api(this), chromeObj);
+  injectAPI(api(this), this.chromeObj);
 }
 
 ExtensionContext.prototype = {
   get cloneScope() {
     return this.sandbox;
   },
 
   injectAPIIntoContentWindow() {
-    // Do not inject content script APIs if the document loaded has not an addon id.
-    let addonId = ExtensionManagement.getAddonIdForWindow(this.contentWindow);
-    if (!addonId || addonId != this.extensionId) {
-      return false;
-    }
-
-    let contentObj = Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
-    Cu.waiveXrays(this.contentWindow).chrome = contentObj;
-
-    let context= this;
-    let contentContext = new Proxy(this, {
-      get: function(obj, prop) {
-        // Customize the clone scope to the contentWindow
-        if (prop == "cloneScope") {
-          return context.contentWindow;
-        }
-        // Raise exception on trying to use WebExtension APIs from a extension with a nuked sandbox.
-        if (!context.sandbox) {
-          throw context.contentWindow.Error("WebExtension API method called on a destroyed extension context.");
-        }
-
-        return obj[prop];
-      },
-    });
-
-    injectAPI(api(contentContext), contentObj);
-
-    this.contentWindowAPI = true;
-
-    return true;
+    Cu.waiveXrays(this.contentWindow).browser = this.chromeObj;
+    Cu.waiveXrays(this.contentWindow).chrome = this.chromeObj;
   },
 
   execute(script, shouldRun) {
     script.tryInject(this.extension, this.contentWindow, this.sandbox, shouldRun);
   },
 
   callOnClose(obj) {
     this.onClose.add(obj);
@@ -310,23 +290,16 @@ ExtensionContext.prototype = {
   },
 
   close() {
     for (let obj of this.onClose) {
       obj.close();
     }
     Cu.nukeSandbox(this.sandbox);
     this.sandbox = null;
-
-    // Overwrite the content script APIs with an empty object if the APIs objects have been
-    // injected in the content window.
-    if (this.contentWindowAPI && ExtensionManagement.getAddonIdForWindow(this.contentWindow)) {
-      Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
-      Cu.createObjectIn(this.contentWindow, { defineAs: "chrome" });
-    }
   },
 };
 
 function windowId(window) {
   return window.QueryInterface(Ci.nsIInterfaceRequestor)
                .getInterface(Ci.nsIDOMWindowUtils)
                .currentInnerWindowID;
 }

::: toolkit/components/extensions/Extension.jsm
@@ +358,5 @@
> +
> +    let id = ExtensionManagement.getAddonIdForWindow(contentWindow);
> +
> +    // We don't inject privileged apis if the addonid is null
> +    // or doesn't exists.

*exist

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +267,5 @@
> +  injectAPIIntoContentWindow() {
> +    // Do not inject content script APIs if the document loaded has not an addon id.
> +    let addonId = ExtensionManagement.getAddonIdForWindow(this.contentWindow);
> +    if (!addonId || addonId != this.extensionId) {
> +      return false;

This check isn't redundant.

@@ +273,5 @@
> +
> +    let contentObj = Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
> +    Cu.waiveXrays(this.contentWindow).chrome = contentObj;
> +
> +    let context= this;

Missing whitespace around the `=`

::: toolkit/components/extensions/ExtensionManagement.jsm
@@ +204,5 @@
> +// extensionURIToAddonID, which ensures that we don't inject our
> +// API into webAccessibleResources or remote web pages.
> +function getAddonIdForWindow(window) {
> +  let principal = window.document.nodePrincipal;
> +  let id = principal.originAttributes.addonId;

Please just return this rather than storing it in a variable first.

@@ +211,5 @@
> +}
> +
> +const NO_PRIVILEGES = "NO_PRIVILEGES";
> +const CONTENTSCRIPT_PRIVILEGES = "CONTENTSCRIPT_PRIVILEGES";
> +const FULL_PRIVILEGES = "FULL_PRIVILEGES";

I don't think these need to be globals. Keeping them in the API_LEVELS object should be fine. Also, I'd rather these be integers. Using strings will complicate things if we ever need to expose them via XPCOM.

@@ +220,5 @@
> +
> +// Finds the API Level ("FULL_PRIVILEGES", "CONTENTSCRIPT_PRIVILEGES", "NO_PRIVILEGES")
> +// with a given a window object.
> +function getAPILevelForWindow(window) {
> +  let id = getAddonIdForWindow(window);

This is only used once. There's no need to store it in a variable.

@@ +222,5 @@
> +// with a given a window object.
> +function getAPILevelForWindow(window) {
> +  let id = getAddonIdForWindow(window);
> +
> +  // Non WebExtension urls has access to apis

*Non-WebExtension URLs have no access to APIs

Also, please capitalize "API" and "URL" in all comments.

@@ +228,5 @@
> +    return NO_PRIVILEGES;
> +  }
> +
> +  let sameTypeParent = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIDocShell).sameTypeParent;

Nit: I'd rather have a |docShell| variable and check |docShell.sameTypeParent| below. Most people looking at this code will immediately understand what |docShell| means, but |sameTypeParent| is ambiguous.

@@ +230,5 @@
> +
> +  let sameTypeParent = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIDocShell).sameTypeParent;
> +
> +  // WebExtension urls loaded into sub-frame UI has "content script api level privileges"

*have

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

In this updated version of the main test case, I added some steps to ensure that webextension assets which are specified in the "web_accessible_resources" manifest attribute can be loaded in an iframe, but if we try to load a "non web accessible resource" in an iframe then a security error is emitted (as it happens on Chromium, even if the error message is different).

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch (v4)

This updated v4 version of the main patch introduces over the v3 version the following changes from the previous feedback comments:

- various fixes on comments
- define API_LEVELS constants as integers (and define them directly into the object without introducing any new global)
- merge the proposed fix to "copy api object from sandbox to the contentWindow"
  (thank you very much Kris! for debugging the issue and finding which option was preventing the object to be copied correctly from the sandbox to the content window)

In this patch there are at least two minor deviations from the changes proposed in the feedback comments on the previous v3 patch:

- injectAPIIntoContentWindow checks if "ExtensionManagement.getAddonIdForWindow(this.contentWindow) == this.extensionId"
  (I'm not sure if in the previous feedback comments it was pointed as "redundant" or "not redundant") 

- in the "ExtensionContext.prototype.close" method I still create the empty "browser" and "chrome" objects to overwrite the dead wrappers (mainly because I think it helps to generate "less scary" error messages when something tries to access the API after the addon is unloaded)

I'm adding to this patch an "f?" instead of "r?" because I'm going to propose some further changes in a separate patch (to be eventually squashed on this patch if we do like the approach) to handle the issue pointed out by Kris in the previous comment:

"the unfortunate side-effect of changing the way we handle content scripts injected into extension documents. It would probably be better to create a separate ExtensionContext object for extension windows, rather than using the one returned by |getContext|"

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-fixup-Bug-1214658-enable-content-script-APIs-into-su.patch

This is a patch applied on (and it will be then "squashed on" if we want to introduce this kind of changes) the patch from attachment 8702626 [details] [diff] [review] to try to address the following concern: "the unfortunate side-effect of changing the way we handle content scripts injected into extension documents. It would probably be better to create a separate ExtensionContext"

I definitely share the same concern and in this patch I try to:

- be more explicit about which kind of ExtensionContext the object represents (content script which "wrap" a window object or an iframe with content script api enabled)

- prevent the creation of both kind of ExtensionContext for the same window
  (in other words: we will not create content scripts which wraps this kind of iframes)

I'm going to check how Chromium behaves in this scenario, but in the mean time I wanted to share this draft of the patch for an initial feedback.

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

This patch is just a rebase of the previous version, without any changes besides fixing the rebase conflicts on the mochitest.ini file)

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8702626 [details] [diff] [review]
0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch (v4)

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

Sorry, I forgot I owed you this review.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +290,5 @@
>      return this.sandbox;
>    },
>  
> +  injectAPIIntoContentWindow() {
> +    // inject in the content window only if it still has the same addonId of the extension

Capitalize "inject"

@@ +292,5 @@
>  
> +  injectAPIIntoContentWindow() {
> +    // inject in the content window only if it still has the same addonId of the extension
> +    // this ExtensionContext represents.
> +    if (ExtensionManagement.getAddonIdForWindow(this.contentWindow) == this.extensionId) {

This check isn't necessary. The previous checks ensure that it's true.

@@ +321,5 @@
> +    // Overwrite the content script APIs with an empty object if the APIs objects have been
> +    // injected in the content window.
> +    if (ExtensionManagement.getAddonIdForWindow(this.contentWindow) == this.extensionId) {
> +      Cu.createObjectIn(this.contentWindow, { defineAs: "browser" });
> +      Cu.createObjectIn(this.contentWindow, { defineAs: "chrome" });

I still don't think this is necessary, but if we keep it, I'd rather we store a flag saying we injected the API, or check that `unwaiveXrays(contentWindow).browser === this.chromeObj`, rather than checking the principal again.

@@ +383,5 @@
> +      let extensionId = ExtensionManagement.getAddonIdForWindow(window);
> +
> +      if (ExtensionManagement.getAPILevelForWindow(window) == CONTENTSCRIPT_PRIVILEGES &&
> +          ExtensionManager.get(extensionId)) {
> +        let extension = DocumentManager.getContext(extensionId, window);

Please name this |context| rather than |extension| to prevent confusion.

::: toolkit/components/extensions/ExtensionManagement.jsm
@@ +208,5 @@
> +  return principal.originAttributes.addonId;
> +}
> +
> +const API_LEVELS = Object.freeze({
> +  NO_PRIVILEGES: 0, CONTENTSCRIPT_PRIVILEGES: 1, FULL_PRIVILEGES: 2,

Separate lines, please

@@ +217,5 @@
> +function getAPILevelForWindow(window) {
> +  const { NO_PRIVILEGES, CONTENTSCRIPT_PRIVILEGES, FULL_PRIVILEGES } = API_LEVELS;
> +
> +  // Non WebExtension URLs has no access to APIs
> +  if (!getAddonIdForWindow(window)) {

At the moment, we already know the extension ID of the window in both callers, so we should probably either skip this check, or pass the ID we're expecting, and check that it's equal to this.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8702633 [details] [diff] [review]
0004-fixup-Bug-1214658-enable-content-script-APIs-into-su.patch

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +242,5 @@
>      let extensionPrincipal = ssm.createCodebasePrincipal(this.extension.baseURI, {addonId: extensionId});
>      prin = [contentPrincipal, extensionPrincipal];
>    }
>  
> +  if (!isContentScript && ExtensionManagement.getAddonIdForWindow(this.contentWindow) == extensionId) {

If `isContentScript` is false, it seems like `ExtensionManagement.getAddonIdForWindow(this.contentWindow) != extensionId` should be an error.

@@ +281,5 @@
>    Cu.waiveXrays(this.sandbox).chrome = this.chromeObj;
>  
>    injectAPI(api(this), this.chromeObj);
> +
> +  if (!isContentScript) {

It seems a bit strange to do this on a negative condition. `isExtensionPage`, which defaults to false, might be better.

@@ +322,5 @@
>  
>      // Overwrite the content script APIs with an empty object if the APIs objects have been
>      // injected in the content window.
> +    if (!this.isContentScript &&
> +        ExtensionManagement.getAddonIdForWindow(this.contentWindow) == this.extensionId) {

`if (!this.isContentScript)` (or, preferably, `if (this.isExtensionPage)`) would be sufficient.

@@ +386,5 @@
>        let extensionId = ExtensionManagement.getAddonIdForWindow(window);
>  
>        if (ExtensionManagement.getAPILevelForWindow(window) == CONTENTSCRIPT_PRIVILEGES &&
> +          ExtensionManager.get(extensionId) &&
> +          !DocumentManager.hasContext(extensionId, window)) {

I don't think the `hasContext` check is necessary.

@@ +473,5 @@
> +    if (!extensions.has(extensionId)) {
> +      return false;
> +    }
> +
> +    return true;

`return extensions.has(extensionId)`

But I don't think this function is needed.

@@ +495,5 @@
> +    let context = new ExtensionContext(extensionId, window, isContentScript);
> +    extensions.set(extensionId, context);
> +
> +    return context;
> +  },

I don't think this function is necessary. We need to keep track of content script contexts because all content scripts for the same window, from the same add-on, are injected into the same context.

We'll only ever create one of these contexts for the page itself, and we don't really want to store it in the same map as the content script contexts.

It should be enough to just create the context directly, when we need it.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8702635 [details] [diff] [review]
0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

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

Please only rename files using `hg mv`. It would be good to use `hg cp` for files you've copied mostly unchanged, too.

Also, please check your patches using `mach eslint`. This one has a lot of errors (see comments).

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_api_injection.html
@@ +15,5 @@
> +  <!DOCTYPE HTML>
> +  <html>
> +    <head>
> +      <meta charset="utf-8">
> +      <script>

type="text/javascript"

Also, "use strict";

@@ +16,5 @@
> +  <html>
> +    <head>
> +      <meta charset="utf-8">
> +      <script>
> +       const BASE = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest";

Please check that we actually have a content script API here.

@@ +23,5 @@
> +    </head>
> +  </html>
> +</textarea>
> +
> +<script type="application/javascript;version=1.8">

type="text/javascript"

@@ +27,5 @@
> +<script type="application/javascript;version=1.8">
> +"use strict";
> +
> +add_task(function* test_contentscript_api_injection()
> +{

Move brace to previous line

@@ +29,5 @@
> +
> +add_task(function* test_contentscript_api_injection()
> +{
> +  function contentScript() {
> +    var iframe = document.createElement("iframe");

`let`

@@ +40,5 @@
> +      content_scripts: [
> +        {
> +          "matches": ["http://mochi.test/*/file_sample.html"],
> +          "js": ["content_script.js"],
> +          "run_at": "document_end"

Missing comma.

@@ +44,5 @@
> +          "run_at": "document_end"
> +        }
> +      ],
> +      "web_accessible_resources": [
> +        "content_script_iframe.html"

Missing comma.

@@ +45,5 @@
> +        }
> +      ],
> +      "web_accessible_resources": [
> +        "content_script_iframe.html"
> +      ]

Missing comma.

@@ +50,5 @@
> +    },
> +
> +    files: {
> +      "content_script.js": "(" + contentScript + ")()",
> +      "content_script_iframe.html": document.querySelector("#test-asset").textContent

Missing comma.

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

Patch rebased and updated based on feedback.

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch

Patch rebased and updated.

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

Patch rebased and updated.

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8706416 [details] [diff] [review]
0002-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch

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

This is getting very close, but there are still a couple points I'm not sure about.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +249,5 @@
> +    if (ExtensionManagement.getAddonIdForWindow(this.contentWindow) != extensionId) {
> +      throw new Error("Invalid target window for this extension context");
> +    }
> +    // This is an iframe with content script API enabled and its principal should be the
> +    // contentWindow itself. (See Bug 1214658 for rationale)

Please add a comment about why we're waiving x-rays.

@@ +388,5 @@
> +      if (ExtensionManagement.getAPILevelForWindow(window, extensionId) == CONTENTSCRIPT_PRIVILEGES &&
> +          ExtensionManager.get(extensionId)) {
> +        DocumentManager.createExtensionContext(extensionId, window, { isExtensionPage: true });
> +
> +        return;

I'm not sure we want to return here.

@@ +475,4 @@
>      return extensions.get(extensionId);
>    },
>  
> +  createExtensionContext(extensionId, window, contextOptions = {}) {

I still don't think this is right. If we create an extension page first, and then try to inject a content script, then the content scripts get injected into the extension page context (which we probably don't want). If we create a content script context first, and then try to create an extension page context, then it throws.

If we're treating the contexts differently, they shouldn't be sharing the same namespace.

@@ +477,5 @@
>  
> +  createExtensionContext(extensionId, window, contextOptions = {}) {
> +    let extensions = this.getExtensionsForWindow(window);
> +    if (extensions.has(extensionId)) {
> +      throw Error("This window already had an extension context");

`new Error`

::: toolkit/components/extensions/ExtensionManagement.jsm
@@ +245,5 @@
> +    return CONTENTSCRIPT_PRIVILEGES;
> +  }
> +
> +  // WebExtension URLs loaded into top frames UI could have full API level privileges.
> +  return FULL_PRIVILEGES;

I know I mentioned this before, but I think I forgot in subsequent reviews. We also can't inject the full API into windows that aren't running in the main process (or, when we have out-of-process extensions, the main extension process, I guess). Whenever the window is in the content process, we should probably default to injecting the content script API.

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8706415 [details] [diff] [review]
0001-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

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

I'd like to avoid using eval in this test as much as possible. Can you just load an extension script into the page that does most of what you're currently doing with eval?

You should also just be able to call any global functions the script defines on the unwrapped window object.

Aside from that, this looks pretty good.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_create_iframe.html
@@ +121,5 @@
> +      iframeWindow = currentIframeWindow;
> +    }
> +
> +    if (iframeLocation.match(/non_web_accessible\.html$/)) {
> +      ok(false, "non_web_accessible.html should not be loaded in an web page iframe");

Don't worry about testing this. There are already tests elsewhere. It would be easier to skip this whole loop.

@@ +133,5 @@
> +  if (matchSecurityError) {
> +    ok(/non_web_accessible\.html/.test(matchSecurityError[2]), "non web accesible URLs are blocked " +
> +                                                               "and emit the expected security errors");
> +  } else {
> +    ok(false, "expected security error not found");

Same here. Let's skip the console listener entirely.

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch

This patch is an updated version of the main patch which adds the missing feature described in this issue.

Last changes:

- extension pages running in the content process defaults to CONTENT_SCRIPT_PRIVILEGES
- extension pages running into iframes and content scripts that are wrapping windows are now tracked by two different DocumentManager's data structures:
  - "DocumentManager.contentScriptWindows" (Map[windowId -> Map[extensionId -> ExtensionContext]]) tracks the content scripts
  - "DocumentManager.extensionPageWindows" (Map[windowId -> ExtensionContext]) tracks the iframe extension pages
- fix iframe extension page's "close" method when a window is already been destroyed (and it is a dead wrapper object)
- minor fixes (e.g. "Error" -> "new Error", do not return after an iframe extension page has been created for an observed window)

Comment 36

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

This patch is an updated version of the patch that adds the main test case.

Last changes applied (which help to simplify the test case):

- avoid as much "eval" as possible (only the last eval is still there, because if the GET_MANIFEST function is called directly, a "permission denied" exception is raised when we try to convert to string the exception raised from GET_MANIFEST)

- do not test the iframe "non web accessible" security error (already tested elsewhere)

- remove the test assets added to mochitest, added to be able to collect security error log messages and not needed anymore

Comment 37

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

Last patch of the queue (which adds a test case to ensure that content script APIs are not injected in regular webpages by mistake) updated, it is just a rebase.

(and the file rename issues is fixed, I forgot to mention it when I attached the previous version of this patch)

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8709015 [details] [diff] [review]
0001-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +291,5 @@
> +  injectAPI(api(this), this.chromeObj);
> +
> +  // This is an iframe with content script API enabled. (See Bug 1214658 for rationale)
> +  if (isExtensionPage) {
> +    this.isExtensionPage = isExtensionPage;

Please set this property regardless of whether the value is truthy.

@@ +403,5 @@
>        /* eslint-enable mozilla/balanced-listeners */
>      } else if (topic == "inner-window-destroyed") {
> +      let windowId = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
> +
> +      // close any existent content-script context for the destroyed window

Always capitalize the first word of a sentence and include a period. Same below.

@@ +517,5 @@
>      }
>    },
>  
>    shutdownExtension(extensionId) {
> +    // cleaup content-script contexts on extension shutdown

Capitalize. Period. "Clean up"

@@ +528,5 @@
>      }
>  
> +    // cleaup iframe extension page contexts on extension shutdown
> +    for (let [winId, context] of this.extensionPageWindows) {
> +      if (context && context.extensionId == extensionId) {

|context| should never be null here.

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8709019 [details] [diff] [review]
0002-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

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

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_create_iframe.html
@@ +33,5 @@
> +
> +      browser.test.assertFalse(hasExtTabsAPI, "the created iframe should not be able to use privileged APIs (tabs)");
> +      browser.test.assertFalse(hasExtWindowsAPI, "the created iframe should not be able to use privileged APIs (windows)");
> +      browser.test.assertTrue(
> +        JSON.stringify(manifest) == JSON.stringify(chrome.runtime.getManifest()),

|JSON.stringify| doesn't have a defined result format. Please don't rely on it producing the same output for similar objects.

@@ +129,5 @@
> +  }
> +
> +  ok(!manifest, "manifest should be undefined");
> +
> +  const BROWSER_RUNTIME_UNDEFINED_EXCEPTION = "TypeError: ww.browser.runtime is undefined";

Is the constant really helpful?

@@ +137,5 @@
> +  let getManifest;
> +  let getManifestException;
> +
> +  try {
> +    getManifest = ww.eval("GET_MANIFEST()");

How about instead of the eval, you move the try-catch and string coercion into a function in the content script, and have it return the error string?

@@ +145,5 @@
> +
> +  ok(!getManifest, "getManifest should be undefined");
> +
> +  const DESTROYED_EXT_METHOD_EXCEPTION = "TypeError: GET_MANIFEST is not a function";
> +  is(getManifestException && getManifestException.toString(), DESTROYED_EXT_METHOD_EXCEPTION,

Just use |String(getManifestException)|. Same for the above.

Comment 40

[Kris Maglione [:kmag]]

Thanks.

Can you try to use reviewboard for your patches in the future? It's not very easy to check changes across revisions in splinter.

Comment 41

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214658-enable-content-script-APIs-into-sub-fram.patch

Patch updated by applying the suggested tweaks:

- always set this.isExtensionPage in the ExtensionContext from ExtensionContent.jsm
- fixes on comments
- remove "context && ..." because it should never be null

Reapplied the previous r+ flag.

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214658-add-test-case-of-an-webextension-page-if.patch

Patch updated by applying the last suggested tweaks:

- check the actual and expected gecko id from the manifest JSON objects (instead of relying on JSON.stringify)

- removed exception constants (the test code is now simpler and clear enough and
  they are not helpful anymore)

- created a function in the iframe script which "try / catch" around GET_MANIFEST
  and convert the error to a string (and the last window.eval is now gone :-))

- all exceptions are converted to strings using String(...)

Reapplied the r+ flag.

Comment 43

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Kris Maglione [:kmag] from comment #40)
> Thanks.
> 
> Can you try to use reviewboard for your patches in the future? It's not very
> easy to check changes across revisions in splinter.

yeah I'm sorry for that: I always try to put in the attachments' comments as many details as possible about the incremental changes for that exact reason.

Having said that, I agree and I want to switch to mozreview asap.

I'm currently using git and git-cinnabar on m-c, and it seems that mozreview support for git-cinnabar is already in development but it is not ready yet.

Anyway, I'm already practising a bit with mercurial bookmarks, history edit and the record extension (because these three mercurial features/extensions seem to recreate the git features that I most heavily use).

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214658-test-content-script-APIs-are-not-injecte.patch

This patch is just a rebase, no real changes applied over the previous version of the same patch.

Reapplied r+ flag.

Comment 45

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #43)
> Anyway, I'm already practising a bit with mercurial bookmarks, history edit
> and the record extension (because these three mercurial features/extensions
> seem to recreate the git features that I most heavily use).

You'll probably be a lot happier with crecord than record. Although, I find
myself happiest when I can avoid either.

Also, I find the following mercurial config settings help a lot:

[alias]
bm = bookmark
he = histedit -r 'pending'
lg = log -pr 'pending'
show = log -pr
prompt = log --template '[hg:{if(activebookmark, "{activebookmark}", if(fxheads, "{fxheads}", "{tags}"))}]' -r .

[revsetalias]
pending = ancestors(.)&draft()
s($1) = sort($1, -rev)
anc($1) = sort(ancestors($1), -rev)


My patch workflow tends to be something along the lines of:

Change a patch:

<make changes>
hg commit -m amend && hg he
<move changeset to the appropriate patch and use `roll`>


Look at changes:

hg lg

or:

hg log -r pending


Rebase onto integration branch:

hg pull fx-team && hg rebase -r pending -d fx-team && hg out -r . fx-team


I have a fancy bit of zsh code to show the `prompt` output in my prompt when
I'm in a mercurial repo.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/743d7567b280
https://hg.mozilla.org/integration/mozilla-inbound/rev/aee8341f15c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b76ab3324cd2

Comment 47

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2607d226d5 for b2g emulator timeouts, https://treeherder.mozilla.org/logviewer.html#?job_id=20378799&repo=mozilla-inbound

Comment 48

[Phil Ringnalda (:philor)]

Or https://treeherder.mozilla.org/logviewer.html#?job_id=20380011&repo=mozilla-inbound on debug, where it got a little further before tripping over its own feet.

Comment 49

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Phil Ringnalda (:philor) from comment #48)
> Or
> https://treeherder.mozilla.org/logviewer.html#?job_id=20380011&repo=mozilla-
> inbound on debug, where it got a little further before tripping over its own
> feet.

Thanks Phil, I'm looking into it

The other content script test file is currently skipped because b2g doesn't currently support
any value of "runat" different from "document_idle".

The two new test files should work correctly on "document_idle", so I would prefer to do not skip these two new tests and make them run correctly on b2g too.

Here is a push to try of the change described above (which I'm going to attach in a new patch to this issue):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=560e415043df&selectedJob=15854024

Comment 50

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1214658-content-scripts-runat-document_idle-is-n.patch

This patch applies a small change on the content scripts used by the new test cases to make them run on "document_idle" (supported on b2g) instead of "document_end".

Comment 51

[Kris Maglione [:kmag]]

Comment on attachment 8711459 [details] [diff] [review]
0004-Bug-1214658-content-scripts-runat-document_idle-is-n.patch

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

I'm going to land the tests separately from the implementation so this doesn't miss the merge. We can always uplift the tests later, if necessary.

Comment 52

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/0192fb95f348fec522f30abc19ff084b452feb98
Bug 1214658 - Enable content script APIs into sub-frames pointed to a valid add-on url. r=kmag

Comment 53

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/e35801edcd8866eb66c755d0fcca5495d861a560
Bug 1214658 - Add test case of an webextension page iframe created from a content script. r=kmag

https://hg.mozilla.org/integration/fx-team/rev/acde7d2d3ffb77f7a786284ef4d1c1fd0d2d2fe7
Bug 1214658 - Test content script APIs are not injected in arbitrary web pages. r=kmag

Comment 54

[Kris Maglione [:kmag]]

There is a bc3 intermittent failure that's happened 3 times on OS-X opt in your try run: https://treeherder.mozilla.org/logviewer.html#?job_id=15864708&repo=try

I'm hoping it's just a quirk of the build, since it didn't show up for any other build in that run. If it shows up on fx-team, though, I'm going to have to back it out again.

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0192fb95f348
https://hg.mozilla.org/mozilla-central/rev/e35801edcd88
https://hg.mozilla.org/mozilla-central/rev/acde7d2d3ffb

Comment 56

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 44.0a1 (2015-09-22) under Windows 10 64-bit.
Manually verified as fixed on Firefox 46 (20160322075646) beta 4 and Firefox 48.0a1 (2016-03-24) across Windows 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11.

Comment 57

[CosminB]

After I installed the webextension from Description (https://addons.allizom.org/en-US/firefox/addon/test-iframe-injection/) in Firefox 48.0a1 (2016-04-05) on a device with Android 4.2.1 I noticed this red line http://i.imgur.com/M9RQZty.png. Should I file a new bug for this issue?

Comment 58

[Terrell Kelley]

Question: will this result in a (window.)chrome object being available to the page context? There is one in Chrome, and it's how a lot of places do browser sniffing. Sure, some go on and test for chrome.webstore, but not all. It's just so idiomatic to test for a chrome object to detect Chrome.

And, yes, unfortunately, sometimes browser sniffing is still necessary. There are odd idiosyncrasies, like Chrome's slower ContentEditable element painting, that you sometimes need to work around.

Comment 59

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to CosminB from comment #57)
> After I installed the webextension from Description
> (https://addons.allizom.org/en-US/firefox/addon/test-iframe-injection/) in
> Firefox 48.0a1 (2016-04-05) on a device with Android 4.2.1 I noticed this
> red line http://i.imgur.com/M9RQZty.png. Should I file a new bug for this
> issue?

I looked into the above test addon and it seems that is working as expected:
the red line is from the iframe injected into the page (and it is supposed to be present),
besides that, the content script should log the available APIs in the moz-extension iframe in a message
which should be found in the console messages on the tab in which the content script is running,
as in the following screenshot from a Firefox Desktop instance: https://imgur.com/YnCSb2k