Cache more computed manifest data in the startup cache

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

2 years ago
Whiteboard: [qf]
Comment hidden (mozreview-request)

Comment 3

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

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

Comment 5

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

Comment 6

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

Comment 7

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

Updated

2 years ago
Keywords: leave-open
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8896649 - Attachment is obsolete: true

Comment 10

2 years ago
mozreview-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/#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+
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 11

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b1ec4367b83
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

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