1247455 - Add a .removeCSS method to complement .insertCSS

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Nancy Grossman]

There should be a way to remove stylesheets that are added with chrome.tabs.insertCSS. That way a WebExtension like Stylish can add and remove its own stylesheets without interfering with, or interference from, the website. A chrome.tabs.removeCSS method would do nicely, or else a new optional 'remove' property, defaulting to false, in the 'details' argument passed to .insertCSS.

It would also be useful to insert a User or User Agent sheet this way instead of an Author sheet. Some elements simply can't be styled except by a UA sheet. This could be specified with a new optional 'origin' property, defaulting to Author, in the 'details' argument.

Finally, it would be great if, in addition to or instead of these changes, the stylesheet service could be exposed as an additional, global .insertCSS method, grafted to chrome.windows or maybe chrome.runtime. This would permit Stylish, for example, to continue life as a WebExtension without losing its ability to style scrollbars and browser chrome.

Comment 1

[Claas Augner [:claas]]

I have started to work on this bug at the 3rd Open Mozilla Night in Berlin and will share my results within the next days.

Comment 2

[Bhanu Gupta]

I am new here, want to start with something. I am good with SQL, and logic. I have some experience in C/C++, and Java. Help me with what should I do.

Comment 3

[Claas Augner [:claas]]

It took a bit longer due to personal reasons, but here are my results from the 3rd Open Mozilla Night based on how "insertCSS" works:
- The entry point is the path |/browser/components/extensions| (found by search for "insertCSS" in DXR).
- The `insertCSS` method is documented in |schemas/tabs.json|.
- The implementation can be found in |ext-tabs.js| where it delegates to an existing `_execute` method which sends an 'Extension:Execute' message to the context.
- Note: This context is the `ExtensionContext` (see |/toolkit/components/extensions/ExtensionContent.jsm|).
- The context receives the message in `receiveMessage`, forwards it to `handleExtensionExecute` which delegates to `DocumentManager#executeScript` (in same file).
- Note: For the sake of completeness, these messages are managed by the `MessageChannel` (see |/toolkit/components/extensions/MessageChannel.jsm|), but this is not essential for this bug.
- Note: The 'Extension:Execute' message type seems to have been first implemented for the `executeScript` method only and then been reused for `insertCSS`.
- The `executeScript` method creates an instance of `Script` (which, as of now, can represent JavaScript or CSS content to be added) and adds it to the context (see `addScript`) which calls `execute` which invokes `tryInject` on the script.
- The `Script#tryInject` method makes use of `nsIDOMWindowUtils#loadSheetUsingURIString` for adding the CSS (see |dom/interfaces/base/nsIDOMWindowUtils.idl|).
- Note: Inline CSS is converted to a data URI.
- Note: There seems to be a bug in the CSS section of `Script#tryInject`, because the `deferred` promise is resolved within the for-loop (so upon the first CSS insertion).

Hence, the solution for this bug involves:
- ExtensionContext.jsm: Extend `Script` to support the removal of CSS (e.g. a new boolean option 'removeCss' which defaults to false and controls whether `tryInject` would call `removeSheetUsingURIString` rather than `loadSheetUsingURIString`).
- ext-tabs.js: Add `removeCSS` method and extend `_execute` to set `removeCss` option when called by this new method.
- tabs.json: Add `removeCSS` documentation.

I will submit a first patch these days.

Comment 4

[Claas Augner [:claas]]

Attachment: 1247455.patch

The patch contains the following changes:
- ext-tabs.js: Add removeCSS method.
- tabs.json: Add removeCSS schema.
- browser_ext_tabs_removeCSS.js: Add test case.
- ExtensionContent.jsm: Handle removeCSS (and reduce code duplication).

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8740680 [details] [diff] [review]
1247455.patch

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

Looks good. Thanks! Just a couple of nits to fix and it will be ready to land.

::: browser/components/extensions/test/browser/browser_ext_tabs_removeCSS.js
@@ +109,5 @@
> +  // Make sure that we're not holding on to references to closed message
> +  // managers.
> +  is(MessageChannel.messageManagers.size, messageManagersSize, "Message manager count");
> +  is(MessageChannel.responseManagers.size, responseManagersSize, "Response manager count");
> +  is(MessageChannel.pendingResponses.size, 0, "Pending response count");

