Closed Bug 1389840 Opened 4 years ago Closed 4 years ago

Cache more computed manifest data in the startup cache

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [qf]
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...
Attachment #8896649 - Flags: review?(aswan) → review+
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.
Attachment #8896650 - Flags: review?(aswan) → review-
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 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.
Whiteboard: [qf] → [qf:p1]
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1edb8f6d5bf
Part 1 - Store more computed manifest data in startup cache. r=aswan
Keywords: leave-open
Attachment #8896649 - Attachment is obsolete: true
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();` ?
Attachment #8896650 - Flags: review?(aswan) → review+
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1ec4367b83568963639dc1a27840e4df4166aa
Bug 1389840: Part 2 - Store last optional permissions state in the startup cache. r=aswan
https://hg.mozilla.org/mozilla-central/rev/5b1ec4367b83
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.