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

RESOLVED FIXED in Firefox 57

Status

defect
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug, {regression})

unspecified
mozilla57
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
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.
(Assignee)

Comment 2

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: regression
Startup perf win --> qf:p1, on the assumption that this is pretty straightforward.
Whiteboard: [qf] → [qf:p1]
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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?
Attachment #8902883 - Flags: review?(tomica) → review+

Comment 9

2 years ago
mozreview-review
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.
Attachment #8902884 - Flags: review?(tomica) → review+
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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..  ;)
Attachment #8902882 - Flags: review?(tomica) → review+
(Assignee)

Comment 12

2 years ago
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
Assignee: nobody → kmaglione+bmo

Comment 14

2 years ago
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!
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Flags: needinfo?(kmaglione+bmo) → qe-verify-

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.