This bug is the second piece of the changes needed to provide the userScripts WebExtensions API (tracked by Bug 1437098 and described in more detail on the following API design and planning wiki page: https://wiki.mozilla.org/WebExtensions/UserScripts) and follow up of Bug 1437861 (which will provide the API methods to register/unregister the userScripts). The goal of this bug is to provide the additional userScripts API methods which allow the extension to provide a set of custom API methods to its own userScripts (`userScripts.setAPIScript` and `userScripts.injectAPIs` in the current API design proposal).

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review226790 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:159 (Diff revision 2) > + > + let expectedError; > + > + try { > + US_sync_api(window); > + } catch(err) { Error: Expected space(s) after "catch". [eslint: keyword-spacing]

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review227418 ::: toolkit/components/extensions/ExtensionChild.jsm:1003 (Diff revision 2) > + // Do not generate content_only APIs, unless explicitly allowed. > + if (this.context.envType !== "content_child" && > + allowedContexts.includes("content_only")) { > + return false; > + } This is needed to allow an API to be available to a content script but not to the other extension pages, "content_only" is then used in the user_scripts.json file to restrict the userScripts.injectAPIs method to the content script contexts (I should also add an additional assertion to ensure that the API method is not available in the test extension background page). ::: toolkit/components/extensions/ExtensionContent.jsm:508 (Diff revision 2) > + // Load and execute the API script once per context. > + if (this.promiseAPIScript) { > + const apiScript = await this.promiseAPIScript; > + context.executeAPIScript(apiScript); > + } Here we load and execute the API script in the content script context (if there is any API script url set). ::: toolkit/components/extensions/ExtensionContent.jsm:535 (Diff revision 2) > + // Inject the custom API registered by the extension API script. > + this.injectAPIs(userScriptSandbox, context); > + Once the API script has been executed (if any), it is supposed to have called `browser.userScripts.injectAPIs` to provide a set of custom API methods and so the UserScript class will inject these APIs (if any) in the user script sandbox right before executing it. ::: toolkit/components/extensions/ExtensionContent.jsm:562 (Diff revision 2) > wantXrays: true, > isWebExtensionContentScript: true, > }); > } > + > + injectAPIs(sandbox, context) { The UserScript's injectAPIs is where the custom userScript APIs are wrapped and injected in the sandbox. The custom APIs are actually injected as lazy globals properties, and so the actual API method wrapper is actually generated only if the API method is being used in the userScript. The custom API method wrapper has currently the following responsabilities: - it clones the API method parameters (coming from the userScript sandbox) into the content script sandbox (which also means that non clonable objects, e.g. like a DOM element, can't be passed to an API objet as a parameter) - the API method parameters may contain callbacks function and so these parameters are cloned using the `{cloneFunctions: true}` options - if the API method paramers cloning fails, a SandboxError (the Error constructor coming from the user script Sandbox) is raised - if the custom API method returns a Promise (e.g. the custom API method is an async function), the result is wrapped in a sandbox.Promise, so that it can be accessed by the userScript code - the returned (or the resolved) values are cloned into the userScript sandbox (and a SandboxError is raised if it can't be cloned). ::: toolkit/components/extensions/ext-c-userScripts.js:96 (Diff revision 2) > + injectAPIs(customAPIMethods) { > + context.setUserScriptAPIs(customAPIMethods); > + }, When the API script calls the `userScripts.injectAPIs` method, the customAPIMethods object is stored in the content script context (to be used by the UserScript class once it is going to execute a userScript). Currently if the method is called twice for the same content script context, an ExtensionError is raised as a warning that only the first set of custom API methods are going to be used. ::: toolkit/components/extensions/ext-userScripts.js:138 (Diff revision 2) > + setAPIScript: async ({url}) => { > + extension.userScriptsAPIURL = url; > + await extension.broadcast("Extension:SetUserScriptsAPIURL", { > + id: extension.id, > + url, > + }); > + }, As we planned in the API design, userScripts.setAPIScript will overwrite the last APIScriptURL that has been set. ::: toolkit/components/extensions/extension-process-script.js:629 (Diff revision 2) > preloadContentScript(contentScript) { > - contentScripts.get(contentScript).preload(); > + let APIScript; > + > + if (userScripts.has(contentScript)) { > + // Preload a userScript using its script options. > + APIScript = ExtensionManager.userScriptsAPIURLs.get(contentScript.extension); > + } > + > + contentScripts.get(contentScript).preload(APIScript); > }, > > loadContentScript(contentScript, window) { > if (DocumentManager.globals.has(getMessageManager(window))) { > + if (userScripts.has(contentScript)) { > + // Load a userScript using its script options (and ensure that the extension API script > + // has been preloaded). > + const APIScript = ExtensionManager.userScriptsAPIURLs.get(contentScript.extension); > + if (APIScript) { > + contentScripts.get(contentScript).preload(APIScript); > + } > + } > contentScripts.get(contentScript).injectInto(window); > } > }, Here in preloadContentScript and loadContentScript there is currently a check to verify if the matched script is a userScript and if it is we ensure that the last set API script for the related extension has been preloaded and it has been passed to the userScript (so that it can be executed in the content script context before we execute the userScript). I'm not thrilled by this solution, I'm probably going to look if I can find a nicer way to do it. ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:128 (Diff revision 2) > await contentPage.close(); > > await extension.unload(); > }); > + > +add_task(async function test_userScripts_custom_extension_APIs() { This additional test case verifies that: - the custom API methods set using the userScripts.injectAPIs API method are actually injected and available in the user script sandbox - test that the autogenerated wrappers applied on the custom API methods are making the custom API methods implementation simple enough - test one synchronous and one asynchronous custom API methods - test that an asynchronous custom API method can use the regular WE APIs (e.g. the messaging APIs) ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:193 (Diff revision 2) > + browser.runtime.sendMessage({param}).then(bgPageRes => { > + // eslint-disable-next-line no-undef > + const cbResult = cb(cloneInto(bgPageRes, scriptGlobal)); > + browser.test.sendMessage("US_async_api_with_callback", cbResult); > + }); While testing the usage of the regular browser.runtime.sendMessage API from inside the asynchronous custom API method, I noticed that we have to expose the userScript sandbox object as an additional parameter to allow the extension to clone the object received from the browser.runtime.sendMessage API into the userScript sandbox. It is worth to mention that `cloneInto` and `exportFunction` are both already available to a WebExtension Content Script because when a content script has to pass the result of a browser.runtime.sendMessage API call to the webpage, it has to be explicitly cloned.

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. Hi Shane, this is a draft patch which provide the second piece of the userScripts API: the injection (and wrapping) of the custom API methods. In comment 4 I've added some notes to highlight some parts of the patch (e.g. to highlight some of the new components created for this patch, the parts that are probably going to change, some open questions/issues that I'm going to look into asap).

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review227668 ::: toolkit/components/extensions/ExtensionContent.jsm:501 (Diff revision 3) > return this.createSandbox(window); > }); > } > > + async preload(apiScriptURL) { > + if (apiScriptURL && this.apiScriptURL !== apiScriptURL) { It seems like the api prevents setting this a second time. If we do allow it, what happens with scripts that had the prior api injected? ::: toolkit/components/extensions/ExtensionContent.jsm:762 (Diff revision 3) > > + setUserScriptAPIs(extCustomAPIs) { > + if (this.userScriptAPIs) { > + // TODO: verify that this is the correct behavior when more than > + // one API script is being set in the same extension content script. > + throw new ExtensionError("Custom userScripts API are already been set"); Why not just register it via the manifest? ::: toolkit/components/extensions/extension-process-script.js:634 (Diff revision 3) > preloadContentScript(contentScript) { > - contentScripts.get(contentScript).preload(); > + let APIScript; > + > + if (userScripts.has(contentScript)) { > + // Preload a userScript using its script options. > + APIScript = ExtensionManager.userScriptsAPIURLs.get(contentScript.extension); shouldn't this be contentScript.extension.policy, or get the policy as done for set? ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:178 (Diff revision 3) > + function apiScript() { > + browser.userScripts.injectAPIs({ I'm not really clear on why we would need to do it this way. It would be nice to either have apiScript be the injected script, or let the background script just call userScripts.injectAPIs and drop userScripts.setAPIScript.

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review227668 > It seems like the api prevents setting this a second time. If we do allow it, what happens with scripts that had the prior api injected? That code is actually preventing to execute the userScript "APIScript" twice in the same content script context (e.g. because more than one userScript matches the related webpage). Besides that, my main concern related to allowing to inject multiple "custom APIs implementations" (and the reason why I added some checks to prevent that) is the confusion that it could create, even for the addon developer that may do it (maybe even by mistake while working on their extension, without noticing what was happening until something start to work differently from what the developer expects), and it could also made investigating issues related to APIs overriden by multiple executions of the confusion harder. > Why not just register it via the manifest? "Registering the userScript APIScript url from the manifest.json" is actually an option that I was also thinking of while I was putting together this first version of the patch, and I also think that it is an interesting option (e.g. it would make it way easier to identify which is the file where the custom APIs are actually defined). I ended to don't apply this change yet because I opted to stick to the API design from the wiki for the first version (besides the addition of the userScripts.list API method which wasn't part of the initial API design described on the wiki page), nevertheless I agree that sounds like a good idea and I'm going to give it a try as part of the next round of changes on this patch. > shouldn't this be contentScript.extension.policy, or get the policy as done for set? the contentScript parameter here is a WebExtensionContentScript, and so its extension property is actually the extension policy: - https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/dom/webidl/WebExtensionContentScript.webidl#58 (I totally agree that the property names are not helping to avoid this kind of confusion, I also look more than once to everything that is called extension to be sure, e.g. depending from where it is coming from, it may be: an instance of the Extension class, or of the BrowserExtensionContent class, or a WebExtensionPolicy). > I'm not really clear on why we would need to do it this way. It would be nice to either have apiScript be the injected script, or let the background script just call userScripts.injectAPIs and drop userScripts.setAPIScript. The main reason why I'm not that keen to allow the extension to directly inject its custom APIs from the background page is that it may be suprising to many addon developers (nevertheless, I agree that it would be technically possible, e.g. we could serialize the custom API methods and send them to the content processes like we do in our test suite when we define the test extension). As an example, something that is likely to be confusing: the addon developer defines the custom API methods as functions in the background page, but then the API methods can't access the background page globals directly (like if it was just a regular javascript closure) and on the contrary they have to exchange messages with the background page to achieve it. About dropping the userScripts.setAPIScript, I agree that we can definitely do it by requiring it to be declared in the manifest.json file (like discussed in one of the previous comments). What do you mean with "It would be nice to either have apiScript be the injected script"? (I'm wondering if I'm missing or mis-reading something from your comment, and I want to be sure that I'm not).

(In reply to Luca Greco [:rpl] from comment #8) > Comment on attachment 8951382 [details] > Bug 1437864 - Implement userScripts API methods to allow an extension to > inject custom APIs in the isolated userScripts sandboxes. > What do you mean with "It would be nice to either have apiScript be the > injected script"? > (I'm wondering if I'm missing or mis-reading something from your comment, > and I want to be sure that I'm not). if apiScript were defined in the manifest, and maybe worked a little like a .jsm... exports = ["GM_something"]; function GM_something() {} or exports = ["GMAPI"]; const GMAPI = { something() {}, }; With that, we could internally insert the functions into the user script rather than having the extension manage that. But I think the part that bothered me the most was having to do both setAPIScript and injectAPIs.

I have a question / concern regarding the "scriptMetadata" object that is optionally passed into `.register` and thus passed into the injected APIs. Within the "API Scope" is the "scriptMetadata" object that's passed to the API functions the same object or is it cloned for each individual function? More directly, if I modify the object in one API function does that affect the object for the other functions? For example: background.js > await browser.userScripts.register({ > js, runsAt, matches, > scriptMetadata: { > foobar: 0 > } > }); api.js > function first_api_func(scriptMetadata) { > console.log('foobar is ', scriptMetadata.foobar); > scriptMetadata.foobar = 2; > } > function second_api_func(scriptMetadata) { > console.log('foobar is ', scriptMetadata.foobar); > } script > first_api_func(); > // foobar is 0 > second_api_func(); > // ?> foobar is 2 ? > // ?> foobar is 0 ?

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review255886 ::: toolkit/components/extensions/ExtensionContent.jsm:543 (Diff revision 8) > let sandboxScripts = this.scriptPromises.map(promise => promise.script); > > // If not all scripts are already available in the cache, block > // parsing and wait all promises to resolve. > - if (!sandboxScripts.every(script => script)) { > - const promise = Promise.all(this.scriptPromises); > + if ((this.promiseAPIScript && !apiScript) || !sandboxScripts.every(script => script)) { > + const promise = Promise.all([this.promiseAPIScript].concat(this.scriptPromises)); [this.promiseAPIScript, ...this.scriptPromises] ::: toolkit/components/extensions/ExtensionContent.jsm:622 (Diff revision 8) > `, sandbox); > > return sandbox; > } > + > + injectAPIs(sandbox, context) { lets get a second eye on this, but lgtm ::: toolkit/components/extensions/ExtensionContent.jsm:822 (Diff revision 8) > > + setUserScriptAPIs(extCustomAPIs) { > + if (this.userScriptAPIs) { > + // TODO: verify that this is the correct behavior when more than > + // one API script is being set in the same extension content script. > + throw new ExtensionError("Custom userScripts API are already been set"); Lets get a better string here. Maybe something like: "userScript APIs may only be injected once."

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review258768 ::: toolkit/components/extensions/ExtensionContent.jsm:820 (Diff revision 10) > return this.sandbox; > } > > + setUserScriptAPIs(extCustomAPIs) { > + if (this.userScriptAPIs) { > + // TODO: verify that this is the correct behavior when more than From our discussion I recall we decided that it could only be set once. If we want to consider a change to that down the road, lets have a followup bug. Otherwise, drop the todo.

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. https://reviewboard.mozilla.org/r/220632/#review260518 I didn't go into a detailed code review since the code mostly depends on the previous bug, but other than a few opinions, mostly looks good. Not sure if GM needs this, but should we perhaps allow extension developers to run before every userscript to monkeypatch window/sandbox global properties if needed. This would allow us to keep plain XHR/JSON/... by default and let extension overwrite as needed? ::: toolkit/components/extensions/ExtensionContent.jsm:651 (Diff revision 13) > + function wrapUserScriptAPIMethod(fn) { > + return Cu.exportFunction(function(...args) { > + let fnArgs; > + > + try { > + fnArgs = Cu.cloneInto(args, cloneScope, {cloneFunctions: true}); I'm not sure if this is safe to do, cloning function probably also does getters/setters and prototypes, and this could lead to security exploits if extensions developers are not careful enough. On the other hand, a "removeListener"-like api might not work with this approach, because I think you get a new object every time you wrap a function. Perhaps we could special-case the last argument to each function, and if it's a function, we would keep a WeakMap of cloned wrappers? ::: toolkit/components/extensions/schemas/user_scripts.json:16 (Diff revision 13) > + "properties": { > + "userScripts": { > + "type": "object", > + "optional": true, > + "properties": { > + "apiScript": { "$ref": "manifest.ExtensionURL"} Agree with Shane, this is a much better design. ::: toolkit/components/extensions/schemas/user_scripts.json:47 (Diff revision 13) > "type": "object", > "description": "An opaque set of user script metadata", > "additionalProperties": { "$ref": "ScriptMetadataPropertyType" } > }, > { > + "id": "CustomAPIMethods", bikeshed: "Exported" instead of "Custom" maybe? ::: toolkit/components/extensions/schemas/user_scripts.json:136 (Diff revision 13) > "$ref": "UserScriptOptions" > } > ] > + }, > + { > + "name": "setScriptAPIs", and here, "setExportedAPI" perhaps?

MozReview-Commit-ID: ALNJDiBwZL7

MozReview-Commit-ID: 3GIFhnxMJVn Depends on D4354

Comment on attachment 9002544 [details] Bug 1437864 - Save a copy of the Error and Promise globals from extension context before they can be redefined. Marking as obsolete the previous mozreview request in favor of the patch submitted on Phabricator (attachment 9004294 [details])

Comment on attachment 8951382 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. Marking as obsolete the previous mozreview request in favor of the patch submitted on Phabricator (attachment 9004295 [details]).

Comment on attachment 9004295 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. r=zombie,mixedpuppy! Shane Caraveo (:mixedpuppy) has approved the revision.

Comment on attachment 9004294 [details] Bug 1437864 - Save a copy of the Error and Promise globals from extension context before they can be redefined. r=zombie,mixedpuppy! Shane Caraveo (:mixedpuppy) has approved the revision.

Comment on attachment 9004294 [details] Bug 1437864 - Save a copy of the Error and Promise globals from extension context before they can be redefined. r=zombie,mixedpuppy! Tomislav Jovanovic :zombie has approved the revision.

Comment on attachment 9004295 [details] Bug 1437864 - Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. r=zombie,mixedpuppy! Tomislav Jovanovic :zombie has approved the revision.

Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/7634d9d14449 Save a copy of the Error and Promise globals from extension context before they can be redefined. r=zombie,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/82d60e34a977 Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. r=zombie,mixedpuppy

Backed out 4 changesets (Bug 1437864, Bug 1470466, Bug 1437861) for Android xpcshell failures. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a6185abfc2f8f661ad7e00e546cfa2c7f3933618 Backout link: https://hg.mozilla.org/integration/autoland/rev/da0f39e97a4a468a7f153a59bb4782478636681f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198732851&repo=autoland&lineNumber=2721

See Bug 1437861 Comment 41.

Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/7488bfbbaf8d Save a copy of the Error and Promise globals from extension context before they can be redefined. r=zombie,mixedpuppy https://hg.mozilla.org/integration/autoland/rev/301f7866f783 Implement userScripts API methods to allow an extension to inject custom APIs in the isolated userScripts sandboxes. r=zombie,mixedpuppy

https://hg.mozilla.org/mozilla-central/rev/7488bfbbaf8d https://hg.mozilla.org/mozilla-central/rev/301f7866f783

For the QA verification of this issue, see the test extension and test plans from Bug 1437861 comment 44 (the test extension provide two custom API available to the userScripts, GM_setValue / GM_getValue, to verify this issue as part of the test plans).

Verified as fixed and the tests related to GM_setValue / GM_getValue can be seen in https://docs.google.com/document/d/1GsNvfO4SMQ1BJJwZB03-C60UDQ2AJ01zavSNOxvf-gc/edit .

Note to docs team: I added a note about this change to the Fx 64 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes Again, this should be made a priority in the next sprint.