Closed Bug 1344590 Opened 7 years ago Closed 7 years ago

Cache parsed and normalized JSON data in indexedDB

Categories

(WebExtensions :: General, enhancement)

Product:
WebExtensions
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(4 files)

Bug 1344590: Part 1 - Handle extension reloads, upgrades, and manager restarts in xpcshell helpers.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1344590: Part 2 - Add Promise-based IndexedDB helper module.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1344590: Part 3 - Handle schema default values for object properties.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
We currently spend a lot of time at startup parsing and normalizing JSON data. We only really need to do most of this work once, when an extension is installed or a schema is reloaded. Storing the normalized result as structured clone data in indexedDB should speed up startup considerably.
Comment on attachment 8843784 [details]
Bug 1344590: Part 1 - Handle extension reloads, upgrades, and manager restarts in xpcshell helpers.

https://reviewboard.mozilla.org/r/117356/#review119836

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:85
(Diff revision 1)
> +      this.id = extension.id;
> +      this.attachExtension(extension);
> +    }
> +  }
> +
> +  destroy() {

maybe a comment that there's nothing to do here? or have the caller check for `destroy()` before calling it?

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:144
(Diff revision 1)
> -      this.testScope.ok(pass, msg);
> +        this.testScope.ok(pass, msg);
> -    });
> -    this.extension.on("test-done", (kind, pass, msg, expected, actual) => {
> +        break;
> +
> +      case "test-done":
> -      this.testScope.ok(pass, msg);
> +        this.testScope.ok(pass, msg);
> -      this.testResolve(msg);
> +        this.testResolve(msg);

I know there's nothing to fallthrough to here but either calling this the `default` case or putting in `break` would be better in case anyone adds another condition below this for some reason.
Attachment #8843784 - Flags: review?(rhelmer) → review+
Comment on attachment 8843784 [details]
Bug 1344590: Part 1 - Handle extension reloads, upgrades, and manager restarts in xpcshell helpers.

https://reviewboard.mozilla.org/r/117356/#review119836

> I know there's nothing to fallthrough to here but either calling this the `default` case or putting in `break` would be better in case anyone adds another condition below this for some reason.

Eh, that's what ESLint is for :)
Comment on attachment 8843786 [details]
Bug 1344590: Part 3 - Handle schema default values for object properties.

https://reviewboard.mozilla.org/r/117360/#review120114
Attachment #8843786 - Flags: review?(aswan) → review+
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120120

still reading but a few questions along the way

::: toolkit/components/extensions/Extension.jsm:447
(Diff revision 2)
> +  // Reads the extension's |manifest.json| file, and stores its
> +  // parsed contents in |this.manifest|.
> +  async loadManifest() {
> +    [this.manifest] = await Promise.all([
> +      this.parseManifest(),
> +      Management.lazyInit(),

parseManifest already does this internally and it doesn't look like you get it started any faster here...

::: toolkit/components/extensions/ExtensionManagement.jsm:323
(Diff revision 2)
>  
> +let cacheInvalidated = 0;
> +function onCacheInvalidate() {
> +  cacheInvalidated++;
> +}
> +Services.obs.addObserver(onCacheInvalidate, "startupcache-invalidate", false);

From a brief read of XPIProvider.jsm this seems to get called a lot, but could perhaps be made more targeted (ie, make the subject be the id of an individual addon to be invalidated rather than invalidating everything)

::: toolkit/components/extensions/ExtensionManagement.jsm:326
(Diff revision 2)
> +  get cacheInvalidated() {
> +    return cacheInvalidated;
> +  },

I was momentarily thrown by the name here which makes this sound like a boolean, which it is not.  maybe call it `cacheGeneration` or somethign?

::: toolkit/components/extensions/ExtensionUtils.jsm:105
(Diff revision 2)
> +      if (Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT) {
> +        IndexedDB.deleteDatabase(this.DB_NAME, {storage: "persistent"});
> +      }

I don't get this, how do we get here in a process other than the parent process?

::: toolkit/components/extensions/ExtensionUtils.jsm:132
(Diff revision 2)
> +      this.dbPromise = this.reallyOpen(true).catch(e => {});
> +    }
> +  },
> +};
> +
> +Services.obs.addObserver(StartupCache, "startupcache-invalidate", false);

This looks racy, this observer calls into reallyOpen() which reads ExtensionManagement.cacheInvalidated, but that is also being changed by a startupcache-invalidate observer.
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120120

> parseManifest already does this internally and it doesn't look like you get it started any faster here...

Only the one in `ExtensionData` does. The caching version doesn't, so it only gets called if we fall back to the base version. There's no harm in calling it twice, though.

> From a brief read of XPIProvider.jsm this seems to get called a lot, but could perhaps be made more targeted (ie, make the subject be the id of an individual addon to be invalidated rather than invalidating everything)

That's my eventual plan, but I decided to start out on the conservative side. In practice, though, I think the existing extension-specific invalidations should be enough, so I'd be willing to remove this.

> I don't get this, how do we get here in a process other than the parent process?

We shouldn't unless someone dispatches startupcache-invalidate in a child. I suppose we could just remove that observer, though. We'll probably need some other kind of cache synchronization if we want to start using this interface in the child process at some point.

> This looks racy, this observer calls into reallyOpen() which reads ExtensionManagement.cacheInvalidated, but that is also being changed by a startupcache-invalidate observer.

The cacheInvalidated check is only there to catch cache flushes that happened early, before the bulk of extension code was loaded. After that, it doesn't really matter. But the ExtensionManagement observer will always be registered first, and therefore always run first, so we don't need to worry about extra flushes from the cacheInvalidated property being bumped.
Comment on attachment 8843785 [details]
Bug 1344590: Part 2 - Add Promise-based IndexedDB helper module.

https://reviewboard.mozilla.org/r/117358/#review120112

I'm not familiar enough with indexedDB to evalute this in detail but its obviously useful enough to enable part 4 and style-wise looks good to me.
Attachment #8843785 - Flags: review?(aswan) → review+
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120120

> We shouldn't unless someone dispatches startupcache-invalidate in a child. I suppose we could just remove that observer, though. We'll probably need some other kind of cache synchronization if we want to start using this interface in the child process at some point.

How about throwing if this is called from a child process unless/until we decide we want something different?
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120278

::: toolkit/components/extensions/Extension.jsm:643
(Diff revision 2)
> +    if (["ADDON_UPGRADE", "ADDON_DOWNGRADE"].includes(startupReason)) {
> +      StartupCache.clearAddonData(addonData.id);
> +    }

it can't hurt to be extra careful, but this should have already happened when the previous version was shut down?
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120284

A couple of questions:
1. I'm unfamiliar with the use of indexedDB from chrome -- does this data all just get stored associated with the system principal?  Does it have the same issues we have with e.g. permanent private browsing mode?  I suppose since this is all a big cache, we don't have a correctness issue if indexedDB just doesn't work or gets cleared for some reason but it makes me uneasy
2. I don't see any mechanism to invalidate schema data when there's a browser update (or if we're using experiments)

::: toolkit/components/extensions/ExtensionUtils.jsm:85
(Diff revision 2)
> +      db.createObjectStore(name);
> +    }
> +  },
> +
> +  clearAddonData(id) {
> +    let range = IDBKeyRange.bound([id], [id, "\uFFFF"]);

I don't understand this, why isn't the id just the key passed directly to `delete()`?
(In reply to Andrew Swan [:aswan] from comment #17)
> 1. I'm unfamiliar with the use of indexedDB from chrome -- does this
>    data all just get stored associated with the system principal?

Yes.

> Does it have the same issues we have with e.g. permanent private
> browsing mode?

No, the system principal never has origin attributes.

> 2. I don't see any mechanism to invalidate schema data when there's a
> browser update (or if we're using experiments)

We just need to bump the schema version when we make schema changes.
Experiment schemas are eventually going to be keyed by add-on ID, but
for now we flush the whole cache any time an add-on is updated.

> ::: toolkit/components/extensions/ExtensionUtils.jsm:85
> (Diff revision 2)
> > +      db.createObjectStore(name);
> > +    }
> > +  },
> > +
> > +  clearAddonData(id) {
> > +    let range = IDBKeyRange.bound([id], [id, "\uFFFF"]);
> 
> I don't understand this, why isn't the id just the key passed directly to
> `delete()`?

Cached values are keyed by ID and locale, since values are localized during the normalization step. We need to delete all values for the ID, regardless of locale, which means we need to use a range filter.
(In reply to Kris Maglione [:kmag] from comment #18)
> > 2. I don't see any mechanism to invalidate schema data when there's a
> > browser update (or if we're using experiments)
> 
> We just need to bump the schema version when we make schema changes.
> Experiment schemas are eventually going to be keyed by add-on ID, but
> for now we flush the whole cache any time an add-on is updated.

Manually bumping the schema version here any time any of the built-in schemas changes sound error-prone.  But I don't have any better suggestion to offer...  Can you please add some comments about the plans you mention above for experiments?

> Cached values are keyed by ID and locale, since values are localized during
> the normalization step. We need to delete all values for the ID, regardless
> of locale, which means we need to use a range filter.

Ah yes, duh.
Comment on attachment 8843787 [details]
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB.

https://reviewboard.mozilla.org/r/117362/#review120326
Attachment #8843787 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5b6337431606e12b66236dcd29073543e2b1b9
Bug 1344590: Part 1 - Handle extension reloads, upgrades, and manager restarts in xpcshell helpers. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/811d4a0a0e2d544dfea7e1b3a883c5d2e5f7b1ab
Bug 1344590: Part 2 - Add Promise-based IndexedDB helper module. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e4035449512b458671037b2c9f8cd002352c6f
Bug 1344590: Part 3 - Handle schema default values for object properties. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d580a74eae4ed16945e49372b53add59ee817a8f
Bug 1344590: Part 4 - Store parsed and normalized extension data in indexedDB. r=aswan
Product: Toolkit → WebExtensions
See Also: → 1545591
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: