1459750 - Clean up leftovers of the content script management logic

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Rob Wu [:robwu]]

I was reading code to get a better understanding of the content script injection logic.
While doing so I encountered some misleading comments and variable names, and dead code.
I'll submit a patch with my findings for review.


BrowserExtensionContent has an unused lazy "scripts" getter.
This originates from the inception of the WebExtensions framework: https://searchfox.org/mozilla-central/diff/b1a00d7c72053d21693f52f4334007dc77ebeb06/toolkit/components/extensions/ExtensionContent.jsm#416
and was used as "extension.scripts" at places such as https://searchfox.org/mozilla-central/diff/b1a00d7c72053d21693f52f4334007dc77ebeb06/toolkit/components/extensions/ExtensionContent.jsm#366


In bug 1317697, StubExtension was introduced to replace BrowserExtensionContent (https://hg.mozilla.org/mozilla-central/rev/1267d47eca93). From here the "extension" is a StubExtension instead of BrowserExtensionContent for content script management in injectExtensionScripts .
But the lazy "scripts" getter was NOT removed.


In bug 1368102, the content script logic was moved to C++. The StubExtension above was replaced with WebExtensionPolicy, in two steps:
1. https://hg.mozilla.org/mozilla-central/rev/32a3b7c39207
  extension.scripts -> extension.policy.contentScripts (where extension is a StubExtension)

2. https://hg.mozilla.org/mozilla-central/rev/350bf7ed1ad3
  extension.policy.scripts -> extension.contentScripts (where extension was a StubExtension, and became a WebExtensionPolicy)

Some variable names and comment still refer to "extension" where "policy" is meant.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1459750 - Remove unused BrowserExtensionContent.scripts

This "scripts" getter started to become unused in:
https://hg.mozilla.org/mozilla-central/rev/1267d47eca93

The "extension" in the "extension.scripts" above was changed
from "BrowserExtensionContent" to "StubExtension".

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

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1459750 - Rename extension to policy where applicable

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

https://reviewboard.mozilla.org/r/242220/#review248056


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


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


::: toolkit/components/extensions/ExtensionChild.jsm:47
(Diff revision 1)
>  
>  const {
>    DefaultMap,
>    EventEmitter,
>    LimitedSet,
>    defineLazyGetter,

Error: 'definelazygetter' is assigned a value but never used. allowed unused vars must match /^console$/. [eslint: no-unused-vars]

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242220/diff/1-2/

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242222/diff/1-2/

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

`contentScripts` is a
`DefaultWeakMap<WebExtensionContentScript, ExtensionContent.Script>`.

Dynamic scripts are used only once and do not need too be cached,
so replace the indirect construction (via the DefaultWeakMap)
with an explicit call to `ExtensionContent.Script`.

And add a JSDoc to the Script, to disambiguate the "extension" type.

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

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1459750 - Remove unused principal variable

Variable `principal` is only used when `this.isExtensionPage` is false.
So move this inside the branch where this condition is satisfied, and
remove the `else if (this.isExtensionPage) { ... }` block.

(This is a follow-up to bug 1214658, where this check was introduced).

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

https://reviewboard.mozilla.org/r/242222/#review248248

There are other places throughout where this has happened.  There is also bug 1441886 for that.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

https://reviewboard.mozilla.org/r/242220/#review248252

This is fine but I want to be sure that Luca sees it as well since he's worked on all the content script stuff.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

https://reviewboard.mozilla.org/r/242244/#review248256

I'm not so certain about this.  If extensions tend towards using executeScript on multiple tabs (which some do) where the script is the same, cacheing the script may be a good thing.  This probably becomes moot once the userScripts feature lands.

Comment 11

[Rob Wu [:robwu]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

https://reviewboard.mozilla.org/r/242244/#review248256

This change won't affect the execution performance of `executeScript`; The ScriptCache in ExtensionContent.jsm is responsible for that.

The contentScripts WeakMap in this process script seems to be used to avoid instantiating `ExtensionContent.Script` (JS) multiple times for a `WebExtensionContentScript` (C++). This makes sense since conceptually there is a 1:1 mapping between the two.

For dynamic scripts, the WeakMap key is generated on the fly and kept in a local scope. So the value in the WeakMap will never be accessed again: https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/toolkit/components/extensions/extension-process-script.js#137
Therefore the current code does not offer any difference in functionality, besides some potential (non-deterministic!) delay in garbage collection.

> once the userScripts feature lands.

That has already landed, and is visible in this file in the "registeredContentScripts" map.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #11)

> > once the userScripts feature lands.
> 
> That has already landed, and is visible in this file in the
> "registeredContentScripts" map.

That's not user scripts.  Bug 1437861 and Bug 1437864.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

https://reviewboard.mozilla.org/r/242244/#review250808

::: toolkit/components/extensions/ExtensionContent.jsm:279
(Diff revision 1)
>  
>  // Represents a content script.
>  class Script {
> +  /**
> +   * @param {BrowserExtensionContent} extension
> +   * @param {object} matcher An object with a "matchesWindow" method and content

Param descriptions start on the next line, aligned with the opening brace of the type.

::: toolkit/components/extensions/extension-process-script.js:146
(Diff revision 1)
>            removeCSS: data.options.remove_css,
>            cssOrigin: data.options.css_origin,
>            jsCode: data.options.jsCode,
>          });
>  
> -        let script = contentScripts.get(matcher);
> +        let script = new ExtensionContent.Script(extensions.get(policy), matcher);

Please leave this as is so we don't wind up duplicating logic.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

https://reviewboard.mozilla.org/r/242244/#review250808

> Please leave this as is so we don't wind up duplicating logic.

The duplication is minimal and quite obvious. `contentScripts.get` is not a factory, but a `DefaultWeakMap`. Before this change, the use of the DefaultWeakMap can be misunderstood as storing the item in a global map for later use. Since the key is a local variable, this is not the case.

The change improves the readability, by clearly showing that the variable is independent of the `contentScript` map and intended to be used temporarily.

With this added explanation, do you still insist on reverting this change?

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

https://reviewboard.mozilla.org/r/242220/#review251454

This part of the cleanup looks good to me.

(As a side note, related to the description in the comment, we should probably mention that StubExtension has been already removed at this point, in Bug 1368102, https://hg.mozilla.org/mozilla-central/rev/350bf7ed1ad3, and the extension scripts are now lazily created by from the DefaultWeakMap defined in extension-process.script.js, https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/toolkit/components/extensions/extension-process-script.js#70-73)

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

https://reviewboard.mozilla.org/r/242222/#review251458

::: toolkit/components/extensions/extension-process-script.js:71
(Diff revision 2)
>    extension.policy = policy;
>    return extension;
>  });
>  
>  var contentScripts = new DefaultWeakMap(matcher => {
>    return new ExtensionContent.Script(extensions.get(matcher.extension),

`matcher.extension` here is also a `WebExtensionPolicy` instance: https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/chrome-webidl/WebExtensionContentScript.webidl#58

Rob, if we are going to land this patch which partially renames some of the places where the policy object is actually named extension in this patch, do you mind to add this issue as a "see also" or a "blocker" for Bug 1441886 and add a comment to mention that `WebExtensionContentScript` (which is the `matcher` here) also has an `WebExtensionPolicy` instance as a property named `extension` which has not been renamed yet?

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Rob Wu [:robwu] from comment #14)
> Comment on attachment 8973880 [details]
> Bug 1459750 - Explicit ExtensionContent.Script call + JSDoc
> 
> https://reviewboard.mozilla.org/r/242244/#review250808
> 
> > Please leave this as is so we don't wind up duplicating logic.
> 
> The duplication is minimal and quite obvious. `contentScripts.get` is not a
> factory, but a `DefaultWeakMap`. Before this change, the use of the
> DefaultWeakMap can be misunderstood as storing the item in a global map for
> later use. Since the key is a local variable, this is not the case.
> 
> The change improves the readability, by clearly showing that the variable is
> independent of the `contentScript` map and intended to be used temporarily.
> 
> With this added explanation, do you still insist on reverting this change?

Besides the improvement in readability ("potential and very small" improvemnt, at least from my point of view), it doesn't seem to provide any significant gain (on the memory usage or performance side), and so it doesn't seem really worth it and I tend to agree with Kris' r- on that part of these changes.

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> Comment on attachment 8973880 [details]
> Bug 1459750 - Explicit ExtensionContent.Script call + JSDoc
> 
> https://reviewboard.mozilla.org/r/242244/#review248256
> 
> I'm not so certain about this.  If extensions tend towards using
> executeScript on multiple tabs (which some do) where the script is the same,
> cacheing the script may be a good thing.  This probably becomes moot once
> the userScripts feature lands.

I confirm that, as Rob briefly described in comment 11, our "CSS stylesheets and JS scripts caching mechanism" shouldn't be affected by this kind of change, because it is actually based on the urls (and not on the ExtensionContent.Script instances), and so the code executed by tabs.executeScript is actually cached when the API call is using extension urls (but when the API call uses a jsCode string it is not currently cached or precompiled, on the contrary we are currently caching css code strings, using their hash as a key for their entry in the CSSCache).

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973962 [details]
Bug 1459750 - Remove unused principal variable

https://reviewboard.mozilla.org/r/242304/#review251480

The change looks fine to me, r=me on green try.

(As a side note, related to the description in the commit message, this seems to be more of a follow up for Bug 1317697, which has actually introduced the `else if` branch that sets the principal when `this.isExtensionPage` is `true`: https://hg.mozilla.org/mozilla-central/rev/1267d47eca93#l6.465)

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242220/diff/2-3/

Comment 20

[Rob Wu [:robwu]]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242222/diff/2-3/

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242244/diff/1-2/

Comment 22

[Rob Wu [:robwu]]

Comment on attachment 8973962 [details]
Bug 1459750 - Remove unused principal variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242304/diff/1-2/

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

https://reviewboard.mozilla.org/r/242244/#review259994

The updated jsdoc looks good to me, but I would prefer if we mention also in the jsdoc's param type that `matcher` is usually a `WebExtensionContentScript` and possibly providing a more precise definition for when the `matcher` type is expected to be `WebExtensionContentScript` and when it is not.

r+wc

::: toolkit/components/extensions/ExtensionContent.jsm:279
(Diff revision 2)
>  
>  // Represents a content script.
>  class Script {
> +  /**
> +   * @param {BrowserExtensionContent} extension
> +   * @param {object} matcher

This jsdoc type could be documented as `{WebExtensionContentScript | object}`, so that it is immediately visible that it is usually an instance of `WebExtensionContentScript`.

::: toolkit/components/extensions/ExtensionContent.jsm:281
(Diff revision 2)
>  class Script {
> +  /**
> +   * @param {BrowserExtensionContent} extension
> +   * @param {object} matcher
> +   *        An object with a "matchesWindow" method and content script execution
> +   *        details. This is often a WebExtensionContentScript object,

"This is often a ..." sounds a bit vague as a definition, I would prefer a more precise definition about the expected type of the matcher.

If I'm not wrong, the matcher is supposed to always be an instance of `WebExtensionContentScript` for all the content scripts registered from the manifest and using the `contentScripts.register` API method, and an object for the content scripts injected using `tabs.executeScript`.

Comment 24

[Rob Wu [:robwu]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242220/diff/3-4/

Comment 25

[Rob Wu [:robwu]]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242222/diff/3-4/

Comment 26

[Rob Wu [:robwu]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242244/diff/2-3/

Comment 27

[Rob Wu [:robwu]]

Comment on attachment 8973962 [details]
Bug 1459750 - Remove unused principal variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242304/diff/2-3/

Comment 28

[Rob Wu [:robwu]]

Comment on attachment 8973848 [details]
Bug 1459750 - Remove unused BrowserExtensionContent.scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242220/diff/4-5/

Comment 29

[Rob Wu [:robwu]]

Comment on attachment 8973849 [details]
Bug 1459750 - Rename extension to policy where applicable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242222/diff/4-5/

Comment 30

[Rob Wu [:robwu]]

Comment on attachment 8973880 [details]
Bug 1459750 - Add JSDoc to ExtensionContent.Script constructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242244/diff/3-4/

Comment 31

[Rob Wu [:robwu]]

Comment on attachment 8973962 [details]
Bug 1459750 - Remove unused principal variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242304/diff/3-4/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a054ee2f2a04
Remove unused BrowserExtensionContent.scripts r=mixedpuppy,rpl
https://hg.mozilla.org/integration/autoland/rev/125bdcf2860a
Rename extension to policy where applicable r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/9aeb585f5f7d
Add JSDoc to ExtensionContent.Script constructor r=rpl
https://hg.mozilla.org/integration/autoland/rev/11a2e09c259b
Remove unused principal variable r=rpl

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a054ee2f2a04
https://hg.mozilla.org/mozilla-central/rev/125bdcf2860a
https://hg.mozilla.org/mozilla-central/rev/9aeb585f5f7d
https://hg.mozilla.org/mozilla-central/rev/11a2e09c259b