1389840 - Cache more computed manifest data in the startup cache

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Description

[Kris Maglione [:kmag]]

We still spend a bunch of time at startup computing manifest data that hasn't changed since the last session. We should store more of it in the startup cache.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1389840: Part 1 - Store more computed manifest data in startup cache.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1389840: Part 2 - Store last optional permissions state in the startup cache.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8896649 [details]
Bug 1389840: Part 1 - Store more computed manifest data in startup cache.

https://reviewboard.mozilla.org/r/167928/#review173306

::: toolkit/components/extensions/Extension.jsm:548
(Diff revision 1)
> -      // Errors are handled by the type checks above.
> +        // Errors are handled by the type checks above.
> -    }
> +      }
>  
> -    let whitelist = [];
> -    for (let perm of this.manifest.permissions) {
> +      let apiNames = new Set();
> +      let dependencies = new Set();
> +      let hostPermissions = new Set();

nit: I've tried to use the phrase "origin permissions" for these, can we try to use that consistently?

::: toolkit/components/extensions/Extension.jsm:604
(Diff revision 1)
> +    // Do not override the add-on id that has been already assigned.
> +    if (!this.id) {
> +      this.id = manifestData.id;
> +    }

I'm confused about `this.id` versus the id pulled from the manifest.  Re-reading the add-ons manager code, I can't see a case where `manifestData.id` is set here but `this.id` is not.  If there is such a case, then the code above below the "An extension always gets permission to its own url." comment will be broken...

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8896650 [details]
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache.

https://reviewboard.mozilla.org/r/167930/#review173312

I don't understand exactly what you're aiming for here.  It looks like with this patch we're still writing `extension-preferences.json` (ugh I think that filename was a typo and was supposed to be `-permissions`) but never reading it, and the startup cache has become the only place we actually go for information about what permissions have been granted.  That's not necessarily a problem in principle but its no longer accurate to call it a cache in that case.
I think a helpful step would be to describe clearly how you want this to work.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8896650 [details]
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache.

https://reviewboard.mozilla.org/r/167930/#review173312

We still read it any time optional permissions are updated. We should also read it any time the startup cache is invalidated, but we don't. I should fix that.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8896649 [details]
Bug 1389840: Part 1 - Store more computed manifest data in startup cache.

https://reviewboard.mozilla.org/r/167928/#review173306

> I'm confused about `this.id` versus the id pulled from the manifest.  Re-reading the add-ons manager code, I can't see a case where `manifestData.id` is set here but `this.id` is not.  If there is such a case, then the code above below the "An extension always gets permission to its own url." comment will be broken...

The add-on manager passes the ID from the XPI signature, or an auto-generated ID for temporary add-ons. But only when we're actually running the extension, not when we're using an ExtensionData instance just to parse it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1edb8f6d5bf
Part 1 - Store more computed manifest data in startup cache. r=aswan

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8896650 [details]
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache.

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

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8896650 [details]
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache.

https://reviewboard.mozilla.org/r/167930/#review175190

::: toolkit/components/extensions/ExtensionParent.jsm:1534
(Diff revision 2)
> +  async set(path, value) {
> +    let [store, key] = await this.getStore(path);
> +
> +    store.set(key, value);
> +  }

Shouldn't this also call `StartupCache.save();` ?

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1ec4367b83568963639dc1a27840e4df4166aa
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache. r=aswan

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5b1ec4367b83