Closed Bug 1437861 Opened 8 years ago Closed 7 years ago

Implement userScripts.register and execute userScripts js code in isolated sandboxes

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Version:
60 Branch
Type:
enhancement

Tracking

(firefox64 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla64
Iteration:
64.1 - Sep 14
Tracking Flags:
Tracking Status
firefox64 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes.
46 bytes, text/x-phabricator-request
mixedpuppy
: review+
zombie
: review+
Details | Review
user-script-register-example.zip
6.61 KB, application/zip
Details
This bug is the first 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). The goal of this bug is to create the new userScripts API namespace and provide a userScripts.register API method which, similarly to contentScripts.register, allow an extension to register a userScript to be injected into the webpages that it matches. Once a webpage matches one of the registered userScripts, a new isolated sandbox should be created and the userScripts js code evaluated into this sandbox (which should provide access to the window and the document of the matched webpages like for a regular WebExtensions Content Script, but none of the WebExtensions APIs should be directly available in the userScript sandbox). (As an additional side note: to completely implement the userScripts API design, the extension should be able to provide a set of custom APIs to inject into the userScripts isolated sandboxes, which will be part of a follow up of this issue).
Summary: Implement userScript register and execute userScripts js code in isolated sandboxes → Implement userScripts.register and execute userScripts js code in isolated sandboxes
Blocks: 1437098
Assignee: nobody → lgreco
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1437864
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review227402 ::: toolkit/components/extensions/ExtensionContent.jsm:483 (Diff revision 6) > return result; > } > } > > +// Represents a user script. > +class UserScript extends Script { The UserScript class extends Script (which represents a content script) and it has the following responsabilities: - it creates, keep tracks and destroys the isolated sandboxes where the user script code is executed - redefines the `inject` method to: - lazily create the isolated sandbox once needed (and subscribe a close method to nuke the sandbox when the related extension context is closed) - evaluate the userScript code in the isolated sandbox side note: the patch attached to Bug 1437864 extends this class further to also: preload the API content script (if any) and execute it in the content script sandbox (if it has not been executed before in the particular extension context). ::: toolkit/components/extensions/ExtensionContent.jsm:499 (Diff revision 6) > + // TODO: use a separate telemetry key for user script injection? > + TelemetryStopwatch.start(CONTENT_SCRIPT_INJECTION_HISTOGRAM, context); UserScript's inject is currently using the same Telemetry key (CONTENT_SCRIPT_INJECTION_HISTOGRAM) of the regular content script, and so I added a note here related to evaluate to use a separate telemetry key for the user scripts. ::: toolkit/components/extensions/ExtensionContent.jsm:519 (Diff revision 6) > + // TODO: add an url or the errors will be associated to ExtensionContent.jsm. > + Cu.evalInSandbox(this.matcher.jsCode, userScriptSandbox, "latest"); Similarly to one of the patch attached to Bug 1410932 (https://reviewboard.mozilla.org/r/192080/diff/2#index_header), we should add an url as the forth parameter of Cu.evalInSandbox, so that the errors raised by evaluating the js code of a userScript are reported using this url (instead of being wrongly associated to the ExtensionContent.jsm) and the debugger server can recognize these urls and list them (and their content) in the debugger panel. In the meantime I've currently added it as a TODO in this patch (so we can evalute if fixing it in a follow up of in this patch while we evalute the rest of the proposed userScripts API implementation). ::: toolkit/components/extensions/ExtensionContent.jsm:529 (Diff revision 6) > + // TODO: here is where the sandbox options could be customized using the > + // userScript sandboxOptions. The userScript sandboxes are currently configured like the content scripts sandboxes (just the sandboxName is sightly different). I'm going to rephrase the comment to mention that evaluating which kind of configurations we can allow on the userScript sandbox as part of Bug 1437867. ::: toolkit/components/extensions/ext-c-userScripts.js:71 (Diff revision 6) > + scriptsAPICache.set(scriptId, scriptAPI); > + > + return scriptAPI; > + }); > + }, > + list(matchMetadata) { This userScripts.list method isn't part of the API design yet, I've added it because I was looking into how the API would have to be changed if the userScripts registered from userScripts.register are kept registered until the extension is unloaded (disabled or uninstalled), instead of being automatically unregistered when the context from which they have been created is destroyed (like we currently do for the contentScripts.register method). The current userScripts.list behaviors are: - when called without any parameter, it returns an array of userScripts API objects (which, similarly to the contentScript API object, only provides an unregister method). - when called with the matchMetadata parameter, it uses to parameter to filter the registered userScript and returning an array of the userScripts that matched the metadata properties (which currently means that all the matchMetadata properties are 'deepEqual' of userScript metadata properties with the same name) I'm thinking that the userScripts API object should probably also provide the userScript metadata (besides the unregister method), e.g. by providing an additional getMetadata() async method to the userScript API object, so that the extension can use these metadata to recognize which of the userScript the API object represents. ::: toolkit/components/extensions/extension-process-script.js:70 (Diff revision 6) > +// WeakMap of the userScripts options, which associate a WebExtensionContentScript > +// instance, used to match the loaded web pages. with the actual userScript options. > +// WeakMap<WebExtensionContentScript, scriptOptions> > +var userScripts = new WeakMap(); > + > var contentScripts = new DefaultWeakMap(matcher => { > + if (userScripts.has(matcher)) { > + return new ExtensionContent.UserScript(extensions.get(matcher.extension), > + userScripts.get(matcher)); > + } This patch doesn't currently apply any change to the WebExtensionContentScript webidl and c++ class, mostly because I wanted to prototype the rest of the changes first, and so it currently uses a WeakMap to recognize if a given `matcher` is supposed to be a content script or a user script (basically the WebExtensionContentScript is used by the userScripts to detect when a webpage matches, then the actual userScripts options are retrieved by the userScripts WeakMap defined at line 73). Once we have evaluated the rest of the design, I'm going to apply the needed changes to `WebExtensionContentScript` and remove the userScripts WeakMap and its usage from this file. ::: toolkit/components/extensions/schemas/user_scripts.json:46 (Diff revision 6) > + "code": { > + "type": "string" > + }, Currently, in the userScriptOptions there is only a single code (string) parameter which should contain the userScript code (as described in the API design), but while re-evaluating the documentation of the Greasemonkey metadata block I noticed that there is a require metadata property (https://wiki.greasespot.net/Metadata_Block#.40require) which is used by a userScript to list an additional set of js libraries to download and execute right before the userScript (in its sandbox), and so I think that I'm going to add a property named "js" which is an array of code and/or extension urls (like it is supported by the contentScripts.register API). ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:20 (Diff revision 6) > + > +// Test that userScripts sandboxes: > +// - can be registered/unregistered/listed from an extension page > +// - have no WebExtensions APIs available > +// - are able to access the target window and document > +add_task(async function test_userScripts_no_webext_apis() { This is a prelimirary test case, it currently verify that: - the userScripts can be registered, listed and unregistered - the userScript is executed on the matched web page - the userScript can't access the WebExtensions APIs side note: this test file will be further expanded by the patch from Bug 1437864, which will introduce an additional test case to test the custom API injection.
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. Hi Shane, this is a draft patch which provide the first piece of the userScripts API (basically the isolated sandbox, without the custom API methods which are part of Bug 1437864), ready for an initial feedback. In comment 7 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).
Attachment #8950575 - Flags: feedback?(mixedpuppy)
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review227556 I think this is moving along well, I don't have any directional changes, but a few questions. ::: toolkit/components/extensions/Extension.jsm:1366 (Diff revision 6) > dependencies: this.dependencies, > permissions: this.permissions, > principal: this.principal, > optionalPermissions: this.manifest.optional_permissions, > schemaURLs: this.schemaURLs, > + userScripts: new Map(), why a new map rather than this.userScripts? ::: toolkit/components/extensions/ExtensionContent.jsm:488 (Diff revision 6) > +class UserScript extends Script { > + constructor(extension, matcher) { > + super(extension, matcher); > + > + this.scriptMetadata = matcher.userScript.scriptMetadata; > + this.sandboxOptions = matcher.userScript.sandboxOptions; how are we controlling what options may be used? We should have a list of what those are so they can be evaluated in sec-review or something. ::: toolkit/components/extensions/ExtensionContent.jsm:499 (Diff revision 6) > + } > + > + async inject(context) { > + // The evaluations below may throw, in which case the promise will be > + // automatically rejected. > + // TODO: use a separate telemetry key for user script injection? If any TODO items remain when landed (or landing in the other bug), be sure to have "TODO Bug #:" ::: toolkit/components/extensions/ExtensionContent.jsm:532 (Diff revision 6) > + return Cu.Sandbox(window, { > + principal, > + sandboxName: `User Script registered by ${this.extension.policy.debugName}`, > + sandboxPrototype: window, > + sameZoneAs: window, > + wantXrays: true, > + isWebExtensionContentScript: true, > + }); Will we need to do any extra stuff for xhr/etc. as we do for content scripts? originAttributes? ::: toolkit/components/extensions/ext-c-userScripts.js:42 (Diff revision 6) > + return { > + unregister: () => { > + return context.wrapPromise(this.unregister()); > + }, > + }; Could use a comment to specify this object is the "RegisteredUserScript". ::: toolkit/components/extensions/ext-c-userScripts.js:62 (Diff revision 6) > + const registeredScript = new UserScriptChild(context, scriptId); > + const scriptAPI = Cu.cloneInto(registeredScript.api(), context.cloneScope, > + {cloneFunctions: true}); > + > + scriptsAPICache.set(scriptId, scriptAPI); This block repeats for list below, perhaps a helper function should be used. ::: toolkit/components/extensions/extension-process-script.js:70 (Diff revision 6) > +// WeakMap of the userScripts options, which associate a WebExtensionContentScript > +// instance, used to match the loaded web pages. with the actual userScript options. > +// WeakMap<WebExtensionContentScript, scriptOptions> > +var userScripts = new WeakMap(); > + > var contentScripts = new DefaultWeakMap(matcher => { > + if (userScripts.has(matcher)) { > + return new ExtensionContent.UserScript(extensions.get(matcher.extension), > + userScripts.get(matcher)); > + } might be good to toss part of the explanation into a comment if this lives for a while longer. e.g. // This is temporary until prototyping is further along. ::: toolkit/components/extensions/schemas/user_scripts.json:27 (Diff revision 6) > + "type": "object", > + "description": "An opaque set of user script metadata", > + "additionalProperties": { "$ref": "ScriptMetadataPropertyType" } > + }, > + { > + "id": "SandboxOptions", I think we need to clarfiy what these will be, or consider moving them to a followup bug. ::: toolkit/components/extensions/schemas/user_scripts.json:74 (Diff revision 6) > + "type": "array", > + "optional": true, > + "items": { "type": "string" } > + }, > + "allFrames": {"type": "boolean", "optional": true, "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame."}, > + "matchAboutBlank": {"type": "boolean", "optional": true, "description": "If matchAboutBlank is true, then the code is also injected in about:blank and about:srcdoc frames if your extension has access to its parent document. Code cannot be inserted in top-level about:-frames. By default it is <code>false</code>."}, Not sure about the use case for this with user scripts. ::: toolkit/components/extensions/schemas/user_scripts.json:113 (Diff revision 6) > + ] > + }, > + { > + "name": "list", > + "type": "function", > + "description": "List the registered userScripts (optionally filtered by a given matchMetadata object)", "userScripts, optionally". Probably should specificy it returns an array of RegisteredUserScript objects. ::: toolkit/components/extensions/schemas/user_scripts.json:124 (Diff revision 6) > + "$ref": "ScriptMetadata" > + } > + ] > + }, > + { > + "name": "setAPIScript", This should probably be added in the other bug along with the implementation. ::: toolkit/components/extensions/schemas/user_scripts.json:126 (Diff revision 6) > + ] > + }, > + { > + "name": "setAPIScript", > + "type": "function", > + "description": "Set an API Content Script which provides userScripts APIs", Wording is a bit awkward, but not sure I'm doing any better. "Sets a script that provides an API to the user scripts." ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:27 (Diff revision 6) > + const matches = ["http://localhost/*/file_sample.html"]; > + > + const script = await browser.userScripts.register({ > + code: ` > + const webextAPINamespaces = this.browser ? Object.keys(this.browser) : undefined; > + document.body.innerHTML = "userScript loaded: " + JSON.stringify(webextAPINamespaces); refering back to my earlier question...should we monkey with "JSON" for the sandbox as we do regular content scripts?
> Not sure about the use case for [matchaboutblank] with user scripts. I'm not sure either (I've personally never seen it), but Greasemonkey 3.x (legacy) allowed scripts to match on about blank. Just food for thought.
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review227556 > why a new map rather than this.userScripts? This is currently following the same approach of the registeredContentScripts property (which is sightly above this one at line 1356). These userScripts Map is the one that is part of the serialized representation of an extension, which is sent to a new process when it is started (e.g. after a crash of a content process, or when a new content process is started for other reasons). The userScripts is assigned to a new empty Map because (like the registeredContentScripts) they are not part of the manifest.json and so they are always empty until the extension starts and may register some. > how are we controlling what options may be used? We should have a list of what those are so they can be evaluated in sec-review or something. I've currently controlled by the API schema, where is currently marked as "unsupported" (and so the API will raise an error if the property is used by an extension), but I think that I'm going to just remove it completely and move it to a separate patch as also suggested in one of the other feedback comments. (I also already filed an issue for it as Bug 1437867) > If any TODO items remain when landed (or landing in the other bug), be sure to have "TODO Bug #:" +1 currently it is more a question/doubt that I didn't wanted to forget, do you think that this should use a different telemetry key? > Will we need to do any extra stuff for xhr/etc. as we do for content scripts? originAttributes? That's a very good question, I guess that we should take into account, I'm not yet sure if it should be the default or something that is part of that sandboxOptions. I'm going to take a look. > This block repeats for list below, perhaps a helper function should be used. +1 good point. > might be good to toss part of the explanation into a comment if this lives for a while longer. e.g. // This is temporary until prototyping is further along. yeah, good idea.
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review228750 ::: toolkit/components/extensions/schemas/user_scripts.json:27 (Diff revision 6) > + "type": "object", > + "description": "An opaque set of user script metadata", > + "additionalProperties": { "$ref": "ScriptMetadataPropertyType" } > + }, > + { > + "id": "SandboxOptions", I currently marked it as unsupported (line 50) and so it is not allowed as a parameter yet, but I agree that we can definitely remove it from this patch and move it in a follow up. ::: toolkit/components/extensions/schemas/user_scripts.json:74 (Diff revision 6) > + "type": "array", > + "optional": true, > + "items": { "type": "string" } > + }, > + "allFrames": {"type": "boolean", "optional": true, "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame."}, > + "matchAboutBlank": {"type": "boolean", "optional": true, "description": "If matchAboutBlank is true, then the code is also injected in about:blank and about:srcdoc frames if your extension has access to its parent document. Code cannot be inserted in top-level about:-frames. By default it is <code>false</code>."}, Well, I'm not sure about how many user scripts uses or really need it, but it seems that it is something that is supported (or it used to be supported): https://github.com/greasemonkey/greasemonkey/issues/2108 (it is actually an @include for Greasemonkey, but our API design expected that it will be the extension to convert the userScripts `@something` directives into the corresponding WebExtensions options). Besides that, I think that the userScripts API may also be useful for other kind extensions (e.g. an extension that provides some features on its own, but also allow the user to extend these feature by registering userScripts to the extensions) and so I think that we should support most of what is supported by the regular content scripts (at least for those for which we don't have reasons to do otherwise).
Attachment #8950575 - Flags: review?(kmaglione+bmo)
Attachment #8950575 - Flags: review?(kmaglione+bmo)
Attachment #8950575 - Flags: review?(tomica)
Iteration: --- → 62.4 - Jul 2
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review255858 This feels light on tests. I'd like to see some failure cases, such as syntax errors in js, empty file, binary data? I'm not sure about this, but what about multiple userScripts running? Clearing review for more tests, but otherwise this lgtm. ::: toolkit/components/extensions/ExtensionContent.jsm:509 (Diff revision 11) > +// Represents a user script. > +class UserScript extends Script { > + constructor(extension, matcher, userScriptOptions) { > + super(extension, matcher); > + > + this.scriptMetadata = userScriptOptions.scriptMetadata; Add a comment that this is an opaque object the extension provides that is passed to the user script. I also don't see where it is being used...looks like in the patch in bug 1437864. I'd rather this be added in that patch, but can live with it here. ::: toolkit/components/extensions/ExtensionContent.jsm:551 (Diff revision 11) > + sandboxScripts = await promise; > + } > + > + // The evaluations below may throw, in which case the promise will be > + // automatically rejected. > + // TODO: use a separate telemetry key for user script injection? Bug # or resolve before landing, but it seems worthwhile having some indicator for this that it is a user script. ::: toolkit/components/extensions/ExtensionContent.jsm:581 (Diff revision 11) > + } > + > + createSandbox(context) { > + const window = context.contentWindow; > + > + // TODO Bug 1437867: here is where the sandbox options could be customized using the This seems to be more of a design-decision (or internal decision) than a TODO item. If that is the case, lets remove the comment, we have the bug to track it. ::: toolkit/components/extensions/ExtensionContent.jsm:585 (Diff revision 11) > + > + // TODO Bug 1437867: here is where the sandbox options could be customized using the > + // userScript sandboxOptions (e.g. optionally avoid to override the XMLHttpRequest and > + // fetch globals based on the userScript sandboxOptions). > + const contentPrincipal = window.document.nodePrincipal; > + const sandbox = Cu.Sandbox(window, { Should we ask for sec-review on the sandbox? ::: toolkit/components/extensions/ExtensionContent.jsm:597 (Diff revision 11) > + Cu.evalInSandbox(` > + window.JSON = JSON; > + window.XMLHttpRequest = XMLHttpRequest; > + window.fetch = fetch; > + `, sandbox); do we also want content.JSON etc. as we do for content scripts? Or is that part of bug 1437867? ::: toolkit/components/extensions/ext-userScripts.js:27 (Diff revision 11) > + * The options object related to the user script > + * (which has the properties described in the user_scripts.json > + * JSON API schema file). > + */ > +class UserScriptParent { > + constructor(details) { The class doc string above does not match the constructor. ::: toolkit/components/extensions/ext-userScripts.js:50 (Diff revision 11) > + exclude_matches: details.excludeMatches, > + include_globs: details.includeGlobs, > + exclude_globs: details.excludeGlobs, > + all_frames: details.allFrames, > + match_about_blank: details.matchAboutBlank, > + run_at: details.runAt || "document_idle", runAt should default in schema ::: toolkit/components/extensions/ext-userScripts.js:132 (Diff revision 11) > + // that is resolved from the register API method returned promise. > + unregister: async (scriptId) => { > + const userScript = this.userScriptsMap.get(scriptId); > + if (!userScript) { > + Cu.reportError(new Error(`No such user script ID: ${scriptId}`)); > + nit remove blank line ::: toolkit/components/extensions/extension-process-script.js:70 (Diff revision 11) > let extension = new ExtensionChild.BrowserExtensionContent(data); > extension.policy = policy; > return extension; > }); > > +// NOTE: This is temporary until prototyping is further along (this WeakMap can probably Will this be addressed now or later? If later lets get a bug for it. ::: toolkit/components/extensions/extension-process-script.js:80 (Diff revision 11) > + if (userScripts.has(matcher)) { > + let userScriptOptions = userScripts.get(matcher); > + > + return new ExtensionContent.UserScript(extensions.get(matcher.extension), matcher, > + userScriptOptions); > + } > + > return new ExtensionContent.Script(extensions.get(matcher.extension), > matcher); I'd suggest something like: let userScriptOptions = {}; if (userScripts.has(matcher)) { userScriptOptions = userScripts.get(matcher); } return new ExtensionContent.UserScript(extensions.get(matcher.extension), matcher, ::: toolkit/components/extensions/schemas/user_scripts.json:61 (Diff revision 11) > + "allFrames": {"type": "boolean", "optional": true, "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame."}, > + "matchAboutBlank": {"type": "boolean", "optional": true, "description": "If matchAboutBlank is true, then the code is also injected in about:blank and about:srcdoc frames if your extension has access to its parent document. Code cannot be inserted in top-level about:-frames. By default it is <code>false</code>."}, add default: false to these ::: toolkit/components/extensions/schemas/user_scripts.json:63 (Diff revision 11) > + "runAt": { > + "$ref": "extensionTypes.RunAt", > + "optional": true, > + "description": "The soonest that the JavaScript or CSS will be injected into the tab. Defaults to \"document_idle\"." > + } seems this can also use default: document_idle
Attachment #8950575 - Flags: review?(mixedpuppy)
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. Paul, feel free to redirect to someone else. Just wanting a sec review specifically on the Cu.sandbox use.
Attachment #8950575 - Flags: review?(ptheriault)
Product: Toolkit → WebExtensions
Attachment #8950575 - Flags: review?(ptheriault) → review?(dveditz)
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review255858 > Add a comment that this is an opaque object the extension provides that is passed to the user script. I also don't see where it is being used...looks like in the patch in bug 1437864. I'd rather this be added in that patch, but can live with it here. I removed this line from this patch, and moved into the patch attached to bug 1437864 (with the addition of the inline comment as suggested).
Blocks: 1470466
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review255858 > Bug # or resolve before landing, but it seems worthwhile having some indicator for this that it is a user script. Follow up filed as Bug 1470466 (I also defined a new `USER_SCRIPT_INJECTION_HISTOGRAM` const that is already used in the TelemetryStopWatch calls here, and added a TODO inline comment that mention this bugzilla issue number near the const that should be changed to switch this telemetry calls to the new histogram).
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review255858 > do we also want content.JSON etc. as we do for content scripts? Or is that part of bug 1437867? yeah, I would prefer if we could avoid the `content.JSON/XMLHttpRequest/fetch` workaround that we used for the contentScript, if possible. My current idea is that a different behavior could be configured by the extension as part of Bug 1437867 (e.g. some of the `sandboxOptions` may allow the extension to do not override some or all these properties, so that there will be no need for using `content.<something>` as a replacement for the unwanted behavior). > Will this be addressed now or later? If later lets get a bug for it. I opted to remove that comment for now (and rename this weakmap to userScriptOptions, given that it is what we are storing there). For now we only have the opaque scriptMetadata object in there (and in follow ups we may add the sandboxOptions as part of Bug 1437867, and maybe some additional one for the Bug 1445909, but in my opinion Bug 1445909 doesn't have to be "userScripts"-specific).
Attachment #8950575 - Flags: review?(mixedpuppy)
Attachment #8950575 - Flags: review?(kmaglione+bmo)
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review260510 Overall looks like a solid design, and fairly easy to understand code, though I have a bunch of questions and some suggestions for simplification, two main being: - letting devs specify the user script ID (and perhaps leaving it optional, in which case we would generate one and return it, like we do for context menus api). - enable passing code in the form of a blob url -- and perhaps *only* allow blob urls (since we can consider this an "advanced" api, the bar can be a bit higher), which should lower memory usage overall. Each of those changes would simplify multiple things, and both of them together might totally eliminate the need for any code code in `child/ext-userScripts.js`. ::: toolkit/components/extensions/ExtensionContent.jsm:506 (Diff revision 15) > await cssPromise; > return result; > } > } > > +// Represents a user script. JSDoc please. ::: toolkit/components/extensions/ExtensionContent.jsm:514 (Diff revision 15) > + super(extension, matcher); > + > + this.scriptPromises = null; > + > + // WeakMap<ContentScriptContextChild, Sandbox> > + this.sandboxes = new DefaultWeakMap((context) => { I'm don't think each code string/js file in each userScript should get its own sandbox. Isn't the use-case here to load a library before your own code, which would be impossible to take advantage of with this setup. Also, how important is it to have a separate sandbox for each user script (from the same extension)? I'm not familiar and worried about how much overhead each Sandbox is. ::: toolkit/components/extensions/ExtensionContent.jsm:528 (Diff revision 15) > + this.compileScripts(); > + > + 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 we're supposed to inject at the start of the document load, > + // and we haven't already missed that point, block further parsing > + // until the scripts have been loaded. > + const {document} = context.contentWindow; > + if (this.runAt === "document_start" && document.readyState !== "complete") { > + document.blockParsing(promise, {blockScriptCreated: false}); > + } > + > + // Destructure the Promise.all resolved array to strip the api script. > + sandboxScripts = await promise; > + } Please refactor this to a method in the base class instead of duplicating. ::: toolkit/components/extensions/ExtensionContent.jsm:557 (Diff revision 15) > + try { > + context.callOnClose({ > + close: () => { > + // Destroy the userScript sandbox when the related ContentScriptContextChild instance > + // is being closed. > + if (this.sandboxes.has(context)) { When would this not be true? ::: toolkit/components/extensions/ExtensionContent.jsm:568 (Diff revision 15) > + }); > + > + // Lazily create a userScript sandbox for the current target window. > + let userScriptSandbox = this.sandboxes.get(context); > + > + // Evaluate the userScript code in its isolated sandbox. Last three comments might be stating the obvious. :) ::: toolkit/components/extensions/ExtensionContent.jsm:582 (Diff revision 15) > + createSandbox(context) { > + const window = context.contentWindow; > + > + const contentPrincipal = window.document.nodePrincipal; > + const sandbox = Cu.Sandbox(window, { > + principal: [contentPrincipal], I couldn't find the `principal` option anywhere, I believe it's passed as the first parameter directly or QI-d from the passed window: https://searchfox.org/mozilla-central/rev/4074ba/js/xpconnect/src/xpcprivate.h#2754-2761 ::: toolkit/components/extensions/ExtensionContent.jsm:583 (Diff revision 15) > + const window = context.contentWindow; > + > + const contentPrincipal = window.document.nodePrincipal; > + const sandbox = Cu.Sandbox(window, { > + principal: [contentPrincipal], > + sandboxName: `User Script registered by ${this.extension.policy.debugName}`, Could be useful to add user script id to the sandbox name? ::: toolkit/components/extensions/ExtensionContent.jsm:587 (Diff revision 15) > + principal: [contentPrincipal], > + sandboxName: `User Script registered by ${this.extension.policy.debugName}`, > + sandboxPrototype: window, > + sameZoneAs: window, > + wantXrays: true, > + isWebExtensionContentScript: true, I'm not sure what this controls (and if it grants any privileges we don't want), and if we need/want it? ::: toolkit/components/extensions/ExtensionContent.jsm:590 (Diff revision 15) > + sameZoneAs: window, > + wantXrays: true, > + isWebExtensionContentScript: true, > + wantGlobalProperties: ["XMLHttpRequest", "fetch"], > + originAttributes: contentPrincipal.originAttributes, > + }); Doesn't this need addon id in metadata? Or is that only used for the debugger, and we don't want user scripts to show up there? ::: toolkit/components/extensions/ExtensionContent.jsm:596 (Diff revision 15) > + > + // Override the JSON, XMLHttpRequest and fetch globals. > + Cu.evalInSandbox(` > + window.JSON = JSON; > + window.XMLHttpRequest = XMLHttpRequest; > + window.fetch = fetch; Are you sure we need to set these if we are not overriding the defaults? ::: toolkit/components/extensions/child/ext-userScripts.js:13 (Diff revision 15) > + ExtensionError, > +} = ExtensionUtils; > + > +/** > + * Represents (in the child extension process) a user script registered > + * programmatically (instead of being included in the addon manifest). nit: no need to mention user scripts in manifest (unless that is planned) ::: toolkit/components/extensions/child/ext-userScripts.js:19 (Diff revision 15) > + * > + * @param {ExtensionPageContextChild} context > + * The extension context which has registered the user script. > + * @param {string} scriptId > + * An unique id that represents the registered user script > + * (generated and used internally to identify it across the different processes). I think it might be a good idea to allow extensions to specify the script `id`, perhaps optionally (like we do for context menu items). That should simplify a few things throughout this patch. ::: toolkit/components/extensions/child/ext-userScripts.js:57 (Diff revision 15) > + > +this.userScripts = class extends ExtensionAPI { > + getAPI(context) { > + // Cache of the UserScriptChild API objects: > + // Map<scriptId, extensionAPIObject> > + const scriptsAPICache = new Map(); This doesn't seem to be used ever, only set. I don't think you need to hold a reference to keep it alive. ::: toolkit/components/extensions/child/ext-userScripts.js:66 (Diff revision 15) > + const blobURLsByHash = new Map(); > + > + // Convert a scriptId into a RegisteredUserScript API object (and add it > + // to the cached ones). > + function convertToAPIObject(scriptId) { > + const registeredScript = new UserScriptChild(context, scriptId); The whole class is a bit heavy for something simple as: let registeredScriptAPI = { unregister: () => context.childManager.callParentAsyncFunction( "userScripts.unregister", [scriptId]); } ::: toolkit/components/extensions/child/ext-userScripts.js:77 (Diff revision 15) > + return scriptAPI; > + } > + > + // Convert a script code string into a blob URL (and use a cached one > + // if the script hash is already associated to a blob URL). > + const convertStringToBlobURL = async (text, mime) => { nit: `getBlobUrl(text)`, "string" goes into JSDoc if needed, and we're only using one mime type. But more importantly, I'm not sure this is the best optimization regarding blob memory/message passing (I'm still confirming how blob urls work between the parent and multiple content processes). ::: toolkit/components/extensions/child/ext-userScripts.js:78 (Diff revision 15) > + } > + > + // Convert a script code string into a blob URL (and use a cached one > + // if the script hash is already associated to a blob URL). > + const convertStringToBlobURL = async (text, mime) => { > + // Compute the hash of the js code string and we already have a blob url for it. nit: I think you accidentally a word. ::: toolkit/components/extensions/child/ext-userScripts.js:93 (Diff revision 15) > + const blob = new context.cloneScope.Blob([text], {type: mime}); > + blobURL = context.cloneScope.URL.createObjectURL(blob); > + > + blobURLsByHash.set(hash, blobURL); > + > + return blobURL; This all looks like way too much bookkeeping just to avoid asking devs to pass code as a blob url. :/ ::: toolkit/components/extensions/child/ext-userScripts.js:98 (Diff revision 15) > + return blobURL; > + }; > + > + // Revoke all the created blob urls once the context is destroyed. > + context.callOnClose({ > + close: async () => { Nothing async here, but also `callOnClose` doesn't await listeners. ::: toolkit/components/extensions/child/ext-userScripts.js:104 (Diff revision 15) > + if (!context.cloneScope) { > + return; > + } > + > + for (let blobURL of blobURLsByHash.values()) { > + context.cloneScope.URL.revokeObjectURL(blobURL); Shouldn't we be revoking blob URLs this in `.unregister` as well? ::: toolkit/components/extensions/child/ext-userScripts.js:111 (Diff revision 15) > + }, > + }); > + > + return { > + userScripts: { > + register(options) { `userScripts.create` and `.remove` would probably be more consistent naming with other existing apis, so it's unfortunate we already shipped `contentScripts.register`. We should add a simple `.unregisterAll` instead of going into the weeds with `.list` and similar. ::: toolkit/components/extensions/child/ext-userScripts.js:118 (Diff revision 15) > + options.js = await Promise.all(options.js.map(js => { > + if (js.file) { > + return js.file; > + } > + > + return convertStringToBlobURL(js.code, "text/javascript"); nit: maybe just `return js.file || getBlobUrl(js.code)` ::: toolkit/components/extensions/extension-process-script.js:84 (Diff revision 15) > +// WeakMap of the userScripts options, which associates a WebExtensionContentScript > +// instance (used to match the loaded web pages) with the actual userScript options > +// (e.g. the opaque scriptMetadata object): > +// > +// WeakMap<WebExtensionContentScript, UserScriptOptions> > +var userScriptsOptions = new WeakMap(); I think you might not need this, see below. ::: toolkit/components/extensions/extension-process-script.js:89 (Diff revision 15) > +var userScriptsOptions = new WeakMap(); > + > var contentScripts = new DefaultWeakMap(matcher => { > - return new ExtensionContent.Script(extensions.get(matcher.extension), > - matcher); > + const extension = extensions.get(matcher.extension); > + > + // Create a UserScript instance if the matcher is related to a userScript. nit: probably unnecessary comment ::: toolkit/components/extensions/extension-process-script.js:388 (Diff revision 15) > registeredContentScripts.set(scriptId, script); > + > + // If the script is a userScript, store the additional properties > + // in the userScripts Weakmap. > + if (options.userScriptOptions) { > + userScriptsOptions.set(script, options.userScriptOptions); I believe you can just set script.userScriptOptions = options... since `script` is a js wrapper (reflector) around a webidl implemented thing, you can just stick custom properties on it (see `policy.debugName` above for example). ::: toolkit/components/extensions/parent/ext-userScripts.js:5 (Diff revision 15) > +/* exported registerUserScript, unregisterUserScript */ > +/* global registerUserScript, unregisterUserScript */ What's this for? ::: toolkit/components/extensions/parent/ext-userScripts.js:49 (Diff revision 15) > + exclude_globs: details.excludeGlobs, > + all_frames: details.allFrames, > + match_about_blank: details.matchAboutBlank, > + run_at: details.runAt, > + js: details.js, > + userScriptOptions: { Let's keep the same naming style for the whole object, `user_script_options` and such. Also, we seems to be converting this back into camelCase later, WTH :( https://searchfox.org/mozilla-central/rev/d29662/toolkit/components/extensions/extension-process-script.js#434 ::: toolkit/components/extensions/parent/ext-userScripts.js:78 (Diff revision 15) > + // Set of the scriptIds registered from this context. > + const registeredScriptIds = new Set(); > + > + // Unregister all the scriptId related to a context when it is closed, > + // and revoke all the created blob urls once the context is destroyed. > + context.callOnClose({ What is the reason to unregister user scripts when the context goes away? That's a pretty surprising api if called from an extension page. If the point is to cleanup when the extension goes away, we probably want `onUninstall`. Also, no blob urls are getting revoked here. ::: toolkit/components/extensions/parent/ext-userScripts.js:87 (Diff revision 15) > + for (let scriptId of registeredScriptIds) { > + extension.registeredContentScripts.delete(scriptId); > + this.userScriptsMap.delete(scriptId); > + } > + > + await context.extension.broadcast("Extension:UnregisterContentScripts", { This is the only "async" thing in this callback, and awaiting it on the last line has no effect. And besides, `callOnClose` doesn't await listeners. ::: toolkit/components/extensions/parent/ext-userScripts.js:99 (Diff revision 15) > + return { > + userScripts: { > + register: async (details) => { > + for (let origin of details.matches) { > + if (!extension.whiteListedHosts.subsumes(new MatchPattern(origin))) { > + throw new ExtensionError(`Permission denied to register a user script for ${origin}`); What about globs? ::: toolkit/components/extensions/parent/ext-userScripts.js:128 (Diff revision 15) > + // the extension code will call script.unregister on the script API object > + // that is resolved from the register API method returned promise. > + unregister: async (scriptId) => { > + const userScript = this.userScriptsMap.get(scriptId); > + if (!userScript) { > + Cu.reportError(new Error(`No such user script ID: ${scriptId}`)); This should probably throw instead. ::: toolkit/components/extensions/parent/ext-userScripts.js:132 (Diff revision 15) > + this.userScriptsMap.delete(scriptId); > + extension.registeredContentScripts.delete(scriptId); > + > + userScript.destroy(); > + > + await extension.broadcast("Extension:UnregisterContentScripts", { > + id: extension.id, > + scriptIds: [scriptId], > + }); nit: this is (mostly) repeated, should probably extract into a function (that takes an array of script IDs, so as to only send one message like above). ::: toolkit/components/extensions/schemas/user_scripts.json:10 (Diff revision 15) > +[ > + { > + "namespace": "userScripts", > + "types": [ > + { > + "id": "ScriptMetadataPropertyType", This is basically `PlainJSONValue` right? We probably already have something similar in our schemas, or if not, let's name it that, and maybe move it to `extension_types.json`? ::: toolkit/components/extensions/schemas/user_scripts.json:21 (Diff revision 15) > + { "type": "array", "items": {"$ref": "ScriptMetadataPropertyType"} }, > + { "type": "object", "additionalProperties": { "$ref": "ScriptMetadataPropertyType" } } > + ] > + }, > + { > + "id": "ScriptMetadata", I don't think we need this indirection, since it's opaque metadata, we can just accept `PlainJSON` type, even if they pass just a number or a string. ::: toolkit/components/extensions/schemas/user_scripts.json:31 (Diff revision 15) > + { > + "id": "UserScriptOptions", > + "type": "object", > + "description": "Details of a user script", > + "properties": { > + "js": { I understand it's not in scope of the original design, but we should totally implement support for CSS. It's not part of Greasemonkey, but userStyles is a big use-case we could support with minimal additions to this (can be in a followup). ::: toolkit/components/extensions/schemas/user_scripts.json:36 (Diff revision 15) > + "js": { > + "type": "array", > + "optional": true, > + "description": "The list of JS files to inject", > + "minItems": 1, > + "items": { "$ref": "contentScripts.ExtensionFileOrCode" } Shouldn't this accept blobs/blob urls as well? ::: toolkit/components/extensions/schemas/user_scripts.json:65 (Diff revision 15) > + }, > + "allFrames": { > + "type": "boolean", > + "default": false, > + "optional": true, > + "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame." nit: this is referencing JS and CSS instead of user scripts. ::: toolkit/components/extensions/schemas/user_scripts.json:100 (Diff revision 15) > + ], > + "functions": [ > + { > + "name": "register", > + "type": "function", > + "description": "Register a user script programmatically given its $(ref:userScripts.UserScriptOptions)", Please add that this returns `RegisteredUserScript` (unless you go with my proposal of returning the user script id). (also _sigh_ that we don't have schema support to define return type for async methods, I really need to fix that) ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:17 (Diff revision 15) > +server.registerDirectory("/data/", do_get_file("data")); > + > +const BASE_URL = `http://localhost:${server.identity.primaryPort}/data`; > + > +// Test that userScripts sandboxes: > +// - can be registered/unregistered/listed from an extension page nit: no listing in this patch (and hopefully ever) ::: toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js:81 (Diff revision 15) > + let extension = ExtensionTestUtils.loadExtension(extensionData); > + > + // Ensure that a content page running in a content process and which has been > + // already loaded when the content scripts has been registered, it has received > + // and registered the expected content scripts. > + let contentPage = await ExtensionTestUtils.loadContentPage(`about:blank`); This doesn't sound right. The (matched) content page will be loaded _after_ user scripts have been registered.
(In reply to Tomislav Jovanovic :zombie from comment #27) > > + // Convert a script code string into a blob URL (and use a cached one > > + // if the script hash is already associated to a blob URL). > > + const convertStringToBlobURL = async (text, mime) => { > > But more importantly, I'm not sure this is the best optimization regarding > blob memory/message passing (I'm still confirming how blob urls work between > the parent and multiple content processes). I consulted with Andrea, at it looks like our api should: 1) accept BLOBs (not blob urls), as that leads the least amount of IPC/memory duplication if the code comes from storage or similar 2) and if we do accept strings, we should probably be sending them to the parent process for keeping. https://mozilla.logbot.info/content/20180702#c14962522-c14962587
(In reply to Tomislav Jovanovic :zombie from comment #27) > > + "properties": { > > + "js": { > > I understand it's not in scope of the original design, but we should totally > implement support for CSS. It's not part of Greasemonkey, but userStyles is > a big use-case we could support with minimal additions to this (can be in a > followup). Actually the existing contentScripts api might already cover this use-case, as userStyles probably doesn't need any advanced handling like sandboxes for userScripts.
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review260510 > - letting devs specify the user script ID (and perhaps leaving it optional, in which case we would generate one and return it, like we do for context menus api). > - enable passing code in the form of a blob url -- and perhaps only allow blob urls (since we can consider this an "advanced" api, the bar can be a bit higher), which should lower memory usage overall. I don't disagree, especially given that they were both part of the initial design choices I was proposing for the contentScripts API, e.g. as it can be seen in the example API snippet that is part of Bug 1332273 comment 28: ``` // Create a new blob URL which provides the updated ruleset const ruleSetBlob = new Blob([`const ruleSet = ${JSON.stringify(ruleSet)};`], {type: "text/javascript"}); const ruleSetURL = URL.createObjectURL(ruleSetBlob); // Register the update content script composed of the updated rule set and the common (static) content script. const contentScriptId = await browser.tabs.registerContentScript({ js: [ruleSetURL, "/adblock_content_script.js"], matches: ["<all_urls>"], run_at: "document_start", }); ``` but the design has been then changed quite a bit in response to the review rounds. Given how similar are the contentScripts API and the userScripts API (and also how much of the internal changes introduced for the contentScripts API are actually leveraged by the userScripts API) I'm a bit concerned of presenting an API that differs too much from the one exposed by the contentScripts API. > JSDoc please. Definitely, I'm not even sure how it is possible that it is not already there :-| > I'm don't think each code string/js file in each userScript should get its own sandbox. Isn't the use-case here to load a library before your own code, which would be impossible to take advantage of with this setup. > > > Also, how important is it to have a separate sandbox for each user script (from the same extension)? I'm not familiar and worried about how much overhead each Sandbox is. So, this was also one of the scenario that I've been thinking about, and so let me provide some context about how I opted to handle this scenario. In my opinion: - a userScript is the set of js code which includes: the userScript source and any js file he may depends on (e.g. a userScript may depend from a bunch of js libraries) - each userScript should run in its own isolated sandbox (separate from each other and from the sandbox used by regular content scripts that the extension may have registered) - a js library that is used by more than one userScript still needs to be evaluated separately in each userScript sandbox, because it is actually a different instance of the same library and it shouldn't share any state across the userScripts that uses it - I would prefer to defer to move all the actual js code that a userScript requires as much as possible, basically until the userScripts is actually matching an actual webpage (so that we don't use memory unnecessarily) - it would be good to cache the precompiled version of each js code entry separately, so that a js library used by more than one userScripts is going to be precompiled once and then shared by all the ones that need it To cover the above criteria I was initially thinking of allowing the extension to register the shared libraries explicitly from a separate API method, but then I opted for the following more implicit approach: - every userScript js code string (every plain js code string that is part of the `js` array) is converted internally into a Blob url (this way we avoid to send this strings across the processes until they are actually used at least once) - we also compute an hash for the js code string, so that if a js code string is shared by more than one userScript registered from the same extension, then we reuse the existent Blob url (this way we are sure of reusing the precompiled version of the same js code, because they share the same Blob url) > When would this not be true? Actually, it has been quite some time from when I've created this patch, I'll take a look to be sure of that (but to be fair, at a first glance I would agree with you that it doesn't seem actually possible). > I couldn't find the `principal` option anywhere, I believe it's passed as the first parameter directly or QI-d from the passed window: > > https://searchfox.org/mozilla-central/rev/4074ba/js/xpconnect/src/xpcprivate.h#2754-2761 Ouch, yeah you are definitely right, that is definitely a mistake (and a related missing test which would have caught it). > Could be useful to add user script id to the sandbox name? Not super helpful on its own, given that the scriptId is never exposed to the extension, but it will be useful to give the sandbox a different name from each other. And so, good point! I'm going to add it. > I'm not sure what this controls (and if it grants any privileges we don't want), and if we need/want it? That's a good point, I took a brief look at where it is actually used and it seems that these are the only two places where it is actually took into account: - https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/js/xpconnect/wrappers/XrayWrapper.cpp#747 - https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/js/xpconnect/wrappers/XrayWrapper.cpp#515 (a bit ironically) one of those comments says: "WebExtensions can't use cloneInto(), so we just let them do the slow thing to maximize compatibility.", which I guess that was true before "Bug 1280482 - Add export helpers to content scripts." which have actually added it (https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/toolkit/components/extensions/ExtensionContent.jsm#571), but it may be still be true for the userScripts if we do not provide the export helpers in that sandboxes. and so I'm not yet sure, but it is definitely something to double-check again before this patch can be considered ready. > Doesn't this need addon id in metadata? Or is that only used for the debugger, and we don't want user scripts to show up there? Good point, at the time I deferred to verify that the userScripts were already debuggable, to get a round of feedback/review first (just to be sure that the approach was good enough before making the patch even bigger), but it is definitely one of the things that I'm going to look into and add an explicit test case. > Are you sure we need to set these if we are not overriding the defaults? That should only make a difference (and it should only be needed) if case of an expanded principal. Nevertheless, it seems a good point to me, it sounds better to remove this part and than in a follow up override those that are needed (and if needed) based on the sandboxOptions (as I was also mentioning in comment 25). > nit: no need to mention user scripts in manifest (unless that is planned) ouch, actually I think that this part of that comment is actually wrong (probably coming from the inline comment from the analogous class used internally from the contentScripts API). > I think it might be a good idea to allow extensions to specify the script `id`, perhaps optionally (like we do for context menu items). > > That should simplify a few things throughout this patch. Like describe above in much more detail, I'm a bit worried of exposing APIs for the userScripts and contentScripts namespaces that differs too much. But I'm not clearing this issue, until we have a final decision about it. > This doesn't seem to be used ever, only set. I don't think you need to hold a reference to keep it alive. In a previous version of this patch there was a `list` API method, I'm wondering if this is just something that I forgot to remove when I removed that method. I'll take a look. > The whole class is a bit heavy for something simple as: > > let registeredScriptAPI = { > unregister: () => > context.childManager.callParentAsyncFunction( > "userScripts.unregister", [scriptId]); > } At a first glance, that seems fair to me, I'm going to take a look on removing the class as you are suggesting in the next round of updates on this patch. > nit: `getBlobUrl(text)`, "string" goes into JSDoc if needed, and we're only using one mime type. > > But more importantly, I'm not sure this is the best optimization regarding blob memory/message passing (I'm still confirming how blob urls work between the parent and multiple content processes). The only thing that we immediately sends across the processes (in the worst case, with the extensions oop mode enabled, "child extension process" -> "main process" -> "child content process") is the string of the blob url. The actual data should then be loaded in the "child content process" when the userScript matches a webpage for the first time, and then the precompiled script should be cached in that process and nothing should be actually copied across the processes. > nit: I think you accidentally a word. yeah, I think that it was meant to be "Compute the hash of the js code string and <<check if>> we already have a blob url for it". > This all looks like way too much bookkeeping just to avoid asking devs to pass code as a blob url. :/ This is actually to: - avoid that a blob url that is used in a userScript (or more of a single userScript) may be invalidated by the extension while it is still part of a registered userScript - avoid to have multiple separate blob urls for the same js code string (which the extension may or may not decide to reuse) More details about the design choices around this are in one of the above comments. > Shouldn't we be revoking blob URLs this in `.unregister` as well? yes, I think you are right, at least if that blob URLs is not shared by multiple blob URLs. > `userScripts.create` and `.remove` would probably be more consistent naming with other existing apis, so it's unfortunate we already shipped `contentScripts.register`. > > We should add a simple `.unregisterAll` instead of going into the weeds with `.list` and similar. Those sound like good points to me, we may still add those to the contentScripts API (and log a warning when the old ones are used instead of removing them, for a nicer transition of the existing extension that may be using it). I'll keep this issue open, to evaluate if we want to provide API methods with names that are also used by other WebExtensions API. > I believe you can just set > > script.userScriptOptions = options... > > since `script` is a js wrapper (reflector) around a webidl implemented thing, you can just stick custom properties on it (see `policy.debugName` above for example). That's a pretty good idea, I like it, I'm going to apply the proposed change in the next round of updates. > What's this for? Doesn't seem to be needed, likely part of a previous version (a pretty old version) of this patch. > Let's keep the same naming style for the whole object, `user_script_options` and such. > > > Also, we seems to be converting this back into camelCase later, WTH :( > > https://searchfox.org/mozilla-central/rev/d29662/toolkit/components/extensions/extension-process-script.js#434 well, definitely a good point. And yeah, if I recall correctly we are currently converting between camelCase and non camelCase because the content scripts use the "underscore" style and the tabs.executeScript API uses the "camelCase one", and that's a bit annoying and confusing. > What is the reason to unregister user scripts when the context goes away? That's a pretty surprising api if called from an extension page. If the point is to cleanup when the extension goes away, we probably want `onUninstall`. > > Also, no blob urls are getting revoked here. These are the reasons why I opted for this behavior (and I hope that I'm not forgetting any other reason): - to match the behavior of the contentScripts API - to avoid to implement some kind of list method (because without a script API object returned by the register method, the extension can't unregister the userScript/contentScript, and the script API object is clearly tied to the context where the script has been registered) - and because we use a Blob constructor that is tied to the extension context that has registered the content script. If I recall correctly (and I'm not wrong), the blob urls for the Blob object that are being nuked are going to be invalidated when the related context has been destroyed (and that is the reason why they are not explicitly revoked in that case). > What about globs? If I recall correctly `MatchPatternSet` should cover also those, but given that it is important that this line is going to block invalid/unallowed behaviors, I'll make sure that we have a more explicit test case for this. > Shouldn't this accept blobs/blob urls as well? That was something that an initial version of the contentScripts.register API was allowing, but than we decided to avoid that during the last review rounds. I'm not clearing this issue yet, but as I also wrote in the previous comments, given that we opted to avoid it in the contentScripts.register API I'm not sure that it is a good idea to allow it in the userScripts API.
Iteration: 62.4 - Jul 2 → 63.4 - Aug 20
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. https://reviewboard.mozilla.org/r/219858/#review269616
Attachment #8950575 - Flags: review?(mixedpuppy) → review+
This patch introduces the userScripts API namespace and the userScripts.register API method, which allows an extension to register some javascript code to run on the matched webpages into an isolated sandbox (whereas all the content scripts from the same extension run in a per-window sandbox shared by all the extension content scripts).
Comment on attachment 8950575 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. Marking as obsolete the previous mozreview request. The new patch submitted to Phabricator (attachment 9004288 [details]) has been rebased and adapted on top of the changes introduced by Bug 1484373.
Attachment #8950575 - Attachment is obsolete: true
Comment on attachment 9004288 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. Shane Caraveo (:mixedpuppy) has approved the revision.
Attachment #9004288 - Flags: review+
Comment on attachment 9004288 [details] Bug 1437861 - Implement userScripts.register and execute userScripts js code in isolated sandboxes. Tomislav Jovanovic :zombie has approved the revision.
Attachment #9004288 - Flags: review+
Iteration: 63.4 - Aug 20 → 64.1 (Sep 14)
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/a92b6e069d7e Implement userScripts.register and execute userScripts js code in isolated sandboxes. r=zombie,mixedpuppy
Eh, the tests are not actually executed because with the addition head_user_input_handling.js head file the xpcshell command becomes longer and adb fails with the pretty cryptic error "service name too long" (it looks that on bugzilla we have a similar bug reported but related to an old B2G python script: Bug 889747): https://treeherder.mozilla.org/logviewer.html#?job_id=198861131&repo=try&lineNumber=2800 I'm going to fold head_use_input_handling.js into the main head.js, which would bring back the resulting adb command to its original (and allowed) length.
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/f151c2fa525f Implement userScripts.register and execute userScripts js code in isolated sandboxes. r=zombie,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/f151c2fa525f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
See Also: → 1491051
Depends on: 1491388
Attached file user-script-register-example.zip (obsolete) —
The attached test extension provides an extension sidebar from which we can customize the userScript parameters and register it. When the test extension registers a new userScript, the previous registered userScript is unregistered first (if any). The test extension also provides two custom API methods: GM_getValue (which retrieves a value from the storage.local API on behalf of the userScript that calls the API method) and GM_setValue (which sets a value in the storage.local API). (These test extension and the test plans are pretty similar to the ones from Bug 1332273 comment 95, which we used to verify the contentScripts.register API. The test plan and the test extension are being tweaked to test the features that these two APIs have in common, and the new features that are only available in the userScripts APIs). Follows the STRs. Main scenario: - Unpack the test extension from the attached zip file - Install the test extension temporarily from about:debugging#addons - open the extension sidebar (if it has not been opened already) - click the "Register script" button to register a user script using the default values for its properties. - open a new tab and load https://mozilla.org/en-US/ Expected behaviors: - The tab should load and the the user script should change its content with the message: "This page has been eaten: {"newStoredValue": "https://mozilla.org/en-US"} - open the webconsole for the tab and the following message should have been logged: - "API SCRIPT EXECUTED: custom userScripts APIs defined" (coming from customUserScriptAPIs.js) - "USER SCRIPT EXECUTING on https://www.mozilla.org/en-US/" (coming from the "blob:moz-extension://..." url, can be verified by hovering the link on the right side with the mouse) this message should also contain an object with the following properties: - documentReadyState (which is usually "interactive" or "completed" when runAt is document_idle) - GM_getValue (coming from customUserScriptAPIs.js, related to the first GM_getValue call in the userScript) this message should also contain an object with the following properties: - name: "testkey" (as the first parameter of the GM_getValue call from the user script) - res (the value read from the storage.local API, initially undefined, then set by the call to GM_setValue) - scriptMetadata (which should contain the scriptMetadata.name from the registered content script, "my-registered-contentscript" by default) - GM_setValue (coming from customUserScriptAPIs.js, related to the GM_setValue call in the user script) - name: "testkey" (as the first parameter of the GM_setValue call from the user script) - value (the url on the current page, as the second parameter of the GM_setValue call from the user script) - GM_getValue (coming from customUserScriptAPIs.js, related to the second GM_getValue call in the userScript) - open a new tab and load https://developer.mozilla.org/ Expected behaviors: - The tab should load and the the user script should change its content with the message: This page has been eaten: {"oldStoredValue": "https://mozilla.org/en-US", "newStoredValue": "https://developer.mozilla.org/en-US/"}" - open the webconsole for the tab and check that the expected console messages have been logged. ---- Additional userScripts matching scenarios: - Scenario 02: the user script is not executed when it doesn't match a page url Expected behaviors: - navigate to mozilla.org => The user script is executed as expected - navigate to developer.mozilla.org => The user script is executed as expected - navigate to example.com => The user script is NOT executed and the script message is NOT rendered in the tab - Scenario 03: matches and exclude matches options hosts: *://*.org/* excludeMatches: *://developer.mozilla.org/* Expected behaviors: - navigate to mozilla.org => The user script is executed - navigate to developer.mozilla.org => The user script is NOT executed - Scenario 04: matches, include globs and exclude globs hosts: *://*.org/* includeGlobs: */en-US/* excludeGlobs: */en-US/firefox/* Expected behaviors: - navigate to mozilla.org/en-US => The user script is executed - navigate to mozilla.org/es-US/firefox/ => The script message is NOT executed - Scenario 05: runAt = "document_end" repeat the main scenario and the other scenario from 01 to 03 with runAt set to document_end (In the messages logged in the tab webconsole, documentReadyState should be "completed") - Scenario 06: runAt = "document_start" repeat the main scenario and the other scenario from 01 to 03 with runAt set to document_start (In the messages logged in the tab webconsole, documentReadyState should be "loading") - Scenario 07: matchAboutBlank=true hosts: *://*.org/* matchAboutBlank: true runAt: document_idle / document_end code: document.body.innerHTML = '<h1>This page has been eaten<h1>' Expected behaviors: - open a new tab and navigate to "about:blank" => The script message is rendered - open a new tab and navigate to "about:robots" => The script message is NOT rendered - Scenario 08: allFrames=true hosts: <all_urls> allFrames: true code: console.log("Script injected into", window.location.href); runAt: document_idle / document_end Expected behaviors: - navigate to youtube.com and open the developer tools => The expected message is logged multiple times (once for the top level webpage, and more times for the iframe included in the youtube webpage)
(In reply to Luca Greco [:rpl] from comment #44) > Created attachment 9009561 [details] > user-script-register-example.zip > > The attached test extension provides an extension sidebar from which we can > customize the userScript parameters and register it. > When the test extension registers a new userScript, the previous registered > userScript is unregistered first (if any). > The test extension also provides two custom API methods: GM_getValue (which > retrieves a value from the storage.local API on behalf of the userScript > that calls the API method) and GM_setValue (which sets a value in the > storage.local API). > > (These test extension and the test plans are pretty similar to the ones from > Bug 1332273 comment 95, which we used to verify the contentScripts.register > API. The test plan and the test extension are being tweaked to test the > features that these two APIs have in common, and the new features that are > only available in the userScripts APIs). > > Follows the STRs. > > Main scenario: > - Unpack the test extension from the attached zip file > - Install the test extension temporarily from about:debugging#addons > - open the extension sidebar (if it has not been opened already) > - click the "Register script" button to register a user script using the > default values > for its properties. > - open a new tab and load https://mozilla.org/en-US/ > > Expected behaviors: > > - The tab should load and the the user script should change its content > with > the message: > "This page has been eaten: {"newStoredValue": > "https://mozilla.org/en-US"} > - open the webconsole for the tab and the following message should have > been logged: > - "API SCRIPT EXECUTED: custom userScripts APIs defined" (coming from > customUserScriptAPIs.js) > - "USER SCRIPT EXECUTING on https://www.mozilla.org/en-US/" (coming from > the "blob:moz-extension://..." url, can be verified by hovering the link on > the right side with the mouse) > this message should also contain an object with the following > properties: > - documentReadyState (which is usually "interactive" or "completed" > when runAt is document_idle) > - GM_getValue (coming from customUserScriptAPIs.js, related to the first > GM_getValue call in the userScript) > this message should also contain an object with the following > properties: > - name: "testkey" (as the first parameter of the GM_getValue call from > the user script) > - res (the value read from the storage.local API, initially undefined, > then set by the call to GM_setValue) > - scriptMetadata (which should contain the scriptMetadata.name from > the registered content script, > "my-registered-contentscript" by default) > - GM_setValue (coming from customUserScriptAPIs.js, related to the > GM_setValue call in the user script) > - name: "testkey" (as the first parameter of the GM_setValue call from > the user script) > - value (the url on the current page, as the second parameter of the > GM_setValue call from the user script) > - GM_getValue (coming from customUserScriptAPIs.js, related to the > second GM_getValue call in the userScript) > > - open a new tab and load https://developer.mozilla.org/ > > Expected behaviors: > > - The tab should load and the the user script should change its content > with > the message: > This page has been eaten: {"oldStoredValue": > "https://mozilla.org/en-US", "newStoredValue": > "https://developer.mozilla.org/en-US/"}" > - open the webconsole for the tab and check that the expected console > messages have been logged. > > ---- > > Additional userScripts matching scenarios: > > > - Scenario 02: the user script is not executed when it doesn't match a > page url > Expected behaviors: > - navigate to mozilla.org => The user script is executed as expected > - navigate to developer.mozilla.org => The user script is executed as > expected > - navigate to example.com => The user script is NOT executed and the > script message is NOT rendered in the tab > > - Scenario 03: matches and exclude matches options > hosts: *://*.org/* > excludeMatches: *://developer.mozilla.org/* > > Expected behaviors: > - navigate to mozilla.org => The user script is executed > - navigate to developer.mozilla.org => The user script is NOT executed > > - Scenario 04: matches, include globs and exclude globs > hosts: *://*.org/* > includeGlobs: */en-US/* > excludeGlobs: */en-US/firefox/* > > Expected behaviors: > - navigate to mozilla.org/en-US => The user script is executed > - navigate to mozilla.org/es-US/firefox/ => The script message is NOT > executed > > - Scenario 05: runAt = "document_end" > repeat the main scenario and the other scenario from 01 to 03 with runAt > set to document_end > (In the messages logged in the tab webconsole, documentReadyState should > be "completed") > > - Scenario 06: runAt = "document_start" > repeat the main scenario and the other scenario from 01 to 03 with runAt > set to document_start > (In the messages logged in the tab webconsole, documentReadyState should > be "loading") > > - Scenario 07: matchAboutBlank=true > hosts: *://*.org/* > matchAboutBlank: true > runAt: document_idle / document_end > code: document.body.innerHTML = '<h1>This page has been eaten<h1>' > > Expected behaviors: > - open a new tab and navigate to "about:blank" => The script message > is rendered > - open a new tab and navigate to "about:robots" => The script message > is NOT rendered > > - Scenario 08: allFrames=true > hosts: <all_urls> > allFrames: true > code: console.log("Script injected into", window.location.href); > runAt: document_idle / document_end > > Expected behaviors: > - navigate to youtube.com and open the developer tools => > The expected message is logged multiple times (once for the top > level webpage, and more times for the iframe included in the youtube webpage) Performed all the tests suggested above and added the results in https://docs.google.com/document/d/1GsNvfO4SMQ1BJJwZB03-C60UDQ2AJ01zavSNOxvf-gc/edit . Please have a look at FAILED results and let me know if we should log new issues for them. Thanks, Victor
Flags: needinfo?(lgreco)
After discussing the issues found with Luca, the conclusion is that the results marked as failed were expected so I will mark this bug as verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(lgreco)
Fix on the test extension (the extension should clear the reference to the last registered script after it has been unregistered, otherwise the test extension is going to raise an exception because it tries to unregister the same script twice and the new script will not be registered). Thanks to Victor for catching this bug in the test extension.
Attachment #9009561 - Attachment is obsolete: true
Note to the docs team: I have added a note to cover this in the Fx 64 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes When we come to document this, we need to do a fair bit of work because we've got nothing on userScripts so far, from what I could see. We should make this a priority in the next sprint.
Note on error text language: 1542382271859 addons.webextension.****@*** WARN Loading extension '****@***': Reading manifest: Error processing permissions.8: Value "userScripts" must either: must either [must either [be one of ["clipboardRead", "clipboardWrite", "geolocation", "idle", "notifications"], be one of ["bookmarks"], be one of ["find"], be one of ["history"], be one of ["menus.overrideContext"], be one of ["search"], be one of ["activeTab", "tabs", "tabHide"], be one of ["browserSettings"], be one of ["cookies"], be one of ["downloads", "downloads.open"], be one of ["topSites"], be one of ["webNavigation"], or be one of ["webRequest", "webRequestBlocking"]], be one of ["alarms", "mozillaAddons", "storage", "unlimitedStorage"], be one of ["browsingData"], be one of ["devtools"], be one of ["identity"], be one of ["menus", "contextMenus"], be one of ["pkcs11"], be one of ["geckoProfiler"], be one of ["sessions"], be one of ["contextualIdentities"], be one of ["dns"], be one of ["management"], be one of ["privacy"], be one of ["proxy"], be one of ["nativeMessaging"], be one of ["telemetry"], be one of ["theme"], or match the pattern /^experiments(\.\w+)+$/], or must either [be one of ["<all_urls>"], must either [match the pattern /^(https?|wss?|file|ftp|\*):\/\/(\*|\*\.[^*/]+|[^*/]+)\/.*$/, or match the pattern /^file:\/\/\/.*$/], or match the pattern /^resource:\/\/(\*|\*\.[^*/]+|[^*/]+)\/.*$|^about:/]
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #48) > Note to the docs team: > > I have added a note to cover this in the Fx 64 rel notes: > https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/ > 64#API_changes Chris, sorry for not mentioning it explicitly earlier, in Firefox 64 the userScripts API is behind a preference, and it is disabled by default (mainly because we are still finalizing the API provided, while it is still experimental and we don't need to keep it backward-compatible). And so in the release notes it would be probably better to mention that the API is experimental (and the API could change) and that it is disabled by default (or we may remove it from the release notes it you think that it shouldn't be part of the release notes until the API is finalized). > When we come to document this, we need to do a fair bit of work because > we've got nothing on userScripts so far, from what I could see. We should > make this a priority in the next sprint. Let's wait a bit to document the API in detail, we are going to apply the changes to the exposed API pretty soon.
Flags: needinfo?(cmills)
(In reply to erosman from comment #49) > Note on error text language: > > 1542382271859 addons.webextension.****@*** WARN Loading extension > '****@***': Reading manifest: Error processing permissions.8: Value > "userScripts" must either: ... There isn't a userScripts permission, the API namespace becomes available when the extension manifest includes a "user_scripts" section (similarly to the browserAction API). You may want to look to the small test extension in attachment 9015282 [details] (but keep in mind that, as mentioned in comment 50, we are working on some changes to the API).
> Chris, sorry for not mentioning it explicitly earlier, in Firefox 64 the userScripts API is behind a preference, and it is disabled by default (mainly because we are still finalizing the API provided, while it is still experimental and we don't need to keep it backward-compatible). Thanks Luca. This is all fine; I've removed it from the rel notes for now, as it is procedure that we don't mention features there until they are enabled in release. > Let's wait a bit to document the API in detail, we are going to apply the changes to the exposed API pretty soon. OK, good to know. When do you expect this land in release?
Flags: needinfo?(cmills)
At this moment, we are targeting 65 for this (this assumes that it passes the testing in beta while still pref'd off). If it misses in 65, then 66.

Is this ready to document now or should I wait until the next sprint?

Flags: needinfo?(lgreco)

(In reply to Irene Smith from comment #54)

Is this ready to document now or should I wait until the next sprint?

I briefly chatted with :ddurst about this and we agreed that we should wait,
mainly because this API is still disabled by default on non-Nightly builds.

Flags: needinfo?(lgreco)

dev-doc-needed flag moved to Bug 1514809.

Keywords: dev-doc-needed

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #55)

I briefly chatted with :ddurst about this and we agreed that we should wait,
mainly because this API is still disabled by default on non-Nightly builds.

Can this ticket be closed? The API has been enabled by default since FF68, per Bug 1514809.

(In reply to arthaey from comment #57)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #55)

I briefly chatted with :ddurst about this and we agreed that we should wait,
mainly because this API is still disabled by default on non-Nightly builds.

Can this ticket be closed? The API has been enabled by default since FF68, per Bug 1514809.

This bugzilla issue is already closed, it is marked as VERIFIED/FIXED (verified means that is has also manually verified by QA).

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

Attachment

General

Creator:
Created:
Updated:
Size: