Closed Bug 1332273 Opened 8 years ago Closed 7 years ago

Implement a contentScripts.register API

Categories

(WebExtensions :: Untriaged, defect, P1)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(firefox53 wontfix, firefox57+ wontfix, firefox59 verified, firefox60 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox57 + wontfix
firefox59 --- verified
firefox60 --- verified
Blocking Flags:
webextensions +

People

(Reporter: bugzilla.mozilla.org, Assigned: rpl)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved] triaged)

Attachments

(3 files, 2 obsolete files)

Bug 1332273 - Support programmatically registered content scripts.
59 bytes, text/x-review-board-request
kmag
: review+
baku
: review+
Details
user-script-extended-example.xpi
5.03 KB, application/x-xpinstall
Details
Bug1332273.gif
680.43 KB, image/gif
Details 
Scripts running during `document-start` that want to ensure they can apply some user-configrable behavior before any page script runs and before the page is painted cannot wait for asynchronous messaging or storage APIs. They need to be passed some initial set of options which can be changed from the background page or panel. In case there is no guarantee that background pages load before the first tab does this would also have to be persisted across restarts so there's no race between background script initialization and content scripts reading that data.

I think currently the only way to achieve something like that is to use synchronous XHRs and intercepting them with webrequest, which is pretty ugly and not great for load times.

The addon-sdk provides `contentScriptOptions`[1], bootstrapped extensions have many ways to achieve this such as preloading the options in a per-process JSM, reading preferences, using `nsIContentProcessMessageManager.initialProcessData` (which makes this 0-RTT even during startup) or using synchronous messages.

bug 1323433 proposes a superset of this but has been deemed low-prio due to complexity. So one-way API to preload data for content script might be simpler to implement.