These checks aren't really necessary here. We just need to have them in in one test.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +202,5 @@
>      if (shouldRun("document_start")) {
>        let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                             .getInterface(Ci.nsIDOMWindowUtils);
> +      // We can insertCSS and removeCSS.
> +      let method = this.remove_css ? winUtils.removeSheetUsingURIString : winUtils.loadSheetUsingURIString;

Can we move this to just before the `for` loop that calls the method?

@@ +205,5 @@
> +      // We can insertCSS and removeCSS.
> +      let method = this.remove_css ? winUtils.removeSheetUsingURIString : winUtils.loadSheetUsingURIString;
> +
> +      // We can handle CSS urls (css) and CSS code (cssCode).
> +      let urls = [];

Can we name this something like `cssUrls` so it's a bit clearer what it does?

Comment 6

[Claas Augner [:claas]]

Attachment: 1247455-v2.patch

The patch contains the changes from the previous patch plus the following changes:
- browser_ext_tabs_removeCSS.js: Remove unnecessary checks.
- ExtensionContent.jsm: Move method determination to where it is needed.
- ExtensionContent.jsm: Rename `urls` array to `cssUrls`.

PS: Thank you for the review.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8741547 [details] [diff] [review]
1247455-v2.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_removeCSS.js
@@ +5,5 @@
> +add_task(function* testExecuteScript() {
> +  let {MessageChannel} = Cu.import("resource://gre/modules/MessageChannel.jsm", {});
> +
> +  let messageManagersSize = MessageChannel.messageManagers.size;
> +  let responseManagersSize = MessageChannel.responseManagers.size;

These variables and the MessageChannel import also need to be removed, now. ESLint won't accept the unused variables.

Comment 8

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=561ca3b2e88e

Comment 9

[Claas Augner [:claas]]

Attachment: Add a .removeCSS method to complement .insertCSS

Comment 10

[Claas Augner [:claas]]

Comment on attachment 8742158 [details] [diff] [review]
Add a .removeCSS method to complement .insertCSS

The patch contains the changes from the previous patch plus the following changes:
- browser_ext_tabs_removeCSS.js: Remove unnecessary import/variables.

I ran `./mach eslint browser/components/extensions/test/browser/browser_ext_tabs_removeCSS.js` without errors.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8742158 [details] [diff] [review]
Add a .removeCSS method to complement .insertCSS

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

Thanks!

Comment 12

[Kris Maglione [:kmag]]

If you're ready for this to land, please add the checkin-needed keyword

Comment 13

[Claas Augner [:claas]]

Thanks, Kris!

Successfully checked that the patch still works, therefore I'm adding the keyword now.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be67702cf86e

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Eric Shepherd [:sheppy]]

I've documented tabs.removeCSS() here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/removeCSS

I've updated tabs.insertCSS to refer to it: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/insertCSS

I've updated the main tabs page to include a mention of the function: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs

I've updated the compatibility data JSON with this PR: https://github.com/mdn/browser-compat-data/pull/24.

I've added a note to Firefox 49 for developers.

Once the PR is accepted and deployed, this should be done. Needinfo wbamberg about that.

Comment 17

[Nancy Grossman]

Does the 'details.code' parameter need a warning? In `tabs.removeCSS` it simply identifies a stylesheet applied with `tabs.insertCSS` - it isn't evaluated as a stylesheet. In `tabs.insertCSS` it _is_ evaluated as a stylesheet in the context of a browser page, but is there really an XSS issue here? The page linked in the warning doesn't mention one. If it can indeed be done, then the XSS page should elaborate so that extension writers can know how to avoid it.

Similarly, `tabs.removeCSS` won't inject the `details.file` stylesheet but only uses the URL to identify a stylesheet already applied by `insertCSS`.

Comment 18

[Will Bamberg [:wbamberg]]

(In reply to Nancy Grossman from comment #17)
> Does the 'details.code' parameter need a warning? In `tabs.removeCSS` it
> simply identifies a stylesheet applied with `tabs.insertCSS` - it isn't
> evaluated as a stylesheet. In `tabs.insertCSS` it _is_ evaluated as a
> stylesheet in the context of a browser page, but is there really an XSS
> issue here? The page linked in the warning doesn't mention one. If it can
> indeed be done, then the XSS page should elaborate so that extension writers
> can know how to avoid it.
> 
> Similarly, `tabs.removeCSS` won't inject the `details.file` stylesheet but
> only uses the URL to identify a stylesheet already applied by `insertCSS`.

Yes, agreed. I've updated the page, would you mind taking a look and seeing if it's better now?

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

I've also drafted a simple example: https://github.com/mdn/webextensions-examples/pull/85

Comment 19

[Nancy Grossman]

That's very good. The warnings are gone, and the examples are clear and well-ordered. Thank you.

It may seem like a minor change to you, but the documentation is always so good here that when I see a warning like that I have to follow it up, even when I'm 99% sure of what I'll find. No doubt others do too.