1393621 - ext-contextualIdentities.js is loaded at startup when it isn't needed

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Kris Maglione [:kmag]]

Bug 1354602 added a startup event listener to the contextualIdentities API, which causes it to be loaded at startup whether or not it's needed. This also causes ExtensionPreferencesManager to be eagerly loaded when it isn't needed.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Thanks for raising this follow up.

We should be able to use defineLazyModuleGetter with some work however as I understand it we don't have the ability to do what is in the onStartup callback at present do we?

Ideally we could solve this to be an optional preference if there was a onPreferenceEnabled callback available.

Comment 2

[Kris Maglione [:kmag]]

ext-contextualIdentities.js needs to not be loaded at all when no extension with the permission is running. I'm not as concerned about ExtensionPreferencesManager.jsm

Comment 3

[Daniel Holbert [:dholbert]]

Startup perf win --> qf:p1, on the assumption that this is pretty straightforward.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1393621: Part 1 - Don't load ext-contextualIdentities at startup without permissions.

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

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1393621: Part 2 - Add test for API modules loaded at startup.

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

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1393621: Part 3 - Add test for framework JSMs loaded at startup.

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

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8902882 [details]
Bug 1393621: Part 1 - Don't load ext-contextualIdentities at startup without permissions.

https://reviewboard.mozilla.org/r/174582/#review180002

::: toolkit/components/extensions/ExtensionCommon.jsm:1222
(Diff revision 1)
>     *        Whether the module may be loaded.
>     */
>    _checkGetAPI(name, extension, scope = null) {
>      let module = this.modules.get(name);
>  
> +    if (module.permissions && !module.permissions.some(perm => extension.hasPermission(perm))) {

It looks this wouldn't work for optional permissions, since the extension never "has" those  permissions on startup?

Also please add the property description to the `APIModule` jsdocs above.

::: toolkit/components/extensions/ExtensionCommon.jsm:1234
(Diff revision 1)
>  
>      if (!module.scopes.includes(scope)) {
>        return false;
>      }
>  
>      if (!Schemas.checkPermissions(module.namespaceName, extension)) {

Hm, wouldn't this check also return false under the same conditions?  

I understand that for extension `onStartup`, this is called without the `scope` and it never gets here, but I don't see a reason why this check couldn't go before the scope check?

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8902883 [details]
Bug 1393621: Part 2 - Add test for API modules loaded at startup.

https://reviewboard.mozilla.org/r/174584/#review180026

Nice!

::: toolkit/components/extensions/test/xpcshell/test_ext_startup_perf.js:14
(Diff revision 1)
> +// Tests that only the minimal set of API scripts are loaded at startup
> +// for a simple extension.
> +add_task(async function test_loaded_api_scripts() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background() {},
> +    manifest: {},

Does it make sense to try loading this using the addon manager, since some APIs behave differently / need it to be properly tested?

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8902884 [details]
Bug 1393621: Part 3 - Add test for framework JSMs loaded at startup.

https://reviewboard.mozilla.org/r/174586/#review180024

::: toolkit/components/extensions/test/xpcshell/test_ext_startup_perf.js:26
(Diff revision 1)
> +  STARTUP_MODULES.push(
> +    "resource://gre/modules/ExtensionChild.jsm",
> +    "resource://gre/modules/ExtensionPageChild.jsm");
> +}
> +
>  // Tests that only the minimal set of API scripts are loaded at startup

Update the comment and test name please.

::: toolkit/components/extensions/test/xpcshell/test_ext_startup_perf.js:48
(Diff revision 1)
>              "No extra APIs should be loaded at startup for a simple extension");
>  
> +
> +  const loader = Cc["@mozilla.org/moz/jsloader;1"].getService(Ci.xpcIJSModuleLoader);
> +  let loadedModules = loader.loadedModules()
> +                            .filter(url => url.startsWith("resource://gre/modules/Extension"));

This seems flimsy and already misses a few JSMs, though not sure we can get more granular without some hoops.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8902882 [details]
Bug 1393621: Part 1 - Don't load ext-contextualIdentities at startup without permissions.

https://reviewboard.mozilla.org/r/174582/#review180002

> It looks this wouldn't work for optional permissions, since the extension never "has" those  permissions on startup?
> 
> Also please add the property description to the `APIModule` jsdocs above.

It does work for optional permissions. Or, at least, it works as well as the previous code worked. Optional permissions do persist across sessions, but either way, we want to check the current permissions state before loading an API module.

> Hm, wouldn't this check also return false under the same conditions?  
> 
> I understand that for extension `onStartup`, this is called without the `scope` and it never gets here, but I don't see a reason why this check couldn't go before the scope check?

Meh. I originally added the exemption for the scope override so we could load some modules at startup regardless of the namespace permissions. That's probably not necessary anymore, but I don't want to deal with the potential headaches of changing it right now.

Comment 11

[Tomislav Jovanovic :zombie]

Comment on attachment 8902882 [details]
Bug 1393621: Part 1 - Don't load ext-contextualIdentities at startup without permissions.

https://reviewboard.mozilla.org/r/174582/#review180040

Since I wouldn't wish headache upon you, I'm gonna assume you already tried that and it broke stuff..  ;)

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94aaf11dd167ecfe54f6055d95fc56d1770b0092
Bug 1393621: Part 1 - Don't load ext-contextualIdentities at startup without permissions. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/648a5d65c827c12713a6c2ff84ad69d8ed84c41c
Bug 1393621: Part 2 - Add test for API modules loaded at startup. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/fab7bacb59284801d9c47f6ef5d0529f5ae20154
Bug 1393621: Part 3 - Add test for framework JSMs loaded at startup. r=zombie

Comment 13

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/94aaf11dd167
https://hg.mozilla.org/mozilla-central/rev/648a5d65c827
https://hg.mozilla.org/mozilla-central/rev/fab7bacb5928

Comment 14

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!