[1] https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts#Passing_configuration_options
Whiteboard: [design-decision-needed] triaged
This would be hugely useful to NoScript, even if it could be polyfilled on non supporting platforms with an obvious performance and complexity cost.
Blocks: webext-port-noscript
Approved; P2. Put on tracking schedule for 57.
Flags: needinfo?(amckay)
webextensions: --- → ?
Flags: needinfo?(amckay)
Priority: -- → P2
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
(In reply to The 8472 from comment #0)
> In case there is no guarantee that background pages load before the first tab
> does this would also have to be persisted across restarts so there's no race
> between background script initialization and content scripts reading that data.
There is no such guarantee.  Additionally, content scripts are only loaded after the background page is done, so you are not guaranteed to be able to run your code before the page's scripts anyway, and must code with that in mind (luckily, only an issue for the first tab in practice).

> I think currently the only way to achieve something like that is to use
> synchronous XHRs and intercepting them with webrequest, which is pretty ugly
> and not great for load times.
Even this is not guaranteed to work for the first tab, as all the webRequest.*.addListener methods are async anyway, and could end up missing the requests at startup.

> bootstrapped extensions have many ways to achieve this such as preloading the
> options in a per-process JSM, reading preferences, using `.initialProcessData`
> (which makes this 0-RTT even during startup) or using synchronous messages.
All of these necessarily affect the startup performance (of the content process), and it could be especially bad if we allow unbound extension data, which can for some addons (ad/tracker/xx blockers) measure in megabytes.

Because of this concern, my current plan is to only guarantee sync data availability with some sane limit on data size (something like 4k per extension).  Any extension that needs more than that could check for availability of the config data in sync, with an event as a fallback.  (Again, this would only be an issue for the first tab loaded per process, as we would obviously preload/cache the data for subsequent tabs).

So the API would look something like:

  from content script:
    browser.runtime.configData;   // ready only property 
    browser.runtime.onConfigDataAvailable.addListener(...);

  from background page:
    browser.runtime.setConfigData(value);
(In reply to Tomislav Jovanovic :zombie from comment #4)
> (In reply to The 8472 from comment #0)

> Even this is not guaranteed to work for the first tab, as all the
> webRequest.*.addListener methods are async anyway, and could end up missing
> the requests at startup.
> 
This sounds incredibly bad for any adblocker and, of course, NoScript.
Are you sure of this? If so, this *must* be fixed ASAP.
Flags: needinfo?(tomica)
> Because of this concern, my current plan is to only guarantee sync data
> availability with some sane limit on data size (something like 4k per extension).

Arbitrary limits like this make for seemingly broken features.  For Greasemonkey, I can't guarantee that (the sum of all installed) scripts' source are any particular size.  As a user, I'd prefer a little slowness to features I depend on sometimes working and sometimes not.

Alternately: if there's a circumstance in which I (any extension author) can guarantee a tiny thing works, but not other things, I'm likely to apply a really dirty hack like: notice that I'm in a start-up case and can only run 4k of code, make that small code "block for a while and reload the page" so the document_start event happens again, then be outside of the startup case and able to make my feature work.  Which is obviously worse than just letting me pass more data to begin with.  The user experience will still be that start up takes a long time, but it's likely to be even worse than just letting me pass some data directly.
> For Greasemonkey, I can't guarantee that (the sum of all installed) scripts' source are any particular size. 

In my opinion scripts or similarly large data should not be stored in such configuration storage as strings. Blobs (if structured clone is allowed) or Blob URIs would be far better. Blob URIs can load their content into the process lazily thus avoiding the bloat and they can use shared memory.

We need a way to eval scripts from URIs synchronously, like loading a <script> tag, but in the sandbox of an addon or in terms of a ES6 Realm[1][2] instead of the content page.
The easiest approach here would be to expose the subscript loader[3] to addons in some way.

I have put some effort into loading things from URIs in greasemonkey and it improved performance and reduced footprint. I would hate to see webextensions undoing those gains.

[1] http://www.ecma-international.org/ecma-262/6.0/#sec-code-realms
[2] https://gist.github.com/dherman/7568885
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader#loadSubScript()
> This sounds incredibly bad for any adblocker and, of course, NoScript.
> Are you sure of this? If so, this *must* be fixed ASAP.

Agree, for Greasemonkey, this will break features.  (From the user's perspective: sometimes things work, sometimes (first tab?) things don't work or break in confusing ways.)

> In my opinion scripts or similarly large data should not be stored in such configuration storage as strings.

Probably off-topic so please email me directly, but: what option do I have under WebExtension besides string?  AFAICT I only get storage.local, which only stores strings, and I can only talk to content processes with sendMessage() (and alikes) which only send strings?

I'd like to be able to store (in background) a blob, later get (only) its URL and pass that URL (synchronously) to content script(s), but I know of no way to do any of those things.
(In reply to Giorgio Maone [:mao] from comment #5)
> This sounds incredibly bad for any adblocker and, of course, NoScript.
> Are you sure of this? If so, this *must* be fixed ASAP.

From my understanding and from bug 1343940 comment #3, that is the case, at least theoretically.  However, I'm not sure if this is ever an issue in practice.

(Also, from my limited understanding how *blockers work, they already must account for situations like this because of a number of hack techniques that web pages employ).


(In reply to Anthony Lieuallen from comment #6)
> Arbitrary limits like this make for seemingly broken features.  

I agree the number is arbitrary, but any limit would be.  It is not ideal, but if the only alternative is unbound amount of data (from unbound number of extensions), I don't think that would be acceptable when firefox starts using 4-8 processes, considering both memory and performance implications.

> Greasemonkey, I can't guarantee that (the sum of all installed) scripts'
> source are any particular size.  As a user, I'd prefer a little slowness to
> features I depend on sometimes working and sometimes not.

Content scripts already don't guarantee that your code will be executed when you specify, so if you don't account for that, it already works only sometimes.

> I'm likely to apply a really dirty hack [...] Which is obviously worse 
> than just letting me pass more data to begin

Not necessarily.  If I have userscripts enabled for only a few domains (common?), then slowing down *every* process load on the odd chance that the first tab in the process is one of those is not better IMO.
Flags: needinfo?(tomica)
webextensions: ? → +
Assignee: nobody → lgreco
4K also isn't nearly enough for my own use-case either. I need to be able to specify each frame's initial cookie, localStorage, indexedDB, etc state before their own script(s) run, and cookies alone can be 4K.

I'm not against a small default limit if the extension could specify a permission allowing for unlimited use (or a very high limit). I should be able to keep the config data clear except in the cases where I absolutely need to pass data to each frame.

However, I am concerned that this would not be a useful API for my case, where each frame needs to be sent specific information (not just each top frame). Would set config data be able to target a specific tabId and frameId? Otherwise, I worry that this approach would race or have locking issues to resolve (since my background script could try to setConfigData for a new frame before the old one manages to read the onConfigData intended for it).

Because of these concerns I'm not sure why it wouldn't be better (at least for my use-case) to simply have an webNavigation.onBeforePageScriptsRun(tabId,frameId) type of event where I can insertScript(frameId), and seed an arbitrary amount of data using window.wrappedJSObject in that script.
Flags: needinfo?(tomica)
Comment on attachment 8854479 [details]
Bug 1332273 - Implement browser.contentScriptOptions API.

The attached patch contains a prototype of the API mostly as described by :zombie in Comment 4:

- the browser.tabs.setContentScripOptions({...}) method, provided only in the privileged extension pages (e.g. the background page, options ui page and popup pages), allows an extension to broadcast a javascript object (an object can be serialized to string and sent to the child processes) to all the child processes.

- the browser.runtime.contentScriptOptions getter, provided only in the content scripts, allows to a content script to synchronously retrieve the object value that has been set.

The property of the browser.runtime.contentScriptOptions property is not persisted across browser restarts and it is cloned when provided to a content script that retrieve the property value (and so every content script running in a process receives a different copy and the changes applied to the cloned value are not shared between different content scripts)

Once received by a child process, the broadcasted data is stored in a property of the initialProcessData, and so I'm a bit concerned as :zombie about how this transient storage can be used (and abused in some cases) by an extension (e.g. would be reasonable to pre-serialize the object into a string and refusing to broadcast an object bigger than a certain amount of data? or remove all the contentScriptOptions stored in the initialProcessData on an memory-pressure notification? should we turn the contentScriptOptions getter into a lazy getter and then return the value cloned when it is accessed for the first time?).


Let me know what you think about it.
Attachment #8854479 - Flags: feedback?(tomica)
Attachment #8854479 - Flags: feedback?(mixedpuppy)
Attachment #8854479 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8854479 [details]
Bug 1332273 - Implement browser.contentScriptOptions API.

https://reviewboard.mozilla.org/r/126418/#review128920

The proposed API is bad for performance, because it allows only one object per extension, shared globally with all content scripts.

In real-world use cases (e.g. rule-based content modifiers such as ad blockers or user script managers), the set of rules can easily grow to multiple megabytes of data. The current API proposal does not allow for future optimizations because of its global nature.

At the very least, extensions should be allowed to assign options to multiple keys. Add-ons can use this to access information in a more efficient way (access a small subset of all options without cloning megabytes of data).

This API is "powerful" because it can significantly affect the performance of Firefox, and therefore it should be locked behind a manifest key or permission. This enables easy static analysis of manifest.json (a highly structured document) to assist in identifying potential performance issues.
At the bottom of https://bugzil.la/1323433, I proposed the `browser.contentScriptOptions` API, which is extensible and auditable by design. If you think that the API looks nice, you can adopt parts of it and the internal implementation will look largely similar to yours (ignore the "matches" and "tabId" parameters in the `browser.contentScriptOptions.set` method for now and the API behaves identical to yours).

::: browser/components/extensions/schemas/tabs.json:910
(Diff revision 1)
>              "parameters": []
>            }
>          ]
>        },
>        {
> +        "name": "setContentScriptOptions",

The `tabs` namespace is not the best place for this API. `runtime` or a new namespace such as `contentScriptOptions` (see my notes preceding the review) sounds like a better candidate.

::: toolkit/components/extensions/ExtensionContent.jsm:123
(Diff revision 1)
> +function getProcessContentScriptOptions(extensionId) {
> +  let optionsMap = Services.cpmm.initialProcessData["Extension:ContentScriptOptions"] || {};
> +  return optionsMap[extensionId];
> +}
> +
> +function clearProcessContentScriptOptions(...args) {

Why are you using `...args` instead of an explicitly named `extensionId` parameter?

And AFAICS, `clearProcessContentScriptOptions` is never called without arguments, so the `args.length` branch is never taken.

::: toolkit/components/extensions/ext-c-runtime.js:102
(Diff revision 1)
>          getURL: function(url) {
>            return extension.baseURI.resolve(url);
>          },
> +
> +        get contentScriptOptions() {
> +          return Cu.cloneInto(extension.contentScriptOptions, context.cloneScope);

`browser.runtime.contentScriptOptions === browser.runtime.contentScriptOptions` is false with this implementation.

When writing documentation, make sure to document that the value of `browser.runtime.contentScriptOptions` should be stored in a local variable before reading properties for performance reasons.


Or change the implementation to be a function, which "feels" heavier than a getter and may deter add-on devs from carelessly killing performance by ignorance.
If you follow my request to have key-value pairs, then you need a method anyway.
See again the sample code at the bottom of https://bugzil.la/1323433.
(In reply to Rob Wu [:robwu] from comment #14)

Thanks a lot for the very helpful feedback Rob, these are the same concerns that I have on this API (some of them are exactly the one that I mentioned in Comment 13).

The changes to the API  that you suggest sound reasonable to me (and they were definitely in the options that I had in mind), I will proceed with this changes in the meantime.
Blocks: 1334174
Another thought is to create browser.storage.memory.  storage already has onchanged.  memory would be a key/value store.  already has a well defined api, but maybe could be modified to have more synchronous reads.

But, if you want to go down the other road...

From the top level documentation on browser.runtime and browser.extension, it seems like browser.extension is the more appropriate place for the api:

runtime:

This module provides information about your add-on and the environment it's running in.

vs. extension:

Utilities related to your add-on. Get URLs to resources packages with your add-on, get the Window object for your add-on's pages, ** get the values for various settings **.

I'm also not clear on why we would limit access to the config to content scripts.  I can see potential use for this in other areas.  Going back to comment 4, I would suggest:

  from any script:
    browser.extension.globalData;   // ready only property 
    browser.extension.onGlobalDataAvailable.addListener(...); // called any time data is changed in background

  from background page:
    browser.extension.setGlobalData(value); // maybe key/value pair instead
I apologize if I'm missing something obvious (it's busy on my end lately), but my concern with these APIs is that I'm not 100% sure how frames' content scripts will be able to determine which part of an addon-specific memory store is "intended for them". Will each content script now know its related tabId and frameId, or be able to query for that info synchronously? Or will it perhaps be possible to specify that a given part of the content-store is specific to a particular window, tab, or frame, and the browser will free it when those ids are no longer used?
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> Another thought is to create browser.storage.memory.  storage already has
> onchanged.  memory would be a key/value store.  already has a well defined
> api, but maybe could be modified to have more synchronous reads.

While I was thinking on an initial approach to propose, I was thinking about extend the storage API for it
but then I chose to not use the storage namespace for the following reason:

- all the storage APIs in the storage namespace provide the same API and none of them has any synchronous API methods (and for very good reasons)

- I would prefer to not provide this synchronous API as an additional storage API available to every one of the other extension pages, but only something restricted to the content scripts

also, the following are other details of the proposed approach that I'm thinking of: 

- I would like to limit this API for storing only very small amounts of data

- I would prefer to not provide any real time update mechanisms on it (it would can receive the data asynchronously there is no need to use this API, you can use the storage API or the messaming APIs already provided in every extension page or content scripts)

> 
> But, if you want to go down the other road...
> 
> From the top level documentation on browser.runtime and browser.extension,
> it seems like browser.extension is the more appropriate place for the api:
> 
> runtime:
> 
> This module provides information about your add-on and the environment it's
> running in.
> 
> vs. extension:
> 
> Utilities related to your add-on. Get URLs to resources packages with your
> add-on, get the Window object for your add-on's pages, ** get the values for
> various settings **.

I agree that the 'runtime' namespace wasn't a great pick, in the meantime 
I moved everything related to it in its own 'contentScriptOptions' namespace.
 
> I'm also not clear on why we would limit access to the config to content
> scripts.  I can see potential use for this in other areas.  

The main reason is that I would like to prevent this API from becoming one additional storage available to any
arbitrary extension page, especially given that is expose a synchronous API and the data is stored in memory.

I don't think that most of the other extension pages need this additional synchronous
in-memory storage API, they can use the messaging API or the other storages available,
or in the worst case they can already access the other views using `extension.getViews` 
(https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extension/getViews) and retrieve
whatever they want from the other extension pages' globals.

> Going back to comment 4, I would suggest:
> 
>   from any script:
>     browser.extension.globalData;   // ready only property 
>     browser.extension.onGlobalDataAvailable.addListener(...); // called any
> time data is changed in background
> 
>   from background page:
>     browser.extension.setGlobalData(value); // maybe key/value pair instead

In my opinion this API doesn't need events:
if an extension can achieve what it needs from an event listener, you don't need this synchronous in-memory
storage, you can use the existent storage API or the messaging APIs.

Does the above reasons sound reasonable to you?
(In reply to Thomas Wisniewski from comment #17)
> I apologize if I'm missing something obvious (it's busy on my end lately),
> but my concern with these APIs is that I'm not 100% sure how frames' content
> scripts will be able to determine which part of an addon-specific memory
> store is "intended for them". Will each content script now know its related
> tabId and frameId, or be able to query for that info synchronously? Or will
> it perhaps be possible to specify that a given part of the content-store is
> specific to a particular window, tab, or frame, and the browser will free it
> when those ids are no longer used?

This issue is currently focused on exploring how to provide the contentScriptOptions as a small amount of data synchronously available to the content scripts, mostly to "seed" their initial behavior when the document is starting to load (and then try to defer the loading on bigger amounts of data through the usage of the asynchronous data storage and messaging APIs).

Make the API able to set a property on a single tab is technically possible, I would not exclude it completely.

For an API to let the content scripts able to detect their own tabId and frameId, I think that it would be better to file a separate issue.

About the size limitations of this storage, I agree with :zombie that the size of this storage really needs to be limited, even more limited of the storage APIs.
As a API able to move bigger amounts of data between the two processes, e.g. images, bigger text files content (e.g. user scripts) etc. (basically anything like a Blob), I agree with :robwu that a nicer solution could be to provide an API to programmatically make some of the Blob URLs generated from an extension as "web accessible" (currently a blob URL generated from the extension pages will fail to load in a content webpage with a "Source URI is not allowed in this document" security error).

I would like to know more about your use case (e.g. to check if the "web accessible" blob URLs solution described above could fit your use case as well).
As anticipated in Comment 18, the updated patch contains a new proposal, 
the most important changes are the following:

- all the API is contained in its own namespace (and a permission, at least currently): browser.contentScriptOptions

- the API limits the maximum number of properties that a single extension can set and the maximum length of the string values (which are currently const defined in the API module, but we can also turn them into values read from the about:config preferences if we think that this could be a reasonable approach)

- the API allows the extension pages (e.g. a background page) to set single named properties, without send the entiry configuration object at every change

- the type of the property values can only be number, boolean or string
Thanks, :rpl. What I need is to essentially be able to pass data to a frame's content script from the background script, so that the content script can use it to seed the frame's state BEFORE other scripts in the frame have a chance to run.

As such a blob-based solution *could* work, if the content script can read the data out of it synchronously (using an XMLHttpRequest I suppose). If it has to use an async FileReader, or some other callback that might only trigger after the frame's scripts begin running, then it's a no-go.

Three solutions that I can see working for such a use case are:

1. Allowing to executeScript(document_start, frameId), in an early listener like webNavigation.onCommitted, and have that run as part of the content script for the frame, but before the ones listed in manifest.js for document_start (which should also ensure that it runs before the frame's other scripts). (This was basically what I requested in bug 1352917, which was duped to this bug).

2. Allowing to setFrameSpecificData(frameId) in the background script, and then readFrameSpecificData() from the content script, where the read is synchronous and guaranteed to call back before the other scripts on the page run. Whether that uses blob URLs or some other mechanism is just an implementation detail in that case.

3. Allowing to runtime.sendMessage() from the content script, with a guarantee that the synchronous callback will run before the rest of the frame's scripts (which doesn't happen right now).

Bear in mind that I don't mind helping to implement something for this use-case, if that helps.
Comment on attachment 8854479 [details]
Bug 1332273 - Implement browser.contentScriptOptions API.

https://reviewboard.mozilla.org/r/126418/#review130366

I wonder whether the limits are useful. To work around the limits, I can imagine an add-on dev splitting their big value over multiple keys and use getPropertyNames / getProperty to retrieve all stored values. This results in extra work for the dev and a degraded performance for add-on users.
How about choosing a generous yet sane limit and enforce it for all data combined?

::: toolkit/components/extensions/ext-c-contentScriptOptions.js:12
(Diff revision 2)
> +    return {
> +      contentScriptOptions: {
> +        getProperty(propertyName) {
> +          if (context.envType !== "content_child") {
> +            throw new Error('This method is only supported in content scripts');
> +          }

Use the "allowedContexts" key in the schema for access control.

Currently there is no key to restrict APIs to content scripts only, so you can add a new one, e.g. by putting the following at the end of [shouldInject][shouldInject] in ExtensionChild.jsm

```
if (this.context.envType !== "content_child" &&
    allowedContexts.includes("content_only")) {
  return false;
}
```

And then add `"allowedContexts": ["content", "content_only"]` to the specific methods  in content_script_options.json. That will make sure that the methods only show up in contexts where these APIs are supported.

The documentation of `shouldInject` is still accurate: https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process#shouldInject

[shouldInject]: http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/toolkit/components/extensions/ExtensionChild.jsm#747-770

::: toolkit/components/extensions/ext-c-contentScriptOptions.js:30
(Diff revision 2)
> +          if (context.envType !== "addon_child") {
> +            throw new Error('This method is not support in content scripts and devtools pages');
> +          }
> +          return context.childManager
> +                        .callParentAsyncFunction("contentScriptOptions.getUsageInfo", []);
> +        }

If `allowedContexts` and `defaultContexts` do not contain "content" in the schema JSON, then they are not enabled in content scripts by default.

If you follow my above suggestion of enforcing access control through the schema, then you can remove getUsageInfo here because the default implementation in the child is to delegate the call to the parent.

::: toolkit/components/extensions/ext-contentScriptOptions.js:26
(Diff revision 2)
> +      usageInfo = contentScriptOptionsUsage.get(extension);
> +    }
> +
> +    return {
> +      contentScriptOptions: {
> +        setProperty(propertyName, propertyValue) {

You should also enforce a maximum length on the key name. Otherwise one can encode a large chunk of data in the key and use getPropertyNames to obtain the values.

If the maximum length is static, you can use the maxLength key in the JSON schema, otherwise (e.g. if you want to allow it to be configured through preferences later), put the check here.
Thanks Rob for the new round of feedback, I agree that the limits to apply and how it would be better to enforce them is one of the aspects that need more evaluations, besides that I'm still not sure that I like the general approach that I'm proposing
(in particular I do not like that it basically provides one more storage API, I would  prefer to just provide something stored in one of the existent storage APIs).

An alternative approach that I'm thinking of is the following:

- provide an API method that allow a privileged extension page (e.g. the background page) to configure which browser.storage.local/browser.storage.sync keys that are going to be preloaded when the content scripts declared in the manifest are injected

- an optional match parameter can restrict when the preloading should happen (e.g. restrict it to a particular content script / url pattern / tabId / topframe / subframe etc.)

- an additional parameter of tabs.executeScript can configure the keys to preload for dinamically injected content scripts

- pause the document loading while we are preloading the configured keys from the extension storages, using the same approach that we are already using here: http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/toolkit/components/extensions/ExtensionContent.jsm#405
Today I spent some additional time on this issue and I'm really not convinced that a synchronous storage is a good solution for the described needs and, especially based on Comment 22, this contentScriptOptions seems a lot more a workaround for another missing feature ("not being currently able to dinamically register content scripts"), than a feature on its own.

What I'm wondering is:

If, on the contrary, we make it possible to programmatically register new content scripts from the background page, and allowing the definition on this new content scripts also from extension Blob URLs (which doesn't need to be "blessed" because they are going only be loaded as extension content scripts instead of being injected into the page), this content scripts will be able to run at document_start (as a content script registered from the manifest) and load everything else asynchronously (e.g. there is probably no really need to load other kind of assets, like images, to inject them at document_start, they can be loaded asynchronously by the registered content script).

I also see that "being able to programmatically register new content script" is a feature that has been already requested more than once (e.g. Bug 1305856 and Bug 1323433) and Kris has shown to be in favor of allowing content script to be registered programmatically (https://bugzilla.mozilla.org/show_bug.cgi?id=1305856#c4).

Given that, as I mentioned in my previous comment, I really don't like the approach that I proposed in the attached patch and that any synchronous storage API would have mostly the same issue (e.g. which is the reasonable amount of data that we can preload? what happens when an extension tries to preload too much data? what we should do in case of "memory pressure"? etc.), I'm going to close the current mozreview request and open a new one with the new proposal:

a prototype of extending the tabs API to support "programmatically registered content scripts" and a related small test case (an xpcshell test case) for an initial feedback.
Attachment #8854479 - Attachment is obsolete: true
(In reply to Luca Greco [:rpl] from comment #25)
> Today I spent some additional time on this issue and I'm really not
> convinced that a synchronous storage is a good solution for the described
> needs and, especially based on Comment 22, this contentScriptOptions seems a
> lot more a workaround for another missing feature ("not being currently able
> to dinamically register content scripts"), than a feature on its own.

Mostly. I stated as much in comment 0. But on the other hand if dynamically loading content scripts is the *only* feature available then loading large hunks of data that they want to use synchronously as jsonp might also be inefficient. E.g. an adblocker that wants to load a selector-based ruleset or a localstorage shim for sites that break when cookies are blocked.

Ideally, to keep memory footprint and script execution time down, such things should exist in as immutable data in a shared compartment that can be read by multiple content script.

But that's mostly a performance optimization. If I had to choose then the ability to register content scripts at runtime would be my preferred option.

> also from extension Blob URLs

This on the other hand would be somewhat limiting. Filesystem URLs are preferred since it allows the user to develop a script, e.g. via an IDE and have the userscript pick up  the current copy whenever it is changed. But I guess filesystem access is orthogonal to the ability to register scripts.
(In reply to The 8472 from comment #26)
> Mostly. I stated as much in comment 0. But on the other hand if dynamically
> loading content scripts is the *only* feature available then loading large
> hunks of data that they want to use synchronously as jsonp might also be
> inefficient. E.g. an adblocker that wants to load a selector-based ruleset
> or a localstorage shim for sites that break when cookies are blocked.

Yeah, I agree that it would not cover every scenario, what I hope is that "being able to create content scripts programmatically" will help to greatly reduce both the scenarios that needs such preload and also the amount of the data and then, if such API is still needed, we can more likely be able to set reasonably low storage limits to it.

Besides that, the adblocker scenario described above should be possible with the proposed patch using something like the following snippet:

     async function updateAdBlockingContentScript(oldContentScript) {
       // Remove the previous content script version
       await browser.tabs.unregisterContentScript(oldContentScript.scriptId);
       URL.revokeObjectURL(oldContentScript.ruleSetURL);
       
       // Load the updated rule set
       const ruleSet = await browser.storage.local.get("adblock_ruleset");

       // 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",
       });

       return {ruleSetBlob, ruleSetURL, contentScriptId};
     }

 
> This on the other hand would be somewhat limiting. Filesystem URLs are
> preferred since it allows the user to develop a script, e.g. via an IDE and
> have the userscript pick up  the current copy whenever it is changed. But I
> guess filesystem access is orthogonal to the ability to register scripts.

Being able to register a content script using a blob URL should make this possible:

- user scripts edited in an external editor can be provided to the extension using
  an input element or drag and drop
- editing user scripts in an editor embedded in the extension itself can be provided
  by storing the blobs into indexedDB (by readind the Blob content and saving new Blobs, or by using a IDBMutableFile)
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

Hi Rob, Hi Shane,
like described in Comment 25 this patch do not provide a contentScripOptions API but an API to register and unregister content scripts programmatically, the javascript and css assets associated to the dinamically registered content scripts can be a relative extension URL (like the content scripts defined in the manifest) or a blob URL created by the same extension.

The two url types can also be mixed in the same registered content script, which should also allow, as described in Comment 28, a way to preload some data into a content script.

I'd really like to get your initial feedback on the proposed approach (and its rationale).

I would be still open to allow some form of data preloading to it (e.g. by specify an additional property to the browser.tabs.registerContentScript and the manifest content_script options, where an extension can specify a set of browser.storage.local keys to preload before the script is injected)
Attachment #8862577 - Flags: feedback?(rob)
Attachment #8862577 - Flags: feedback?(mixedpuppy)
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review137634

This approach looks great! It is much closer to the actual use case than the previous solution, and it can also be used to implement sync storage if an add-on author really wants to. Furthermore, it has more potential for being efficient in Firefox's resource usage.

`tabs` is an odd namespace for this. How about a new namespace, `declarativeContentScript`?

::: browser/components/extensions/ext-tabs.js:515
(Diff revision 1)
>            let tab = await promiseTabWhenReady(tabId);
>  
>            return tab.sendMessage(context, "Extension:DetectLanguage");
>          },
>  
> +        async registerContentScript(options) {

You should also check whether the add-on has the permission to register a script for the given URLs.

::: toolkit/components/extensions/Schemas.jsm:846
(Diff revision 1)
> +  strictExtensionBlobUrl(string, context) {
> +    // Do not accept a string which is not a blob URL with the origin of the current extension.
> +    let blobURL = new URL(string);
> +
> +    if (blobURL.protocol == "blob:") {
> +      if (!context.checkLoadURL(blobURL.origin)) {

I think that this check is too permissive, since it allows remote URLs to declaratively be loaded.

Based on
http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/toolkit/components/extensions/Schemas.jsm#303

I ran the following snippet in the global JS console

```
Services.scriptSecurityManager.checkLoadURIStrWithPrincipal(Services.scriptSecurityManager.createCodebasePrincipal(NetUtil.newURI('moz-extension://aaa/'),{}), 'http://example.com/', Services.scriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL)
```

This snippet did not throw, so that suggests that `checkLoadURL` would return true and allow remote URLs.

Try checking whether the inner URL of the blob URL matches the extension's origin.

::: toolkit/components/extensions/extension-process-script.js:551
(Diff revision 1)
>      this.instanceId = data.instanceId;
>      this.manifest = data.manifest;
>  
>      this.scripts = data.content_scripts.map(scriptData => new ScriptMatcher(this, scriptData));
>  
> +    // A map of dinamically registered content scripts

s/dinamically/dynamically/

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registering.js:59
(Diff revision 1)
> +
> +    let blobScript = new Blob([`(${userScript})()`], {type: "text/javascript"});
> +    let blobScriptURL = URL.createObjectURL(blobScript);
> +
> +    const blobScriptId = await browser.tabs.registerContentScript({
> +      js: [blobScriptURL],

Please add a test for "css" too.
Attachment #8862577 - Flags: feedback?(rob) → feedback+
One thing more: For user script managers, the add-on should be allowed the possibility of running the script in separate sandboxes (or even directly in the page, a la @grant none in Greasemonkey).
Is this possible or considered out of scope?

See also bug 1353468, where I am requesting such a feature.
Flags: needinfo?(lgreco)
I'm surprised this works. Last time I tried to implement a similar solution in greasemonkey the subscript loader didn't accept a blob URI. There even is bug 1221148 for this. Has something changed?

(In reply to Rob Wu [:robwu] from comment #31)
> One thing more: For user script managers, the add-on should be allowed the
> possibility of running the script in separate sandboxes (or even directly in
> the page, a la @grant none in Greasemonkey).
> Is this possible or considered out of scope?

|@grant none| in GM does not actually execute things in the page context, it still uses a sandbox, but with xrays turned off. That has the effect that the user script still gets a set of pristine primordial objects that haven't their prototypes manipuldated by the page content and it also does not pollute the page global.
Re comment #32

And keeps scripts isolated from each other.  They each get a `const GM_info = {...};` with different contents.  Today if I `tabs.executeScript()` that, only the first works and the second breaks (redeclared constant).  Distinct configurable (possibly xray-off) per-content-script sandboxes are desirable.
Flags: needinfo?(tomica)
I've just updated the attached patch with a new version, the main changes applied to it are the following:

- The proposal has been adapted to the changes around the WebExtensions ContentScripts recently landed on m-c
  (mainly Bug 1322235 and Bug 1368102, which means that part of the patch now is applied to the new WebIDL and the related C++ components related to the WebExtensions ContentScripts)
 
- The registerContentScript API method now accepts Blob objects instead of blob urls, by creating and keeping track of the registered Blobs and their blob urls in the API implementation, we can also ensure that the Blob and its blob url are still valid while the registered content script that contains it still registered.

Another reason to prefer Blob instances as js or css resources in the registerContentScript API method, instead of blob urls, is that we can more easily (and efficiently) check that the blob has the expected/supported content type (I didn't added this check yet, but it will be easy to add now that the API method receives the Blob instance) and/or the size of the registered Blob is not higher than a defined value (e.g. a maximum allowed size coming from an "about:config" preference).

I'm going to add more tests in the next update on this patch, in the meantime I'm going to ask to Kris to take an initial look at the proposal as soon as he can.
Flags: needinfo?(lgreco)
I'm not sure if this is particularly useful to note (to head off future interop issue perhaps?), but the Blink webview has addContentScripts/removeContentScripts APIs: https://bugs.chromium.org/p/chromium/issues/detail?id=461052
There's also a request to get those added to extensions as well: a https://bugs.chromium.org/p/chromium/issues/detail?id=471801

Those APIs don't seem to be useful for this case, as they do not take blobs, but I figured it was worth mentioning.

Safari extensions also have APIs named this way, but are similarly useless for this bug's intended use-case: https://developer.apple.com/documentation/safariextensions/safariextension/1635517-addcontentscript
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review138704

::: browser/components/extensions/ext-tabs.js:655
(Diff revision 3)
> +          const contentScript = new RegisteredContentScript({context, options});
> +          const {scriptId} = contentScript;
> +
> +          registeredContentScriptsMap.set(scriptId, contentScript);
> +
> +          context.callOnClose({

This looks like it will call for each registered script.  Could we have one call that removes them all?

::: browser/components/extensions/schemas/tabs.json:96
(Diff revision 3)
> +            "isInstanceOf": "Blob"
> +          }
> +        ]
> +      },
> +      {
> +        "id": "RegisteredContentScript",

I wonder if we should split css and js entirely, have a contentScript and contentCSS type.

::: browser/components/extensions/schemas/tabs.json:132
(Diff revision 3)
> +            "items": { "$ref": "ExtensionFileOrBlob" }
> +          },
> +          "js": {
> +            "type": "array",
> +            "optional": true,
> +            "description": "The list of CSS files to inject",

description wrong

::: toolkit/components/extensions/Extension.jsm:902
(Diff revision 3)
>  
> -  receiveMessage({name, data}) {
> +  receiveMessage({name, data, target}) {
>      if (name === this.MESSAGE_EMIT_EVENT) {
>        this.emitter.emit(data.event, ...data.args);
> +    } else if (name === this.MESSAGE_GET_REGISTERED_CONTENT_SCRIPTS) {
> +      this.emitter.emit("send-registered-content-scripts", {target});

what if target is undefined?  Also seems data is unused here, can data be target?

::: toolkit/components/extensions/ExtensionPolicyService.cpp:316
(Diff revision 3)
> +          ProcessScript().PreloadContentScript(script);
> +        } else {
> +          ProcessScript().LoadContentScript(script, aDocInfo.GetWindow());
> +        }
> +      }
> +    }

seems straight forward but I'd like kmag to look.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:275
(Diff revision 3)
> +  if (aRv.Failed()) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return;
> +  }
> +
> +  mRegisteredContentScripts.AppendElement(Move(contentScript));

Are we keeping two maps (here and in ext-tabs)?  Can we reduce that to one?
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review138704

> I wonder if we should split css and js entirely, have a contentScript and contentCSS type.

Currently I preferred to keep the registered content script consistent with the content script registered using the manifest.json, mostly because I think that it would help the addon developers, and so css and js are handled in the same way here.

I'm not sure if splitting them would be helpful, but I do think that keeping them consistent with the "content scripts statically registered from the manifest.json" can also help to make it easier to migrate a content script which used to be defined and registered statically into the dinamically registered version.

Nevertheless, from an implementation point of view, splitting them should be pretty simple, we can keep most of the implementation as it is and just split the API method exposed to the extension.

> what if target is undefined?  Also seems data is unused here, can data be target?

`data` is only expected and used by the other message manager event and it is optional (the particular event have to explicitly send the data as part of the message manager event to be sure that `data` is there on the receiver side), while `target` represents the receiver of the message manager event, basically the `browser` XUL element, and so it should never be `undefined`.

> Are we keeping two maps (here and in ext-tabs)?  Can we reduce that to one?

These two maps are conceptually different and they have different roles (and they contain different kind of objects):

- the Map in ext-tabs keeps track of the "options" related to the registered content script and convert the Blob objects into blob urls, this map only exists in the main process and no WebExtensionContentScript instance is created in this process, we keep track of this data in the main process, so that we can revoke the blob urls when a content script has been unregistered and to restore the registered content scripts if a tab process crash and a new content process is created to replace it
  
- WebExtensionPolicy also has its own map (well, it is an array actually), but these are WebExtensionContentScript instances, which should only exist in the content processes (actually I'm going to add a check to ensure that the "Extension:RegisterContentScript" message is only processes in a content process, because we are going to broadcast it to every existent process when it is registered, which includes the main process and the extension process)
Attachment #8862577 - Flags: review?(kmaglione+bmo)
Attachment #8904292 - Flags: review?(mixedpuppy)
Comment on attachment 8904292 [details]
Bug 1332273 - Lock registered content scripts API method behind a preference.

https://reviewboard.mozilla.org/r/176066/#review181080

Lets not do this.  The other patch is standalone functionality we need, no reason to pref off.
Attachment #8904292 - Flags: review?(mixedpuppy) → review-
Attachment #8904292 - Attachment is obsolete: true
(In reply to Shane Caraveo (:mixedpuppy) from comment #50)
> Comment on attachment 8904292 [details]
> Bug 1332273 - Lock registered content scripts API method behind a preference.
> 
> https://reviewboard.mozilla.org/r/176066/#review181080
> 
> Lets not do this.  The other patch is standalone functionality we need, no
> reason to pref off.

Sounds good to me, patch dropped from the attached mozreview request.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181082

::: browser/components/extensions/ext-tabs.js:7
(Diff revision 11)
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
>  // The ext-* files are imported into the same scopes.
>  /* import-globals-from ext-utils.js */
> +/* import-globals-from ../../../toolkit/components/extensions/ext-registerContentScript.js */

Why a full path here?

::: browser/components/extensions/ext-tabs.js:512
(Diff revision 11)
>  
>            return tab.sendMessage(context, "Extension:DetectLanguage");
>          },
>  
> +        registerContentScript(options) {
> +          return registerContentScript(context, options);

this is unfortunate for readabilty of the code.  I have to keep a mental picture of which "registerContentScript" is being called.

::: browser/components/extensions/schemas/tabs.json:24
(Diff revision 11)
> +      {
> +        "$extend": "Permission",
> +        "choices": [{
> +          "type": "string",
> +          "enum": [
> +            "tabs.registerContentScript"

Need the permission string in browser.properties?

::: mobile/android/components/extensions/ext-tabs.js:6
(Diff revision 11)
>  /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
> +// The ext-* files are imported into the same scopes.
> +/* import-globals-from ../../../../toolkit/components/extensions/ext-registerContentScript.js */

full path again, not sure why this is necessary.

::: toolkit/components/extensions/ExtensionContent.jsm:350
(Diff revision 11)
>      TelemetryStopwatch.start(CONTENT_SCRIPT_INJECTION_HISTOGRAM, context);
>      try {
>        for (let script of scripts) {
> +        if (!script) {
> +          // Ignore any script that has failed to load (an error has been already logged
> +          // from the this.loadScripts method).

do you mean compileScripts?

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registering.js:30
(Diff revision 11)
> +      browser.test.assertTrue(expectedStates.includes(readyState),
> +                              `readyState "${readyState}" is one of [${expectedStates}]`);
> +      browser.test.sendMessage("script-run-" + expectedStates[0]);
> +    });
> +
> +    const content_scripts = [

This seems to only test files, what about blobs?  Seems you're only testing an invalid blob below.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181106

It's a good start, but needs work.

::: dom/webidl/WebExtensionPolicy.webidl:114
(Diff revision 11)
>    DOMString getURL(optional DOMString path = "");
>  
> +  /**
> +   * Retrieve all the registered content scripts.
> +   */
> +  sequence<WebExtensionContentScript> getRegisteredContentScripts();

This should be an attribute. But I don't see why we should need a separate property for this. These should just be part of the existing `contentScripts` list.

::: dom/webidl/WebExtensionPolicy.webidl:116
(Diff revision 11)
> +  /**
> +   * Clear all the registered content scripts.
> +   */
> +  void clearRegisteredContentScripts();

Unnecessary. Just remove the scripts individually.

::: dom/webidl/WebExtensionPolicy.webidl:121
(Diff revision 11)
> +  /**
> +   * Register a new content script programmatically.
> +   */
> +  [Throws]
> +  void registerContentScript(DOMString scriptId, WebExtensionContentScriptInit scriptInit);
> +
> +  /**
> +   * Unregister a content script previously registered programmatically.
> +   */
> +  [Throws]
> +  void unregisterContentScript(DOMString scriptId);

No string IDs for things that we have real objects for. Just add an `active` attribute to the content script object like we have for `WebExtensionPolicy`.

::: mobile/android/components/extensions/ext-tabs.js:5
(Diff revision 11)
> +// The ext-* files are imported into the same scopes.
> +/* import-globals-from ../../../../toolkit/components/extensions/ext-registerContentScript.js */

Please don't use `import-globals-from` for API scripts. The fact that these comments were added to other scripts is a bug that needs to be fixed.

::: mobile/android/components/extensions/ext-tabs.js:367
(Diff revision 11)
> +        registerContentScript(options) {
> +          return registerContentScript(context, options);
> +        },

These aren't tab-specific. They don't belong in the tabs API.

::: mobile/android/components/extensions/schemas/tabs.json:1043
(Diff revision 11)
> +      {
> +        "name": "registerContentScript",
> +        "permissions": ["tabs.registerContentScript"],
> +        "type": "function",
> +        "description": "Register a content script programmatically",
> +        "async": "callback",

No callback variants for APIs that don't exist in Chrome, please.

::: mobile/android/components/extensions/schemas/tabs.json:1071
(Diff revision 11)
> +        "async": true,
> +        "parameters": [
> +          {
> +            "name": "contentScriptId",
> +            "type": "string",
> +            "optional": true

Why optional?

::: toolkit/components/extensions/Extension.jsm:769
(Diff revision 11)
> +    // Listen for content processes that crashed and they have been restored
> +    // and needs all the programmatically registered content scripts.
> +    this.MESSAGE_GET_REGISTERED_CONTENT_SCRIPTS =
> +      `Extension:GetRegisteredContentScripts:${addonData.id}`;
> +    Services.ppmm.addMessageListener(this.MESSAGE_GET_REGISTERED_CONTENT_SCRIPTS, this);

No special messages for this, please. Let's just handle it the way we handle other content process init data.

::: toolkit/components/extensions/ExtensionContent.jsm:227
(Diff revision 11)
>      this.requiresCleanup = !this.removeCss && (this.css.length > 0 || matcher.cssCode);
>    }
>  
>    compileScripts() {
> -    return this.js.map(url => this.scriptCache.get(url));
> +    return this.js.map(url => this.scriptCache.get(url).catch(err => {
> +      Cu.reportError(new Error(`Failing to load Content Script JS from URL "${url}"`));

Nit: `Failed to load content script at URL: ${url}`

::: toolkit/components/extensions/ExtensionContent.jsm:311
(Diff revision 11)
> +            if (!sheet) {
> +              // Ignore any sheet that has failed to load (an error has been already logged
> +              // from the this.loadCSS method).
> +              continue;

This seems like an unrelated change. Same for the `.catch()` clauses above. Separate patch please.

::: toolkit/components/extensions/ExtensionParent.jsm:440
(Diff revision 11)
>  defineLazyGetter(ProxyContextParent.prototype, "apiObj", function() {
>    return this.apiCan.root;
>  });
>  
>  defineLazyGetter(ProxyContextParent.prototype, "sandbox", function() {
> -  return Cu.Sandbox(this.principal);
> +  return Cu.Sandbox(this.principal, {wantGlobalProperties: ["URL"]});

This seems like an unrelated change.

::: toolkit/components/extensions/ExtensionPolicyService.cpp:304
(Diff revision 11)
> +
> +    nsTArray<RefPtr<WebExtensionContentScript>> registeredContentScripts;
> +    policy->GetRegisteredContentScripts(registeredContentScripts);
> +
> +    for (auto& script : registeredContentScripts) {
> +      if (script->Matches(aDocInfo)) {
> +        if (aIsPreload) {
> +          ProcessScript().PreloadContentScript(script);
> +        } else {
> +          ProcessScript().LoadContentScript(script, aDocInfo.GetWindow());
> +        }
> +      }
> +    }

No separate array for these, please.

::: toolkit/components/extensions/ext-registerContentScript.js:14
(Diff revision 11)
> +XPCOMUtils.defineLazyServiceGetter(this, "uuidGen",
> +                                   "@mozilla.org/uuid-generator;1",
> +                                   "nsIUUIDGenerator");
> +
> +function generateRegisteredScriptId() {
> +  let uuid = uuidGen.generateUUID().number;
> +  uuid = uuid.slice(1, -1); // Strip { and } off the UUID.
> +
> +  return uuid;
> +}

Please just use `ExtensionUtils.getUniqueId`, or if we need this on the native side, generate it on the native side when we create the binding object.

::: toolkit/components/extensions/extensions-toolkit.manifest:6
(Diff revision 11)
>  # scripts
>  category webextension-scripts toolkit chrome://extensions/content/ext-toolkit.js
>  category webextension-scripts-content toolkit chrome://extensions/content/ext-c-toolkit.js
>  category webextension-scripts-devtools toolkit chrome://extensions/content/ext-c-toolkit.js
>  category webextension-scripts-addon toolkit chrome://extensions/content/ext-c-toolkit.js
> +category webextension-scripts registerContentScript chrome://extensions/content/ext-registerContentScript.js

No extra files loaded by default at startup, please.

::: toolkit/components/extensions/schemas/types.json:11
(Diff revision 11)
> +        "id": "ExtensionFileOrBlob",
> +        "choices": [
> +          {
> +            "$ref": "manifest.ExtensionURL"
> +          },
> +          {
> +            "type": "object",
> +            "additionalProperties": { "type": "any" },
> +            "isInstanceOf": "Blob"
> +          }
> +        ]

Please file a separate issue for Blob support and I'll consider it, but it seems quite separate from this issue.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registering.js:62
(Diff revision 11)
> +    // Raise an exception when the content script cannot be registered
> +    // because the extension has no permission to access the specified origin.
> +
> +    let exception;
> +    try {
> +      await browser.tabs.registerContentScript({

`await browser.test.assertRejects(...)`

::: toolkit/components/extensions/test/xpcshell/xpcshell-content.ini:5
(Diff revision 11)
>  [test_ext_i18n.js]
>  skip-if = os == "android"
>  [test_ext_i18n_css.js]
>  [test_ext_contentscript.js]
> +[test_ext_contentscript_registering.js]

Nit: Registration
Attachment #8862577 - Flags: review?(kmaglione+bmo) → review-
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181106

> This should be an attribute. But I don't see why we should need a separate property for this. These should just be part of the existing `contentScripts` list.

My main reason for keeping the two content scripts list separated was that the existent attribute is marked as
"Cached, Constant, Frozen":

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/WebExtensionPolicy.webidl#70-71

My guess is that this is one of the optimization applied to the WE content scripts during their refactoring into C++ components, and it makes completely sense for the content scripts defined in the manifest, because they never change over the lifetime of the extension.

On the contrary the registered content scripts are going to be added and removed at runtime, but I didn't want to remove the optimizations applied for the manifest content scripts (which are currently the most common ones)
just to make space for the dinamically registered content scripts (which are probably going to be used by a smaller number of extensions, e.g. because by using this feature they will not work on the other browsers that do not provide this custom API).

> Unnecessary. Just remove the scripts individually.

Sounds good.

> No string IDs for things that we have real objects for. Just add an `active` attribute to the content script object like we have for `WebExtensionPolicy`.

The registered content scripts are likely to change over the life of the extension (I mean registered and unregistered), shouldn't we remove them from the array of the available content scripts when the extension remove it?

Also:
The unregistering of a dinamically registered content script comes from the main process, and so the string id is what we sent to the content processes to know which is the content script to remove, and so we will need another method to retrieve a content script by scriptId, then marking it as "not active" (and in my opinion we should still remove it from the array of the existent content scripts).

> Please don't use `import-globals-from` for API scripts. The fact that these comments were added to other scripts is a bug that needs to be fixed.

Sounds good, I will move this API into its own namespace (based on the other review comments), and this import-globals-from is not going to be needed anymore.

> These aren't tab-specific. They don't belong in the tabs API.

Sounds good (I think that it is tab-related as much as tabs.executeScript, but it is not a big deal I'm ok into moving it into its own namespace)

> No callback variants for APIs that don't exist in Chrome, please.

Sounds good.

> Why optional?

Initially my idea was to allow to unregister all the scriptIds when unregisterContentScript is called without a parameter, but I will change it and make it mandatory.

> No special messages for this, please. Let's just handle it the way we handle other content process init data.

That was exactly my initial idea but, once I looked a bit deeper into it, the idea of changing the `Services.ppmm.initialProcessData["Extension:Extensions"]` everytime an extension dynamically register/unregister wasn't thrilling me because I was a bit concerned that it wasn't the best way to deal with it.

How about using a different property on the Services.ppmm.initialProcessData to keep track of this dinamically registered content scripts without keep filtering and re-serializing the "Extension:Extensions" property, how that sounds to you?

> This seems like an unrelated change. Same for the `.catch()` clauses above. Separate patch please.

Mostly related to the Blob, which is going to be moved in a follow up, I'm going to move it into the same follow up issue related to support Blob.

> This seems like an unrelated change.

Related to the convertion of the Blobs into blob urls (mostly to avoid to send the Blob data around until is actually needed, when the registered content script actually match a page).

I'm going to move it into the "Blob support" follow up.

> Please just use `ExtensionUtils.getUniqueId`, or if we need this on the native side, generate it on the native side when we create the binding object.

Sounds good.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181106

> That was exactly my initial idea but, once I looked a bit deeper into it, the idea of changing the `Services.ppmm.initialProcessData["Extension:Extensions"]` everytime an extension dynamically register/unregister wasn't thrilling me because I was a bit concerned that it wasn't the best way to deal with it.
> 
> How about using a different property on the Services.ppmm.initialProcessData to keep track of this dinamically registered content scripts without keep filtering and re-serializing the "Extension:Extensions" property, how that sounds to you?

I just tried the approach described in my previous comment (using another property on the Services.ppmm.initialProcessData, e.g. Services.ppmm.initialProcessData["Extension:RegisteredContentScripts"]) 
and it worked great and it also looks much much better than using the special messages \o/

I'm going to include these changes in the next update on this patch.

Thanks a lot for pushing me to reconsider this approach one more time.
See Also: → 1353468
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181082

> Need the permission string in browser.properties?

I'm not totally sure about this, I think that we probably don't need a permission string for it:

the extension still have to request the "host permissions" ("<all_urls>" or a more specific matcher), and iirc we don't have a permission string for the "content_scripts" manifest.json property, but we use the "host permissions" to make the user aware of which part of the visited urls the extension can have access too.

I'm going to double-check with aswan that I'm not wrong about it.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181106

> My main reason for keeping the two content scripts list separated was that the existent attribute is marked as
> "Cached, Constant, Frozen":
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/WebExtensionPolicy.webidl#70-71
> 
> My guess is that this is one of the optimization applied to the WE content scripts during their refactoring into C++ components, and it makes completely sense for the content scripts defined in the manifest, because they never change over the lifetime of the extension.
> 
> On the contrary the registered content scripts are going to be added and removed at runtime, but I didn't want to remove the optimizations applied for the manifest content scripts (which are currently the most common ones)
> just to make space for the dinamically registered content scripts (which are probably going to be used by a smaller number of extensions, e.g. because by using this feature they will not work on the other browsers that do not provide this custom API).

Actually, I've been still thinking about it (sorry, I wrote most of this patch months ago) and now I recall that the actual reason was that I wanted to keep the patch simpler until we reached the point that we all agree that the proposed API is ok. 

I'm going to take another look because I'm pretty sure that there isn't any real issue to prevent us to use the existent contentScripts attribute to retrieve both the statically registered and the dinamically registered content scripts like you suggested.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181440

Some drive-by feedback.

::: toolkit/components/extensions/ext-registerContentScript.js:68
(Diff revision 11)
> +
> +  _convertBlobURLs() {
> +    const {context, options, overriddenOptions} = this;
> +
> +    // TODO(rpl): we could also reject for:
> +    // - Blobs without the expected content type

This can easily be checked, but adding it later would risk breaking backwards-compatibility.

If you think that the content type must match, add the check now. The list of JS MIME types is at https://html.spec.whatwg.org/multipage/scripting.html#javascript-mime-type

::: toolkit/components/extensions/ext-registerContentScript.js:109
(Diff revision 11)
> +const registeredContentScripts = new DefaultWeakMap(() => new Map());
> +
> +// A map of the content script ids registered by a context.
> +//
> +// WeakMap[context -> Set[string]
> +const contextRegisteredScriptIds = new DefaultWeakMap(() => new Set());

After reviewing the code below, I've noticed that this "context" is not some arbitrary context, but supposedly/always an extension context, uniquely owned by one extension (since the `registerContentScript` method can only be used by an extension page, which belongs to exactly one extension.

For an easier understanding of the logic, what about a nested DefaultWeakMap, like this:

```
const registeredContentScripts = new DefaultWeak(extension => {
  // code here that should run only once per extension.
  extension.once("shutdown", () => { /* cleanup here */ });
  return new DefaultWeakMap(context => {
    // code here that should run only once per extension context.
    context.callOnClose({ close() { /* cleanup here*/ } });
  });
});
```

The above would make the `initRegisteredContentScript` function unnecessary, and also the implementation of (un)registerContentScript easier to follow.

::: toolkit/components/extensions/ext-registerContentScript.js:123
(Diff revision 11)
> +      target.sendAsyncMessage("Extension:RegisterContentScript",
> +                              contentScript.serialize());
> +    }
> +  }
> +
> +  if (extension.hasPermission("tabs.registerContentScript")) {

Why is this check here?
The schema validator should have ensured that registerContentScript cannot be called if the extension does not have the permission.

And if the permission is somehow necessary, then the hasPermission call belongs below, at the top of `global.registerContentScript`.

::: toolkit/components/extensions/ext-registerContentScript.js:130
(Diff revision 11)
> +    if (!registeredContentScripts.has(extension)) {
> +      extensionRegisteredScripts = registeredContentScripts.get(extension);
> +
> +      // Send the registered content scripts to any content process that has been
> +      // started after the scripts have been registered (e.g. any restored crashed tabs)
> +      extension.on("send-registered-content-scripts", onSendRegisteredContentScripts);

s/send/request/ ?

::: toolkit/components/extensions/ext-registerContentScript.js:136
(Diff revision 11)
> +      extension.once("shutdown", () => {
> +        extension.off("send-registered-content-scripts", onSendRegisteredContentScripts);
> +      });
> +    }
> +
> +    contextScriptIds = contextRegisteredScriptIds.get(context);

The logic below should run only once per (extension, context) tuple.

So you should first check if `contextRegisteredScriptIds.has(context)`, and if so, return.

::: toolkit/components/extensions/ext-registerContentScript.js:143
(Diff revision 11)
> +    // Unregister all the scriptId related to a context when it is closed.
> +    context.callOnClose({
> +      close() {
> +        for (let scriptId of contextScriptIds) {
> +          extensionRegisteredScripts.delete(scriptId);
> +          extension.broadcast("Extension:UnregisterContentScript", {

Would it make sense to optimize this by broadcasting only one message with a list of scriptIds?

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registering.js:120
(Diff revision 11)
> +    browser.runtime.sendMessage(["chrome-namespace-ok"]);
> +  }
> +
> +  let extensionData = {
> +    manifest: {
> +      applications: {gecko: {id: "contentscript@tests.mozilla.org"}},

Is this line really necessary? I thought that the ID would be auto-generated.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registering.js:298
(Diff revision 11)
> +  equal(unregisteredBlobStylesResults.registeredExtensionBlobStyleBG, "rgba(0, 0, 0, 0)",
> +        "The expected style has been applied once extension blob style has been unregistered");
> +
> +  await contentPage.close();
> +  await extension.unload();
> +});

The implementation unregisters the scripts when the context is closed. Unless I overlooked, I didn't find any test coverage for that behavior.
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review181470

Is unregistering the content scripts upon unload a design decision or had it just been easier to implement?
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review182364

::: toolkit/components/extensions/ext-registerContentScript.js:102
(Diff revisions 11 - 12)
>  const registeredContentScripts = new DefaultWeakMap(() => new Map());
>  
> -// A map of the content script ids registered by a context.
> +// A set of the scriptsId related to a context.
>  //
>  // WeakMap[context -> Set[string]
>  const contextRegisteredScriptIds = new DefaultWeakMap(() => new Set());

Can this global variable be a local variable in the scope of the `getAPI` function?

::: toolkit/components/extensions/ext-registerContentScript.js:158
(Diff revisions 11 - 12)
> -            scriptId,
> -          });
>          }
> +        contextRegisteredScriptIds.delete(context);
>  
> -        contextScriptIds.clear();
> +        extension.broadcast("Extension:UnregisterContentScripts", {

Only broadcast if `scriptIds` is non-empty.

::: toolkit/components/extensions/ext-c-registerContentScript.js:28
(Diff revision 12)
> +
> +  api() {
> +    return {
> +      // TODO(rpl): allow to read the options related to the registered content script?
> +      unregister: () => {
> +        return this.context.cloneScope.Promise.resolve().then(async () => {

Is this really needed? I think that you can just use `async unregister`.
https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/toolkit/components/extensions/ExtensionCommon.jsm#667-681

In other code where you applied the same logic before (https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/browser/components/extensions/ext-c-devtools-panels.js#243), there you had to return a custom promise because otherwise `wrapPromise` [1] would use `applySafe`, which uses `cloneInto` on the promise resolution object (which would break your code in ext-c-devtools-panel.js, because there you already called `cloneInto`, but with `cloneFunctions:true`).

[1] https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/toolkit/components/extensions/ExtensionCommon.jsm#438-442

::: toolkit/components/extensions/ext-c-registerContentScript.js:52
(Diff revision 12)
> +              "contentScripts.register", [options]
> +            );
> +
> +            const registeredScript = new ChildRegisteredContentScript(context, scriptId);
> +
> +            return Cu.cloneInto(registeredScript.api(), context.cloneScope, {cloneFunctions: true});

Does not need to be part of this patch, but would it make sense to add a schema attribute to state that the return value should be cloned with `cloneFunctions:true`? That would make the ext-c code prettier.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_registration.js:333
(Diff revision 12)
> +      },
> +    ];
> +
> +    for (const scriptOptions of content_scripts) {
> +      const script = await browser.contentScripts.register(scriptOptions);
> +      browser.test.assertTrue(script && typeof script == "object" &&

How about using `Object.keys(script)` to ensure that we don't inadvertently expose APIs?
Follows some notes on the updated version of this patch:

I've turned registeredContentScripts into a property, using [Constant] in the webidl prevents the property cached value from being invalidated, so this new property is marked as [Pure] and its cached value is invalidated when the registered scripts have been changed (on the contrary there will be no value in exposing them through the property if it is not updated after the first read).

By keeping them separated we can also include some information about how many of the content scripts are dinamically or statically registered in a telemetry probe.

I haven't removed ClearRegisteredContentScripts because it is currently used to remove every registered content scripts at once when the extension policy is disabled.

The scriptId string is now only used internally (it is never provided or used by the extension code, which will now just call script.unregister() in the script resolved by the register API method)

Blobs are not directly supported anymore (by this patch) as a js or css property items and I've replaced it with support for js and css strings, because this API should be able to be used to inject js/css strings, where tabs.executeScript doesn't provide the needed guarantees (e.g. being able to run on document_start).

Internally the js or css strings are turned into a blob urls, and these urls are the ones that are broadcasted to the child processes (e.g. this way we can cache this content scripts as we do for the content scripts registered in the manifest), and so the "URL" and "Blob" globals are imported into the Proxy contexts sandboxes.

I've also applied some additional changes based on Rob feedback.
status-firefox57: --- → affected
Priority: P2 → P1
Unfortunately, its too late to land this in 57. Because this patch impacts content scripts which cover a large number of add-ons, it is too high risk for crash landing a day or two before merge. We know this impacts a few add-ons, but we'll aiming at landing this early in the Firefox 58 cycle instead.
status-firefox57: affected → wontfix
Unfortunate, altough it would be consequent to have at least a setting to enable legacy addons in 57 then (don't know how I can survice months without Greasemonky and my user scripts).
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review199772

::: dom/webidl/WebExtensionPolicy.webidl:111
(Diff revision 15)
> +  /**
> +   * The set of the dinamically registered content scripts.
> +   */
> +  [Cached, Pure, Frozen]
> +  readonly attribute sequence<WebExtensionContentScript> registeredContentScripts;

Again, there's no need for a separate property for this.

::: dom/webidl/WebExtensionPolicy.webidl:117
(Diff revision 15)
> +  /**
> +   * Clear all the registered content scripts.
> +   */
> +  void clearRegisteredContentScripts();

Again: There's no need for this.

::: dom/webidl/WebExtensionPolicy.webidl:128
(Diff revision 15)
> +  /**
> +   * Unregister a content script previously registered programmatically.
> +   */
> +  [Throws]
> +  void unregisterContentScript(DOMString scriptId);

Again: We should not be using IDs for things that we have actual object references for.

::: toolkit/components/extensions/ExtensionPolicyService.cpp:304
(Diff revision 15)
> +
> +    for (auto& script : policy->RegisteredContentScripts()) {
> +      if (script->Matches(aDocInfo)) {
> +        if (aIsPreload) {
> +          ProcessScript().PreloadContentScript(script);
> +        } else {
> +          ProcessScript().LoadContentScript(script, aDocInfo.GetWindow());
> +        }
> +      }
> +    }

Not necessary.

::: toolkit/components/extensions/WebExtensionPolicy.h:10
(Diff revision 15)
>  
>  #ifndef mozilla_extensions_WebExtensionPolicy_h
>  #define mozilla_extensions_WebExtensionPolicy_h
>  
>  #include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/WebExtensionContentScriptBinding.h"

Not necessary.

::: toolkit/components/extensions/WebExtensionPolicy.h:28
(Diff revision 15)
>  namespace mozilla {
>  namespace extensions {
>  
>  using dom::WebExtensionInit;
>  using dom::WebExtensionLocalizeCallback;
> +using dom::WebExtensionContentScriptInit;

Not necessary. Please create WebExtensionContentScript objects directly and then register them.

::: toolkit/components/extensions/WebExtensionPolicy.h:183
(Diff revision 15)
> +  nsTArray<nsString> mRegisteredContentScriptIds;
> +  nsTArray<RefPtr<WebExtensionContentScript>> mRegisteredContentScripts;

1) Please never, ever, use parellel arrays as associative arrays.
2) This is not necessary.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:210
(Diff revision 15)
>  
>    if (!EPS().UnregisterExtension(*this)) {
>      return false;
>    }
>  
> +  ClearRegisteredContentScripts();

Not necessary.

::: toolkit/components/extensions/ext-c-registerContentScript.js:1
(Diff revision 15)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

This should be named ext-c-contentScripts.js

::: toolkit/components/extensions/ext-c-registerContentScript.js:5
(Diff revision 15)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +// The ext-* files are imported into the same scopes.

They aren't. Only variables assigned to properties of the `global` object are accessible by other modules.

::: toolkit/components/extensions/ext-c-registerContentScript.js:6
(Diff revision 15)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +// The ext-* files are imported into the same scopes.
> +/* import-globals-from ../../../toolkit/components/extensions/ext-c-toolkit.js */

Please don't do this.

::: toolkit/components/extensions/ext-c-registerContentScript.js:12
(Diff revision 15)
> +
> +/**
> + * Represents a content script registered at runtime instead of being included in the
> + * addon manifest.
> + *
> + * @param {ExtensionPageContextChild}

Missing param name.

::: toolkit/components/extensions/ext-c-registerContentScript.js:13
(Diff revision 15)
> +/**
> + * Represents a content script registered at runtime instead of being included in the
> + * addon manifest.
> + *
> + * @param {ExtensionPageContextChild}
> + *   The extension context which has registered the content script.

Please align with opening curly brace of type name.

::: toolkit/components/extensions/ext-c-registerContentScript.js:36
(Diff revision 15)
> +      return this.context.cloneScope.Promise.reject(
> +        new this.context.cloneScope.Error("Unable to unregister a content script multiple times")
> +      );

`throw new ExtensionError(...)`

::: toolkit/components/extensions/ext-c-registerContentScript.js:45
(Diff revision 15)
> +
> +    await this.context.childManager.callParentAsyncFunction(
> +      "contentScripts.unregister", [this.scriptId]
> +    );
> +
> +    this.unregistered = true;

Set this before you await, or multiple calls will not be handled correctly.

::: toolkit/components/extensions/ext-c-registerContentScript.js:47
(Diff revision 15)
> +      "contentScripts.unregister", [this.scriptId]
> +    );
> +
> +    this.unregistered = true;
> +
> +    return undefined;

Unnecessary.

::: toolkit/components/extensions/ext-c-registerContentScript.js:54
(Diff revision 15)
> +
> +  api() {
> +    // TODO(rpl): allow to read the options related to the registered content script?
> +    return {
> +      unregister: () => {
> +        // NOTE: this is needed because an async function defined in the privileged

`return context.wrapPromise(this.unregister())`

::: toolkit/components/extensions/ext-registerContentScript.js:19
(Diff revision 15)
> +
> +class RegisteredContentScript {
> +  constructor({context, options}) {
> +    this.context = context;
> +    this.options = options;
> +    this.overriddenOptions = {};

Please just clone the original options object and then mutate it rather than storing two objects.

::: toolkit/components/extensions/ext-registerContentScript.js:22
(Diff revision 15)
> +    this.context = context;
> +    this.options = options;
> +    this.overriddenOptions = {};
> +    this.scriptId = getUniqueId();
> +    this.blobURLs = new Set();
> +    this.blobs = new Set();

This isn't necessary.

::: toolkit/components/extensions/ext-registerContentScript.js:43
(Diff revision 15)
> +    const {context} = this;
> +
> +    context.forgetOnClose(this);
> +
> +    for (const blobURL of this.blobURLs) {
> +      if (this.context.cloneScope) {

Please do this check outside of the `for` loop.

::: toolkit/components/extensions/ext-registerContentScript.js:76
(Diff revision 15)
> +
> +      return data;
> +    };
> +
> +    if (options.js && options.js.length > 0) {
> +      overriddenOptions.js = options.js.map(convertToURL, "text/javascript");

The second argument to `map` is the `this` object. The second argument to its callback is a numeric index.

::: toolkit/components/extensions/ext-registerContentScript.js:85
(Diff revision 15)
> +      overriddenOptions.css = options.css.map(convertToURL, "text/css");
> +    }
> +  }
> +
> +  serialize() {
> +    const {context, options, overriddenOptions, scriptId} = this;

This destructuring really does not help anything.

::: toolkit/components/extensions/ext-registerContentScript.js:99
(Diff revision 15)
> +// Keep track of the registered content scripts options in the process data,
> +// in a Map[extensionId -> scriptId -> scriptOptions].
> +const RegisteredContentScriptProcessData = {
> +  get scriptsProcessData() {
> +    // Store the registered content script in the initialProcessData, so that all
> +    // the existent registered content scripts are propagated to the
> +    // new content child processes (e.g. after a crashed tab is being restored).
> +    let data = Services.ppmm.initialProcessData;
> +    if (!data["Extension:RegisteredContentScript"]) {
> +      data["Extension:RegisteredContentScript"] = new Map();
> +    }
> +
> +    return data["Extension:RegisteredContentScript"];
> +  },
> +
> +  set(extension, scriptId, scriptOptions) {
> +    const scriptsProcessData = this.scriptsProcessData;
> +
> +    if (!scriptsProcessData.has(extension.id)) {
> +      scriptsProcessData.set(extension.id, new Map());
> +    }
> +
> +    const extensionScripts = scriptsProcessData.get(extension.id);
> +
> +    extensionScripts.set(scriptId, scriptOptions);
> +  },
> +
> +  unset(extension, scriptId) {
> +    const scriptsProcessData = this.scriptsProcessData;
> +    if (!scriptsProcessData.has(extension.id)) {
> +      return;
> +    }
> +
> +    const extensionScripts = scriptsProcessData.get(extension.id);
> +    extensionScripts.delete(scriptId);
> +  },
> +};

All of this is overkill. Please just add a Map to the initial process data for the extension and store it on the extension object.

::: toolkit/components/extensions/ext-registerContentScript.js:142
(Diff revision 15)
> +this.contentScripts = class extends ExtensionAPI {
> +  getAPI(context) {
> +    const {extension} = context;
> +
> +    // Set of the script ids registered by an extension context.
> +    const scriptIds = new Set();

`new Map()`

::: toolkit/components/extensions/ext-registerContentScript.js:147
(Diff revision 15)
> +    const scriptIds = new Set();
> +
> +    // Unregister all the scriptId related to a context when it is closed.
> +    context.callOnClose({
> +      close() {
> +        const extensionRegisteredScripts = registeredContentScripts.get(extension);

Please store this as a property of the ExtensionAPI object rather than doing repeated WeakMap lookups.

::: toolkit/components/extensions/ext-registerContentScript.js:172
(Diff revision 15)
> +              return Promise.reject({
> +                message: `Cannot register a content script for ${origin}`,
> +              });

`throw new ExtensionError(...)`

::: toolkit/components/extensions/ext-registerContentScript.js:206
(Diff revision 15)
> +
> +          const extensionRegisteredScripts = registeredContentScripts.get(extension);
> +
> +          if (!scriptIds.has(scriptId)) {
> +            return Promise.reject({
> +              message: `No ${scriptId} content script registered by this context has been found`,

This is an internal error. There's no sense in propagating it to the extension.

::: toolkit/components/extensions/extension-process-script.js:195
(Diff revision 15)
> +
> +      for (let script of extension.registeredContentScripts) {
> +        if (script.matchesWindow(window)) {
> +          contentScripts.get(script).injectInto(window);
> +        }
> +      }

Unnecessary.

::: toolkit/components/extensions/extension-process-script.js:290
(Diff revision 15)
> +    Services.cpmm.addMessageListener("Extension:UnregisterContentScripts", this);
>  
>      let procData = Services.cpmm.initialProcessData || {};
>  
>      for (let data of procData["Extension:Extensions"] || []) {
> -      this.initExtension(data);
> +      this.initExtension(data, procData["Extension:RegisteredContentScript"]);

No need for a separate object for this.

::: toolkit/components/extensions/extension-process-script.js:343
(Diff revision 15)
>  
> +    // Register any existent dinamically registered content script for the extension
> +    // when a content process is started for the first time (which also cover
> +    // a content process that crashed and it has been recreated).
> +    if (registeredContentScriptsMap && registeredContentScriptsMap.has(policy.id)) {
> +      for (let [, scriptData] of registeredContentScriptsMap.get(policy.id)) {

There's no sense in making this a map if you ignore the keys and store the ID in the value, anyway. Please just store a map of id -> options.

::: toolkit/components/extensions/extension-process-script.js:391
(Diff revision 15)
> +        if (!policy) {
> +          throw new Error(`Unexpected undefined policy on "${data.id}"`);
> +        }

This is going to result in the broadcast() caller waiting forever for a response, and nothing useful being done with the error.

Any `broadcast` message listener needs to always send a "Completed" response.

::: toolkit/components/extensions/schemas/content_scripts.json:18
(Diff revision 15)
> +            "$ref": "manifest.ExtensionURL"
> +          },
> +          {
> +            "type": "object",
> +            "properties": {
> +              "text": {

This should be called "code", to match the naming elsewhere. But it's strange to have this be an array of sometimes-strings, sometimes-single-property-objects. It should probably be a separate "jsCode" array.

::: toolkit/components/extensions/schemas/content_scripts.json:37
(Diff revision 15)
> +          "exclude_matches": {
> +            "type": "array",
> +            "optional": true,
> +            "minItems": 1,
> +            "items": { "$ref": "manifest.MatchPattern" }
> +          },
> +          "include_globs": {
> +            "type": "array",
> +            "optional": true,
> +            "items": { "type": "string" }
> +          },
> +          "exclude_globs": {

These should be camelCase.

::: toolkit/components/extensions/schemas/content_scripts.json:65
(Diff revision 15)
> +          "all_frames": {"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."},
> +          "match_about_blank": {"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>."},
> +          "run_at": {

These should be camelCase.
Attachment #8862577 - Flags: review?(kmaglione+bmo) → review-
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review201570


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/WebExtensionPolicy.cpp:222
(Diff revision 16)
> +                                          ErrorResult& aRv)
> +{
> +  // Raise an abort error if the script is already registered.
> +  int32_t pos = mContentScripts.IndexOf(&script);
> +  if (pos >= 0) {
> +    aRv.Throw(NS_ERROR_ABORT);

Error: You must immediately return after calling this function [clang-tidy: mozilla-must-return-from-caller]
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review182364

> Does not need to be part of this patch, but would it make sense to add a schema attribute to state that the return value should be cloned with `cloneFunctions:true`? That would make the ext-c code prettier.

I do agree, but it would be nicer to also wrap the API object with the schema wrappers like we do for the API namespaces (e.g. so that the API call parameters are validated by the schema wrappers).
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review199772

> This should be called "code", to match the naming elsewhere. But it's strange to have this be an array of sometimes-strings, sometimes-single-property-objects. It should probably be a separate "jsCode" array.

yeah, I agree, I opted to change it to always use an object:

- `{code: "..."}` when the content script (or style) is going to be loaded from a string
- `{file: "..."}` when the content script (or style) is going to be loaded from an extension url
Hi Kris,
I've reworked the patch based on the review comments, this new version should be much more near to the goal.

(I'm really sorry that the previous version of the patch didn't included all the suggestions yet, but thanks to our brief chat over IRC, I finally cleared all my doubts, in particular the ones described in Comment 65, and it was finally clear to me what kind of changes this patch still needed to match your advices in the review comments).
Attachment #8862577 - Flags: review?(mixedpuppy)
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review203042

::: dom/webidl/WebExtensionPolicy.webidl:70
(Diff revision 19)
>    attribute MatchPatternSet allowedOrigins;
>  
>    /**
>     * The set of content scripts active for this extension.
>     */
> -  [Cached, Constant, Frozen]
> +  [Cached, Pure, Frozen]

Nit: Please keep sorted.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:220
(Diff revision 19)
> +void
> +WebExtensionPolicy::RegisterContentScript(WebExtensionContentScript& script,
> +                                          ErrorResult& aRv)
> +{
> +  // Raise an abort error if the script is already registered.
> +  if (mContentScripts.Contains(&script)) {

Also need to check `script.mExtension == this`

::: toolkit/components/extensions/WebExtensionPolicy.cpp:221
(Diff revision 19)
> +WebExtensionPolicy::RegisterContentScript(WebExtensionContentScript& script,
> +                                          ErrorResult& aRv)
> +{
> +  // Raise an abort error if the script is already registered.
> +  if (mContentScripts.Contains(&script)) {
> +    aRv.Throw(NS_ERROR_ABORT);

Nit: `NS_ERROR_INVALID_ARG`

::: toolkit/components/extensions/WebExtensionPolicy.cpp:226
(Diff revision 19)
> +    aRv.Throw(NS_ERROR_ABORT);
> +    return;
> +  }
> +
> +  RefPtr<WebExtensionContentScript> newScript = &script;
> +  mContentScripts.AppendElement(Move(newScript));

This should be something like:

    if (mContentScripts.AppendElement(Move(newScript), fallible) {
      ClearCachedContentScripts();
    } else {
      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
    }

::: toolkit/components/extensions/WebExtensionPolicy.cpp:235
(Diff revision 19)
> +
> +void
> +WebExtensionPolicy::UnregisterContentScript(const WebExtensionContentScript& script,
> +                                            ErrorResult& aRv)
> +{
> +  mContentScripts.RemoveElement(&script);

This should throw if `RemoveElement` returns false, otherwise there's no point in making this function fallible.

::: toolkit/components/extensions/ext-c-contentScripts.js:19
(Diff revision 19)
> + *        The extension context which has registered the content script.
> + * @param {string} scriptId
> + *        An unique id that represents the registered content script
> + *        (generated and used internally to identify it across the different processes).
> + */
> +class ChildRegisteredContentScript {

Nit: `ContentScriptChild`

::: toolkit/components/extensions/ext-c-contentScripts.js:25
(Diff revision 19)
> +    context.callOnClose(this);
> +  }
> +
> +  close() {
> +    // NOTE: there is no need to send an unregister when the context is closed,
> +    // because its main process counterpart will be automatically unregistered
> +    // when the context is closed.
> +    this.context = null;
> +  }

No need for `callOnClose` just to set `context` to null.

::: toolkit/components/extensions/ext-c-contentScripts.js:38
(Diff revision 19)
> +  }
> +
> +  async unregister() {
> +    if (this.unregistered) {
> +      throw new ExtensionError(
> +        "Unable to unregister a content script multiple times"

Nit: "Content script already unregistered"

::: toolkit/components/extensions/ext-c-contentScripts.js:39
(Diff revision 19)
> +
> +  async unregister() {
> +    if (this.unregistered) {
> +      throw new ExtensionError(
> +        "Unable to unregister a content script multiple times"
> +      );

Nit: Move to previous line.

::: toolkit/components/extensions/ext-c-contentScripts.js:46
(Diff revision 19)
> +
> +    this.unregistered = true;
> +
> +    await this.context.childManager.callParentAsyncFunction(
> +      "contentScripts.unregister", [this.scriptId]
> +    );

Nit: Move to previous line.

::: toolkit/components/extensions/ext-c-contentScripts.js:67
(Diff revision 19)
> +      contentScripts: {
> +        register(options) {
> +          return context.cloneScope.Promise.resolve().then(async () => {
> +            const scriptId = await context.childManager.callParentAsyncFunction(
> +              "contentScripts.register", [options]
> +            );

Nit: Move to previous line.

::: toolkit/components/extensions/ext-c-contentScripts.js:72
(Diff revision 19)
> +            );
> +
> +            const registeredScript = new ChildRegisteredContentScript(context, scriptId);
> +
> +            return Cu.cloneInto(registeredScript.api(), context.cloneScope, {
> +              cloneFunctions: true,

Nit: `{cloneFunctions: true}`

::: toolkit/components/extensions/ext-contentScripts.js:6
(Diff revision 19)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* exported registerContentScript, unregisterContentScript */
> +/* global registerContentScript: true, unregisterContentScript: true */

No need for `: true`

::: toolkit/components/extensions/ext-contentScripts.js:27
(Diff revision 19)
> + * @param {RegisteredContentScriptOptions} details
> + *        The options object related to the registered content script
> + *        (which has the properties described in the content_scripts.json
> + *        JSON API schema file).
> + */
> +class ParentRegisteredContentScript {

Nit: `ContentScriptParent`

::: toolkit/components/extensions/ext-contentScripts.js:49
(Diff revision 19)
> +      throw new Error("Unable to destroy RegisterContentScript twice");
> +    }
> +
> +    this.destroyed = true;
> +
> +    const {context} = this;

This is unnecessary.

::: toolkit/components/extensions/ext-contentScripts.js:53
(Diff revision 19)
> +
> +    const {context} = this;
> +
> +    context.forgetOnClose(this);
> +
> +    if (this.context.cloneScope) {

This should be unnecessary. Are there cases where this winds up being null?

::: toolkit/components/extensions/ext-contentScripts.js:70
(Diff revision 19)
> +  _convertOptions(details) {
> +    const {context} = this;
> +
> +    const options = {
> +      matches: details.matches,
> +      exclude_matches: details.exclude_matches,

`details.excludeMatches`

::: toolkit/components/extensions/ext-contentScripts.js:136
(Diff revision 19)
> +        }
> +
> +        const scriptIds = Array.from(parentScriptsMap.keys());
> +
> +        for (let scriptId of scriptIds) {
> +          parentScriptsMap.delete(scriptId);

Nit: Unnecessary.

::: toolkit/components/extensions/ext-contentScripts.js:152
(Diff revision 19)
> +              throw new ExtensionError(
> +                `Cannot register a content script for ${origin}`
> +              );

Nit: One line.

Also, "Permission denied to register content script ..."

::: toolkit/components/extensions/ext-contentScripts.js:183
(Diff revision 19)
> +        // the extension code will call script.unregister on the script API object
> +        // that is resolved from the register API method returned promise.
> +        async unregister(scriptId) {
> +          if (!parentScriptsMap.has(scriptId)) {
> +            Cu.reportError(new Error(
> +              `No ${scriptId} content script registered by this context has been found`

Nit: Something like `No such content script ID: ${scriptId}`

::: toolkit/components/extensions/ext-contentScripts.js:184
(Diff revision 19)
> +        // that is resolved from the register API method returned promise.
> +        async unregister(scriptId) {
> +          if (!parentScriptsMap.has(scriptId)) {
> +            Cu.reportError(new Error(
> +              `No ${scriptId} content script registered by this context has been found`
> +            ));

Nit: Move to previous line.

::: toolkit/components/extensions/ext-contentScripts.js:191
(Diff revision 19)
> +            return;
> +          }
> +
> +          const contentScript = parentScriptsMap.get(scriptId);
> +
> +          if (contentScript) {

This check is redundant. Please just move the `get` call above, and replace the `has` check with `if (!contentScript)`

::: toolkit/components/extensions/ext-contentScripts.js:200
(Diff revision 19)
> +              id: extension.id,
> +              scriptIds: [scriptId],
> +            });
> +          }
> +
> +          extension.registeredContentScripts.delete(scriptId);

Please do this at the same time you delete it from `parentScriptMap`

::: toolkit/components/extensions/extension-process-script.js:416
(Diff revision 19)
> +        let policy = WebExtensionPolicy.getByID(data.id);
> +
> +        if (policy) {
> +          if (this.registeredContentScripts.has(data.scriptId)) {
> +            Cu.reportError(new Error(
> +              `Registering content script ${data.scriptId} on ${data.id} more than once`
> +            ));
> +          }
> +
> +          const parsedOptions = parseScriptOptions(data.options);
> +          const script = new WebExtensionContentScript(policy, parsedOptions);
> +          this.registeredContentScripts.set(data.scriptId, script);
> +          policy.registerContentScript(script);
> +        }

Please put a try-catch block around this.

::: toolkit/components/extensions/extension-process-script.js:422
(Diff revision 19)
> +
> +        if (policy) {
> +          if (this.registeredContentScripts.has(data.scriptId)) {
> +            Cu.reportError(new Error(
> +              `Registering content script ${data.scriptId} on ${data.id} more than once`
> +            ));

Please move this to the previous line.

Also, need to bail out early here.

::: toolkit/components/extensions/extension-process-script.js:438
(Diff revision 19)
> +        if (policy) {
> +          for (const scriptId of data.scriptIds) {
> +            const script = this.registeredContentScripts.get(scriptId);
> +            if (script) {
> +              policy.unregisterContentScript(script);
> +              this.registeredContentScripts.delete(scriptId);
> +            }
> +          }
> +        }

Try-catch here too, please.

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:179
(Diff revision 19)
> +  deepEqual(policy.contentScripts, [script1],
> +            "script1 has been added to the policy contentScripts");
> +
> +  Assert.throws(
> +    () => policy.registerContentScript(script1),
> +    /NS_ERROR_ABORT/,

Nit: `e => e.result == Cr.NS_ERROR_INVALID_ARG`

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:180
(Diff revision 19)
> +            "script1 has been added to the policy contentScripts");
> +
> +  Assert.throws(
> +    () => policy.registerContentScript(script1),
> +    /NS_ERROR_ABORT/,
> +    "Got the expected NS_ERROR_ABORT when trying to register a script more than once"

Nit: Please move to previous line.

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:181
(Diff revision 19)
> +
> +  Assert.throws(
> +    () => policy.registerContentScript(script1),
> +    /NS_ERROR_ABORT/,
> +    "Got the expected NS_ERROR_ABORT when trying to register a script more than once"
> +  );

Please also test that trying to register a content script for the wrong extension throws.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js:19
(Diff revision 19)
> +const BASE_URL = `http://localhost:${server.identity.primaryPort}/data`;
> +
> +function check_applied_styles() {
> +  const urlElStyle = window.getComputedStyle(
> +    document.querySelector("#registered-extension-url-style")
> +  );

Please move to previous line.

Also, no need for `window.`

::: toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js:22
(Diff revision 19)
> +  const urlElStyle = window.getComputedStyle(
> +    document.querySelector("#registered-extension-url-style")
> +  );
> +  const blobElStyle = window.getComputedStyle(
> +    document.querySelector("#registered-extension-text-style")
> +  );

Please move to previous line.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js:46
(Diff revision 19)
> +      runAt: "document_start",
> +    });
> +
> +    let textScript = await browser.contentScripts.register({
> +      css: [{code: cssCode}],
> +      matches: ["http://localhost/*/file_sample_registered_styles.html"],

Please add tests for all properties, including `excludeMatches`, `includeGlobs`, and `excludeGlobs`
Attachment #8862577 - Flags: review?(kmaglione+bmo) → review+
Attachment #8862577 - Flags: review?(mixedpuppy)
Attachment #8862577 - Flags: review?(amarchesini)
Comment on attachment 8862577 [details]
Bug 1332273 - Support programmatically registered content scripts.

https://reviewboard.mozilla.org/r/134442/#review206014

r+ for the DOM part.
Attachment #8862577 - Flags: review?(amarchesini) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/54ce507fdd65
Support programmatically registered content scripts. r=baku,kmag
https://hg.mozilla.org/mozilla-central/rev/54ce507fdd65
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Keywords: dev-doc-needed
Summary: Pass initial options to content scripts without async messaging (contentScriptOptions equivalent) → Implement a contentScripts.register API
Depends on: 1419576
Thanks for creating this API. I have two suggestions to make:
1. at the moment the registered content scripts are executed after the content scripts defined in the manifest. I would really like to run the registered before. Or even better: be able to specify if they run before or after. I can see applications for both.
2. If the first is not able I would at least want to be able to tell if registering content script is supported by the browser in a manifest content script. I do not know of a way to do this since browser.contentScript is complete missing in content scripts (and browser.runtime.getBrowserInfo as well). It makes few sense to register a content script in a content script, but be able to tell if the background script could do it makes a lot of it. 

The background is that I need the content of the storage in the content script in a synchronous way. This can easily be done with content scripts. But I do also want to support older Firefox versions which do not support this new API (esp. FF ESR 52 which will stick around for quite a time). I can check in the background script if it's supported and start my workarounds (like they are done at the moment...) but in the content script I'm not able to tell if I need the receiving end of the workaround or not.

PS: a third way of solving the issue would be an API which can unregister content scripts from the manifest. But I think this would be the most complicated one.
Will this be back-ported to FF 58?

Amazing however, first tests with GM & uses of this are working perfectly ( https://github.com/greasemonkey/greasemonkey/issues/2663 ). 

status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(In reply to sgl4kn from comment #88)
> Will this be back-ported to FF 58?

No, this is a large patch and should go through the Nightly, Beta stabilisation cycle. It's great that its working for you and we'd like to feedback and testing on it through that cycle. Thanks for doing that.
I've written some docs on this:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts

The compat data is still in review[1] so the tables aren't there yet, but they will be soon. There's also an example in review[2].

[1] https://github.com/mdn/browser-compat-data/pull/841
[2] https://github.com/mdn/webextensions-examples/pull/325

I also updated this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Loading_content_scripts to mention this new way to load content scripts. I can't think of anywhere else we need updates.

Please let me know if this covers it.
Flags: needinfo?(lgreco)
Thanks Will, the docs looks great, follows some additional notes that you may want to include:

- In the MDN doc page (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts
) we should also mention that the userScripts are automatically unregistered when the page that has created them
is destroyed (e.g. if they are registered from the background page they will also be unregistered automatically
when the background page is destroyed, and if they are registered from a sidebar, or a popup, they are going to be
unregistered automatically when the sidebar, or the popup, is being destroyed)

- In the compat doc data "unregister" seems to be part of the "contentScripts" API namespace, but it is actually only 
  exposed by the RegisteredContentScript objects, and so I was wondering if it should be moved inside "RegisteredContentScript" (it seems that we used this approach for other APIs only available into API objects, e.g. for the devtools panel API object: https://github.com/mdn/browser-compat-data/blob/de8dadfa280cb61064a0b797538bb9cf66e94f4e/webextensions/api/devtools.json#L207-L208)
Flags: needinfo?(lgreco)
Thanks :rpl!

(In reply to Luca Greco [:rpl] from comment #91)
> 
> - In the compat doc data "unregister" seems to be part of the
> "contentScripts" API namespace, but it is actually only 
>   exposed by the RegisteredContentScript objects, and so I was wondering if
> it should be moved inside "RegisteredContentScript"

I think it is scoped under RegisteredContentScript, isn't it:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts#Browser_compatibility
?

Or do I not understand you?
Flags: needinfo?(lgreco)
(In reply to Will Bamberg [:wbamberg] from comment #92)
> Thanks :rpl!
> 
> (In reply to Luca Greco [:rpl] from comment #91)
> > 
> > - In the compat doc data "unregister" seems to be part of the
> > "contentScripts" API namespace, but it is actually only 
> >   exposed by the RegisteredContentScript objects, and so I was wondering if
> > it should be moved inside "RegisteredContentScript"
> 
> I think it is scoped under RegisteredContentScript, isn't it:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/
> contentScripts#Browser_compatibility
> ?
> 
> Or do I not understand you?

Never mind, I was looking at the compat table pull request and 
I wasn't reading the json correctly, now that the compat table is rendered in the MDN doc page 
(it wasn't yet rendered yesterday) I definitely agree that unregister is scoped exactly where it should be!

Thanks for double-checking it, it looks great!
Flags: needinfo?(lgreco)
(In reply to Luca Greco [:rpl] from comment #91)

> In the MDN doc page (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts we should also mention that...

Thanks Luca, I've added a bit on that that.
Keywords: dev-doc-needed → dev-doc-complete
The attached user-script-extended-example.xpi is an extended version of the user-script example from the mdn/webextensions-examples repository
(the source of the original extension is https://github.com/mdn/webextensions-examples/tree/master/user-script, and the modified version in a branch of my fork at: https://github.com/rpl/webextensions-examples/tree/wip/extend-user-script-example/user-script).

The extended user-script extension includes:

- all the supported properties of contentScripts.register have been included in the extension popup (so that we can register a contentScripts by use more combinations of the supported properties)

- store the properties value used in the popup into the extension storage,
  and restore the last stored values when the popup is reopened (so that there is no need of type all properties for every combination of the properties that we want to verify explicitly)

- print the last error raised by contentScripts.register in the popup (so that it is easier to know if the content script has never been registered because the combination of the properties is invalid), and store the last error raised in the extension storage (so that it is still visible when the popup has been closed and opened again)

Follows the STR to use to verify this bugzilla issue.

Install the test extension, open the extension popup and register a content script which includes the following parameters:

  - Scenario 01: matches option
      hosts: *://*.org/*
      code: document.body.innerHTML = '<h1>This page has been eaten<h1>'
    
    Expected behaviors:
      - navigate to mozilla.org => The script message is rendered in the tab
      - navigate to developer.mozilla.org => The script message is rendered
      - navigate to example.com => The script message is NOT rendered in the tab

  - Scenario 02: matches and exclude matches options 
      hosts: *://*.org/*
      excludeMatches: *://developer.mozilla.org/*
    
    Expected behaviors:
      - navigate to mozilla.org => The script message is rendered in the tab
      - navigate to developer.mozilla.org => The script message is NOT rendered

  - Scenario 03: matches, include globs and exclude globs
      hosts: *://*.org/*
      includeGlobs: */en-US/*
      excludeGlobs: */en-US/firefox/*
    
    Expected behaviors:
      - navigate to mozilla.org/en-US => The script message is rendered
      - navigate to mozilla.org/es-US/firefox/ => The script message is NOT rendered

  - Scenario 04: runAt = "document_end"
    repeat the scenario from 01 to 03 with runAt set to document_end

  - Scenario 05: runAt = "document_start"
      hosts: *://*.org/*
      code: console.log("Script loaded", window.location.href, document.readyState);

    Expected behaviors:
     - navigate to mozilla.org and open the developer tools => 
       the expected message is logged in the devtools webconsole 
       (with the expected url and readyState "loading", or "interactive") 

  - Scenario 06: 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 07: allFrames=true
      hosts: <all_urls>
      allFrames: true
      code: console.log("Script injected into", window.location.href);
    
    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)

  - Scenario 08: contentScript.register "invalid excludeMatches" error
      hosts: "*://*.org/*
      excludeMatches: about:addons
    
    Expected behaviors:
      - an error message has been shown in the extension popup =>
        'Type error for parameter contentScriptOptions (Error processing excludeMatches.0: Value "about:addons" must either: ...'
Attached image Bug1332273.gif — 
This issue is verified as fixed on Firefox 60.0a1 (20180201100326) and Firefox 59.0b5 (20180128191456) under Wind 7 64-bit and Mac OS X 10.13.2

All the scenarios from comment 95 have passed the tests.

I added a video where you can see that the issue is reproducible on Firefox 58.0.1 (20180128191252) and fixed on Firefox 60.0a1 (20180201100326)

Thanks Luca!
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: --- → verified
Blocks: 1437098
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: