Closed Bug 1381605 Opened 2 years ago Closed 2 years ago

Do not load ExtensionSettingsStore's JSON file synchronously

Categories

(WebExtensions :: General, enhancement, P1)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

A change was made at some point which causes ExtensionSettingsStore's JSON file to load synchronously. This was done to allow a number of methods in ExtensionSettingsStore to be synchronous, but it's something we shouldn't do.

This can be mitigated by introducing an initialize method into ExtensionSettingsStore which will asynchronously load the JSON file, and then calling that method in the onStartup of the APIs that require the store, which will block the initialization of those extensions which need it, but will not block startup in general.
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

I've pushed my changes to the ExtensionSettingsStore to MozReview but I'm not sure what's the best approach for the ExtensionPreferencesManager. One complication with it is that it runs code in Management.on("shutdown") and Management.on("startup") which needs the settings store to be initialized. I am considering just adding an initializeStore() method and then calling that from every method inside ExtensionPreferencesManager that accesses the store, but that seems like a lot of duplication. Also, because those methods would have to await the resolution of the store's initialization promise, they'd each need to be asynchronous as well, which is something I believe we wanted to change.

Do you have any suggestions for a good way to address these issues?
Attachment #8889617 - Flags: feedback?(aswan)
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

what you've got so far looks good.  As for the preferences side, yes, you need to make sure you've called `SettingsStore.initialize()` before you use the settings store, but that's a one-liner, I don't see that as duplication.  And most (all?) of the methods in question are already async.  I'm sorry I don't remember the context of your last remark (about "something we wanted to change")
Attachment #8889617 - Flags: feedback?(aswan) → feedback+
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

https://reviewboard.mozilla.org/r/160638/#review167160

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:128
(Diff revision 2)
>    let expectedItem = await ExtensionSettingsStore.getSetting(STORE_TYPE, name);
>    let item = await ExtensionSettingsStore[action](extension, STORE_TYPE, name);

neither of these should be async any more?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:190
(Diff revision 2)
>     *          the preferences were not changed.
>     */
>    async setSetting(extension, name, value) {
>      let setting = settingsMap.get(name);
> +    await ExtensionSettingsStore.initialize();
>      let item = await ExtensionSettingsStore.addSetting(

I don't think there's a good reason for `addSetting()` to be async any more?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:291
(Diff revision 2)
>     * @param {Extension} extension
>     *        The extension for which all settings are being unset.
>     */
>    async removeAll(extension) {
> +    await ExtensionSettingsStore.initialize();
>      let settings = await ExtensionSettingsStore.getAllForExtension(extension, STORE_TYPE);

why are you awaiting here?  or is that part of a separate bug

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:100
(Diff revision 2)
> +  return initialize();
> +}
> +
> +// Get the internal settings store, which is persisted in a JSON file.
> +function getStore(type) {
> +  if (!_initializePromise || !_store.dataReady) {

shouldn't this just check `_store.dataReady`?  if it is called when the initialization promise exists but hasn't yet finished, its going to blow up below.
also at this point, I think a name like `ensureType()` would be more descriptive since the caller is supposed to call `initialize()` themselves, so this isn't really doing anything to set up the store.
Attachment #8889617 - Flags: review?(aswan) → review+
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

https://reviewboard.mozilla.org/r/160638/#review167160

> I don't think there's a good reason for `addSetting()` to be async any more?

`addSetting()` calls `AddonManager.getAddonByID()` internally and needs to await that, so I think it does still need to be async, unless I'm not understanding (which is entirely possible).

> why are you awaiting here?  or is that part of a separate bug

Nope, good catch.

> shouldn't this just check `_store.dataReady`?  if it is called when the initialization promise exists but hasn't yet finished, its going to blow up below.
> also at this point, I think a name like `ensureType()` would be more descriptive since the caller is supposed to call `initialize()` themselves, so this isn't really doing anything to set up the store.

I have it as `if (!_initializePromise || !_store.dataReady)` because I want it to short-circuit if `!_initializePromise`, otherwise the call to `!_store.dataReady` will generate an error because `_store` is undefined at that point. I guess I could avoid that by initializing `_store` to `{}` instead of making it undefined to begin with. I'll make that change.

I kept the name of the function as `getStore` because it was returing the store, but I guess now that's not necessary and the functions that use the store can just use `_store` directly. That feels a bit less encapsulated to me, but considering that we're not doing any setup in `getStore` (as you pointed out) I guess there's no need for the getter anymore.
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

https://reviewboard.mozilla.org/r/160638/#review167160

> `addSetting()` calls `AddonManager.getAddonByID()` internally and needs to await that, so I think it does still need to be async, unless I'm not understanding (which is entirely possible).

ah, yes.
Comment on attachment 8889617 [details]
Bug 1381605 - Do not load ExtensionSettingsStore's JSON file synchronously,

https://reviewboard.mozilla.org/r/160638/#review167160

> I have it as `if (!_initializePromise || !_store.dataReady)` because I want it to short-circuit if `!_initializePromise`, otherwise the call to `!_store.dataReady` will generate an error because `_store` is undefined at that point. I guess I could avoid that by initializing `_store` to `{}` instead of making it undefined to begin with. I'll make that change.
> 
> I kept the name of the function as `getStore` because it was returing the store, but I guess now that's not necessary and the functions that use the store can just use `_store` directly. That feels a bit less encapsulated to me, but considering that we're not doing any setup in `getStore` (as you pointed out) I guess there's no need for the getter anymore.

wtf reviewboard i had a comment here that got dropped.
anyway, i don't feel strongly about the name, whatever you prefer is fine.
but as for the test, if the important thing is whether `_store` exists, lets just check that directly.  ie `if (!_store || !_store.ready) { ...}`
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ffc57ebe8e9
Do not load ExtensionSettingsStore's JSON file synchronously, r=aswan
https://hg.mozilla.org/mozilla-central/rev/5ffc57ebe8e9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1381579
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.