1269141 - Content script CSS should be removed when disabling an add-on

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[drugan]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160425115943

Steps to reproduce:

userAgent: "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0"
OR userAgent: "Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0"
OR userAgent: "Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0"

Steps to reproduce:

1. Create content.css file in an extension directory and for demonstration purposes put this rule into the file:

body{border:10px dashed red}

2. Put this entry into manifest.json of the extension:

"content_scripts": [{"matches": ["<all_urls>"],"all_frames": true,"css": ["content.css"]}]

3. Open a couple of tabs and then install the extension. Check the tabs for the content.css file injected and having red border as expected.
4. Open Browser Toolbox console.
5. Disable/enable the extension.



Actual results:

What happened is the uncaught Exception in the console after re-enabling:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMWindowUtils.loadSheetUsingURIString]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/ExtensionUtils.jsm :: runSafeSyncWithoutClone :: line 27"  data: no] ExtensionUtils.jsm:30:0



Expected results:

What expected it is a clear console with no Exceptions/errors.

For the first look it might seem a minor issue having no impact on the extension or browser functionality/perfomance/memory leak but it excludes getting "Full Review" status for webextensions on https://addons.mozilla.org. Only "Preliminary Review" which is a bit pity and might prevent developers from adopting this new way of building extensions for Firefox.

Comment 1

[Nancy Grossman]

Attachment: Demo WE: content scripts run once per enable, not once per page load

Comment 2

[Nancy Grossman]

(In reply to drugan from comment #0)
> 3. Open a couple of tabs and then install the extension. Check the tabs for
> the content.css file injected and having red border as expected.

Oh my, that was unexpected. A Chrome extension doesn't immediately affect open tabs when it's enabled; content stylesheets aren't applied and content scripts aren't run until the page is reloaded. Likewise, content stylesheets remain in place and content scripts continue to run after the extension is disabled.

If I install the same extension in Firefox (46), content scripts and stylesheets immediately run in every tab. If I disable the extension and re-enable it, the content scripts run a second time in a second namespace.

I tested with this content script:

id = Date.now();
console.info("in re: https://bugzilla.mozilla.org/show_bug.cgi?id=1269141", id, window.id);
document.body.addEventListener('click', function (ev) {
  console.warn('Ouch! Stop that!', id, window.id)
});

In Firefox, each time the extension is enabled you'll see one more warning per click, with a different `id`. Also, `window.id` isn't `id` as it is in Chrome, but that may be unrelated.

Comment 3

[drugan]

(In reply to Nancy Grossman from comment #2)

> Oh my, that was unexpected. A Chrome extension doesn't immediately affect
> open tabs when it's enabled; content stylesheets aren't applied and content
> scripts aren't run until the page is reloaded. Likewise, content stylesheets
> remain in place and content scripts continue to run after the extension is
> disabled.

No, I am not agree. The Firefox behaviour is what we expect from the browser with a RESTARTLESS extension installed. Yes, developers could be forced manually to inject content scripts into already opened "matches" as they do in Chrome and Opera but why? There is no difference between a tab.url opened before the extension was installed/enabled and after that. It is the same matched tab in which content scripts have to work. Further more, some novice developers could miss/ignore the need/possibility to inject scripts and as a consequence to clutter their extension description with annoying messages like "Please, do not forget to reload opened tabs!". As the most of users (I am) do not like to read, instead  in a hurry to click so we will have a lot of comments/support requests on the issue which is not exist really.

If a developer don't want to inject scripts globally through manifest.json into "matches" there are tabs.executeScript() and tabs.insertCSS() for them, I think.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1269141: Remove content script CSS when disabling an extension.

Review commit: https://reviewboard.mozilla.org/r/57758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57758/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8760033 [details]
Bug 1269141: Remove content script CSS when disabling an extension.

https://reviewboard.mozilla.org/r/57758/#review54806

Looks good to me.  Script is getting heavily overloaded and reaching the point where it would benefit from some refactoring but that doesn't need to happen here.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/8a363789d659493f11efc0a4621b107b10e3fef3
Bug 1269141: Remove content script CSS when disabling an extension. r=aswan

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a363789d659

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a363789d659