Closed Bug 1406181 Opened 7 years ago Closed 6 years ago

Use indexedDB as the backend for storage.local

Categories

(WebExtensions :: Storage, enhancement, P1)

enhancement

Tracking

(relnote-firefox 66+, firefox62 verified, firefox63 verified)

VERIFIED FIXED
mozilla62
Iteration:
63.3 - Aug 6
Tracking Status
relnote-firefox --- 66+
firefox62 --- verified
firefox63 --- verified

People

(Reporter: andy+bugzilla, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 2 obsolete files)

59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
7.42 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
149.28 KB, image/png
Details
This is a tracking bug for moving storage.local to be backed by indexedDB, instead of serialized to a file in the profile directory.

The main benefits are expected to be:
* lower memory usage bug 1371255
* better at storing small changes to a large structure as shown by uBlock Origin and ABP

There would also need to be a migration of data into this new storage from the serialization file.

See also bug 1277612.
Status: NEW → ASSIGNED
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8920223 - Flags: review?(kmaglione+bmo)
Attachment #8920224 - Flags: review?(kmaglione+bmo)
Attachment #8920225 - Flags: review?(kmaglione+bmo)
Component: WebExtensions: General → WebExtensions: Storage
Blocks: 1282972
# Profiles Firefox "extension child process" enabled + ABP 3.0.2 (manually collected profile)

Follows profiles (manually) collected from:
- 2 Firefox instances in OOP mode (with the extension running in the extensions child process):
  - one Firefox instance is using the current storage.local JSONFile backend
  - the other one using the storage.local IndexedDB backend (which has: the patches attached to this issue 
    and the "extensions.webextensions.useExtensionStorageIDB" about:config preference switched to true).

Both these Firefox instances have AdBlock Plus 3.0.2 installed and the same 11 filters lists subscribed (the default ones + some additional ones subscribed from https://adblockplus.org/subscriptions).

The following are the shared profiles from perf-html.io:

- with the current JSONFile backend: https://perfht.ml/2zh92hJ
- with the experimental IndexedDB backend: https://perfht.ml/2zfSs1D

Here is a bit more detailed description of the scenario executed and collected in the above two profiles:
- profiling data collection started just before pressing the "Update all filter lists" in the Advanced tab 
  of the ABP Options page
- profiling data collection stopped just a bit after that all the subscriptions have been updated 
 (while scrolling the page continuously to get a feeling of the kind of "user perceivable" jank 
 that a user may experience while the storage.local API is sending the data across the processes, 
 serializing/deserializing it and finally storing it on disk)

(the following video shows the scenario as executed in the two firefox instances, unfortunately the jank that is happening during the page scrolling can't be perceived exactly as it does locally): https://youtu.be/C1YeGB2pH7U.

The JSONFile backend profile seems to show (unsurprisingly) that more "event processing delay" ranges are happening in the main process (which was something that was easily perceived, e.g. scrolling the page showed visible pauses) compared to the one collected from the instance that is using the IndexedDB as the storage backend, that has less "event processing delay" ranges in the main process 
(especially because, in the last version of the experimental IndexedDB backend, most of the storage.local.set code is running in the child process instead of the main process, and the IndexedDB calls that require some code to run in the main process, to be able to write the data on disk, are going to run in a different thread of the main process, and so the only part of the storage.local.set call that still uses "heavily" the main process is "notify the onChanged message to the subscribed listeners").

# mochitest-browser collected profiles

Besides the above two profiles, collected by executing the scenario manually on both the Firefox instances, I've also used a custom mochitest-browser test to collect more profiling data (so that I can more quickly collect profiling data from more combination of the different settings, e.g. with and without the extension child process and with and without e10s).

mochitest-browser profiling tested scenario:

- start 2 test extensions, wait both to start their background page and content scripts, start profiler
- each one sets two keys with a 20MB of data value
  (the second extension after ~500ms)
  from a content script (running in the content child process),
- wait for the onchanged message in the background page (running in the extension child process)
- wait 1 sec (which gives Firefox some idle time to save the data into the JSONFile
- stop profiling and dump it to a json file

(unfortunately these profiles are currently missing the C++ symbols, I'll take a deeper look at how to successfully symbolicate these profiles collected without the geckoprofiler addon, on the plus side they also have data collected from
the IndexedDB threads).

## "extension child process" enabled / "content child process" enabled

Profiles collected with the remote webextensions mode enabled (with the background page running in the extension child process and the content script running in the content process) 

- with JSONFile backend: https://perfht.ml/2zmNQXS
- with IDB backend: https://perfht.ml/2zm5iLX

## "extension child process" disabled / "content child process" enabled

Profiles collected with the remote webextensions mode disabled (with the background page running in the main process and the content script running in the content
process)

- with JSONFile backend: https://perfht.ml/2zl11Ze
- with IDB backend: https://perfht.ml/2zlXNEP

## "extension child process" disabled / "content child process" disabled

Profiles collected with e10s disable
(mostly because it gives a profile which should be similar to the ones we could collect from on Firefox for Android, which is currently in non-e10s mode)

- with JSONFile backend: https://perfht.ml/2zllFIz
- with IDB backend: https://perfht.ml/2zlYYnJ
This patch contains the mochitest-browser test from which the last groups of profiles linked in comment 24 have been collected.
# mochitest-browser collected profiles (with C++ symbols)

Follows the profiles collected from the mochitest-browser test, this time with the C++ symbols (using mstange's analyze-tryserver-profiles python scripts with the following changes applied https://github.com/mstange/analyze-tryserver-profiles/compare/master...rpl:symbolicate_profile_with_nm_symbols?expand=1)

## "extension child process" enabled / "content child process" enabled

Profiles collected with the remote webextensions mode enabled (with the background page running in the extension child process and the content script running in the content process) 

- with JSONFile backend: https://perfht.ml/2A5WHgZ
- with IDB backend: https://perfht.ml/2A7j3yt

## "extension child process" disabled / "content child process" enabled

Profiles collected with the remote webextensions mode disabled (with the background page running in the main process and the content script running in the content
process)

- with JSONFile backend: https://perfht.ml/2A4TQF0
- with IDB backend: https://perfht.ml/2A8PNri

## "extension child process" disabled / "content child process" disabled

Profiles collected with e10s disable
(mostly because it gives a profile which should be similar to the ones we could collect from on Firefox for Android, which is currently in non-e10s mode)

- with JSONFile backend: https://perfht.ml/2A6xbYP
- with IDB backend: https://perfht.ml/2A6xZNn
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review219436

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:34
(Diff revision 2)
> +this.ExtensionStorageIDB = {
> +  // Map<extension-id, Set<Function>>
> +  listeners: new Map(),
> +
> +  _open(extensionId) {
> +    return IndexedDB.open(`${IDB_NAME_PREFIX}-${extensionId}`, IDB_OPTIONS, db => {

This database needs to have an extension origin.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:44
(Diff revision 2)
> +  _loadAllStoredKeys(db) {
> +    return db.objectStore(IDB_DATA_STORENAME, "readonly").getAllKeys();
> +  },
> +
> +  async _loadData(db, keysParam) {
> +    const storedKeys = await this._loadAllStoredKeys(db);

There's no need to do this when we have a list of keys to retrieve. If you really need to know the difference between a key with an undefined value and a nonexistent key, you can use a cursor. Or we can just forbid `undefined` as a storage value and normalize it to `null`.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:54
(Diff revision 2)
> +
> +    const objectStore = db.objectStore(IDB_DATA_STORENAME, "readonly");
> +
> +    for (let key of keys) {
> +      if (storedKeys.includes(key)) {
> +        loadedPromises.push(objectStore.get(key).then(value => {

It would be much more efficient to call `Promise.all()` on the get requests and then iterate over the results, rather than adding a .then() clause to each request.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:79
(Diff revision 2)
> +   *        said object. Any values which are StructuredCloneHolder
> +   *        instances are deserialized before being stored.
> +   * @returns {Promise<void>}
> +   */
> +  async set(extensionId, items) {
> +    const db = await this._open(extensionId);

Opening a fresh database connection every time we want to read or change a key is a bad idea.

Also, passing around DB objects like this is weird. If you want to build something on top of a DB, please create a class that owns the DB instance for an extension.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:83
(Diff revision 2)
> +  async set(extensionId, items) {
> +    const db = await this._open(extensionId);
> +    const data = await this._loadData(db, Object.keys(items));
> +
> +    const changes = {};
> +    const savedPromises = [];

Nit: `savePromises`

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:87
(Diff revision 2)
> +    for (let key in items) {
> +      let value = items[key];
> +      changes[key] = {oldValue: data[key], newValue: value};
> +      savedPromises.push(store.put(value, key));
> +    }
> +
> +    await Promise.all(savedPromises);

This all needs to happen in a single transaction, or we'll wind up with interleaved transactions and listeners that contain stale values.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:87
(Diff revision 2)
> +    const changes = {};
> +    const savedPromises = [];
> +
> +    const store = db.objectStore(IDB_DATA_STORENAME, "readwrite");
> +
> +    for (let key in items) {

`for (let key of Object.keys(items))`

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:138
(Diff revision 2)
> +
> +    // If keysParam is an object, use the passed value for every non existent key.
> +    if (hasDefaults) {
> +      for (let key of keys) {
> +        if (!(key in data)) {
> +          data[key] = keysParam[key];

It would be better to just fill in the defaults object as we get values and then return it.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:157
(Diff revision 2)
> +    if (!keys || keys.length === 0) {
> +      return null;
> +    }

Why?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:173
(Diff revision 2)
> +    for (let key of keys) {
> +      if (key in data) {
> +        changes[key] = {oldValue: data[key]};
> +        promisesChanged.push(store.delete(key));
> +        changed = true;
> +      }
> +    }
> +
> +    if (changed) {
> +      await promisesChanged;

Again, this all needs to happen in a transaction.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:198
(Diff revision 2)
> +    const db = await this._open(extensionId);
> +    const data = await this._loadData(db);
> +
> +    let changed = false;
> +    let changes = {};
> +
> +    for (let key in data) {
> +      changes[key] = {oldValue: data[key]};
> +      changed = true;
> +    }
> +
> +    if (changed) {
> +      const store = db.objectStore(IDB_DATA_STORENAME, "readwrite");
> +
> +      await store.clear();

Single transaction.
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Comment on attachment 8920223 [details]
Bug 1406181 - Move serialize/deserialize helpers from ext-c-storage.js to ExtensionStorage.jsm.

https://reviewboard.mozilla.org/r/191232/#review219442
Attachment #8920223 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8920224 [details]
Bug 1406181 - Use indexedDB as the backend for storage.local.

https://reviewboard.mozilla.org/r/191234/#review219444

::: toolkit/components/extensions/Extension.jsm:1500
(Diff revision 3)
> +      if (this.useExtensionStorageIDB) {
> +        await this.migrateStorageLocalData();

We don't need to do this at every startup, only when the storage backend changes.

::: toolkit/components/extensions/Extension.jsm:1537
(Diff revision 3)
> +    let oldStorageFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +    oldStorageFile.initWithPath(ExtensionStorage.getStorageFile(this.id));
> +
> +    if (!oldStorageFile.exists()) {

This needs to use the OS.File API.

::: toolkit/components/extensions/Extension.jsm:1545
(Diff revision 3)
> +    if (!oldStorageFile.exists()) {
> +      return;
> +    }
> +
> +    const isEmpty = await ExtensionStorageIDB.isEmpty(this.id);
> +

Nit: Remove line.

::: toolkit/components/extensions/Extension.jsm:1552
(Diff revision 3)
> +      // Cancel the data migration is the indexedDB backend for the extension is not empty.
> +      return;
> +    }
> +
> +    let jsonFile;
> +

Remove line.

::: toolkit/components/extensions/Extension.jsm:1568
(Diff revision 3)
> +    for (let [key, value] of jsonFile.data.entries()) {
> +      try {
> +        await ExtensionStorageIDB.set(this.id, {[key]: value});
> +      } catch (err) {
> +        completedWithErrors = true;
> +
> +        // Report the migration error and continue to migrate the remaining data.
> +        Cu.reportError(
> +          new Error(`Extension error during storage.local data migration for ${this.id} ` +
> +                    `on key '${key}': ${err.message} :: ${err.stack}`)
> +        );
> +      }
> +    }

We should do this in a single request, and make sure we don't try to notify listeners.

::: toolkit/components/extensions/Extension.jsm:1578
(Diff revision 3)
> +
> +        // Report the migration error and continue to migrate the remaining data.
> +        Cu.reportError(
> +          new Error(`Extension error during storage.local data migration for ${this.id} ` +
> +                    `on key '${key}': ${err.message} :: ${err.stack}`)
> +        );

Nit: Move to previous line. Also, no need for `new Error`.

::: toolkit/components/extensions/Extension.jsm:1582
(Diff revision 3)
> +                    `on key '${key}': ${err.message} :: ${err.stack}`)
> +        );
> +      }
> +    }
> +
> +    oldStorageFile.renameTo(null, `${oldStorageFile.leafName}.migrated`);

We should probably just remove this if the migration was successful.

Also, this needs to use the OS.File API.

::: toolkit/components/extensions/Extension.jsm:1737
(Diff revision 3)
> +  get useExtensionStorageIDB() {
> +    if (this._useExtensionStorageIDB == null) {
> +      this._useExtensionStorageIDB = Services.prefs.getBoolPref(EXT_STORAGE_IDB_PREF,
> +                                                                EXT_STORAGE_IDB_PREF_DEFAULT);
> +    }
> +
> +    return this._useExtensionStorageIDB;
> +  }

Please just use `defineLazyPrefGetter`

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:66
(Diff revision 3)
> +    const db = await this._open(extensionId);
> +    const storedKeys = await this._loadAllStoredKeys(db);
> +    await db.close();
> +    return storedKeys.length === 0;

This is seriously inefficient.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:211
(Diff revision 3)
>      let changes = {};
>      let promisesChanged = [];
>  
>      for (let key of keys) {
> -      if (key in data) {
> +      if (skipNotify) {
> +        promisesChanged.push(store.delete(key));

This doesn't need to be in both branches.

::: toolkit/components/extensions/ext-c-storage.js:68
(Diff revision 3)
> -        },
> +      },
> +      set: async function(items) {
> +        const stopwatchKey = {};
> +        TelemetryStopwatch.start(storageSetHistogram, stopwatchKey);
> +        try {
> +          const hasListeners = await hasParentListeners();

This is going to be super expensive and racy.

::: toolkit/components/extensions/ext-c-storage.js:73
(Diff revision 3)
> +            result = await context.childManager.callParentAsyncFunction("storage.local.set", [
> +              serialize(items),
> +            ]);

This isn't really a good idea. We should always perform the IDB transactions in a child process. If we need to handle cross-process listeners, we should either find some way to use transactions to handle ordering them correctly, or have non-extension processes proxy their changes and listeners through the extension process.

::: toolkit/components/extensions/ext-c-storage.js:78
(Diff revision 3)
> +              skipNotify: true,
> +            });

`{skipNotify: true})`
Attachment #8920224 - Flags: review?(kmaglione+bmo)
Comment on attachment 8920225 [details]
Bug 1406181 - Test storage.local JSONFile and IndexedDB backends.

https://reviewboard.mozilla.org/r/191236/#review219448
Attachment #8920225 - Flags: review?(kmaglione+bmo) → review+
Does this supersede bug 1277612? Why is it better to have a standalone IndexedDB backend vs. using the kinto.js library as an API that wraps IndexedDB or Sqlite?
(In reply to Ethan Glasser-Camp (:glasserc) from comment #31)
> Does this supersede bug 1277612? Why is it better to have a standalone
> IndexedDB backend vs. using the kinto.js library as an API that wraps
> IndexedDB or Sqlite?

That's a good question. In general, I'd prefer to use as much of the same implementation as possible for the local and sync backends. The only real issue I see is that we'd really like the DB interaction to happen as much in the child process as possible, and I'm not sure the kinto backend allows for that.

Also, addendum to the above comments:

We should really either be storing values as StructuredCloneBlobs in the DB, or find some way to serialize and deserialize them in the extension compartment. Otherwise we're going to wind up reintroducing a bunch of unnecessary clones every time data is read or changed.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #32)
> We should really either be storing values as StructuredCloneBlobs in the DB,
> or find some way to serialize and deserialize them in the extension
> compartment. Otherwise we're going to wind up reintroducing a bunch of
> unnecessary clones every time data is read or changed.

Maybe IndexedDB wants to grow a little bit more chrome-supporting logic.  I see two aspects that could help:

1) Re: comment 27 and using the extension's origin, there's some prior art for the dom/cache API which has a notion of (a fixed, enumerated set of) namespaces, with the ServiceWorker internals using that namespace to store the ServiceWorker scripts under their own origin, but where content can't directly access them.  If IndexedDB grew similar explicit functionality, that might be useful here.

   It's of course possible to hack something up using OriginAttributes and attribute patterns.  IndexedDB might even want to go that same route to implement namespaces.  (Noting that when I raised the prospect of adding an attribute for differing levels of session persistence, :baku indicated a new attribute would be inappropriate, but the container userContextId mechanism might be an acceptable way to partition things without exploding the number of dimensions... we can allocate specific id's for specific purposes... possibly negative ones.)


2) StructuredCloneBlob's ability to deal with other globals looks super useful.  If IDB added some ChromeOnly methods so that add/put could take a global, and that the result values could be extracted into a specific global, that sounds like it would facilitate the use-case without getting into the downsides of making StructuredCloneBlob something that can be persisted, duplicating persistence-related efforts, or losing the benefits of IndexedDB's disk-backed Blobs and their deduplication-via-under-the-hood-references magic.

  IndexedDB already limits persistence to a subset of all the things that can notionally be structured cloned.  An effort has been made to try and keep things stable across versions, although this hasn't stopped things from accidentally sneaking in that complicate things (ex: Bug 1431241, also the initial landing of StructuredCloneBlob).  There's now a spec for persistence at https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeforstorage and a general WebIDL extensibility mechanism ([Serializable] and [Transferable]) that is tracked for DOM as Bug 1350254.  While those enhancements will likely end up StructuredCloneHolder anyways, it would still be less scary if only IndexedDB had to worry about persistence and upgrades.



As another aside, re: the mention of listeners, I believe we are strongly considering https://github.com/WICG/indexed-db-observers although I don't see us down as committed at https://www.chromestatus.com/feature/5669292892749824.  Certainly, since Chrome already has a prototype implementation hidden behind a flag, if you're hoping for cross-browser consistent semantics, building on that rather than something more ad-hoc might be good.
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review219436

> This all needs to happen in a single transaction, or we'll wind up with interleaved transactions and listeners that contain stale values.

Based on the implementation of the `objectStore method` (and its jsdoc comment https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/modules/IndexedDB.jsm#307-308), `db.objectStore(...)` return value should be the transaction, am I reading it wrong?

That is also the reason why store.put has to be called synchronously for every key and then await on `Promise.all(savePromises)`, otherwise (e.g. if we await inside the loop on every single promise returned by `store.put(...)`), only the first `store.put(...)` would succeed, and the rest would fail because the transaction has been already closed).

is there anything that I'm missing about the behavior implemented by the IndexedDB.jsm wrapper to ensure that everything is going to happen in the same transaction?
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review219436

> Based on the implementation of the `objectStore method` (and its jsdoc comment https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/modules/IndexedDB.jsm#307-308), `db.objectStore(...)` return value should be the transaction, am I reading it wrong?
> 
> That is also the reason why store.put has to be called synchronously for every key and then await on `Promise.all(savePromises)`, otherwise (e.g. if we await inside the loop on every single promise returned by `store.put(...)`), only the first `store.put(...)` would succeed, and the rest would fail because the transaction has been already closed).
> 
> is there anything that I'm missing about the behavior implemented by the IndexedDB.jsm wrapper to ensure that everything is going to happen in the same transaction?

Nevermind I think that I found the answer myself: the part that read the existent data from the db wasn't part of the code highlighted by this comment and so it wasn't immediately clear from the comment itself, nevetheless the data is being read and written in two different transactions, and they should be part of a single transaction.
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review219436

> This database needs to have an extension origin.

I agree, I avoided it in the first prototype on the IDB backend because I was unsure about which strategy we could use to make the database invisible from the extension code.

Nevertheless the strategy that ashut mentioned in his comment (using an extension principal isolated in a "reserved" userContextId) is exactly the one that I was thinking of, and ashut's comment motivated me to giving it a try asap, and so the new version of the backend is now going to open the database using the extension principal.

> There's no need to do this when we have a list of keys to retrieve. If you really need to know the difference between a key with an undefined value and a nonexistent key, you can use a cursor. Or we can just forbid `undefined` as a storage value and normalize it to `null`.

One reason was that when browser.storage.local.get is called with null as parameter, the API is supposed to retrieve all the stored data and return it.

I totally agree that we can avoid to retrieve all the keys at once, and so I've rewritten it to use a cursor when keysParam is null.

> Opening a fresh database connection every time we want to read or change a key is a bad idea.
> 
> Also, passing around DB objects like this is weird. If you want to build something on top of a DB, please create a class that owns the DB instance for an extension.

I definitely agree, the first prototype wasn't yet reusing the db connection, the new version is now using a class that abstract the db connection and provides methods to read/write data from/to it, this db connection is then reused for multiple API requests (cached in a WeakMap with the extension as a key, and currently also removed from the WeakMap on idle).

> Why?

It was to early exit if there are no keys to remove.
Comment on attachment 8920224 [details]
Bug 1406181 - Use indexedDB as the backend for storage.local.

https://reviewboard.mozilla.org/r/191234/#review219444

> We don't need to do this at every startup, only when the storage backend changes.

That is true, it was of the point I wanted to discuss, I'm going to take a look again to find a reasonable strategy to detect when the backend is changing for the first time (but also, let me know if you have any ideas or preferred strategy that we could use to achieve it).

> We should probably just remove this if the migration was successful.
> 
> Also, this needs to use the OS.File API.

That was one of the options I was thinking of, but given that the amount of data that can be currently stored in not that match (the JSONFile backend breaks when the stored data is becoming too big) I opted to just rename the file (which can be still used to recover same data if something goes wrong).
Attachment #8920224 - Attachment is obsolete: true
Attachment #8938663 - Attachment is obsolete: true
Attachment #8938663 - Flags: review?(kmaglione+bmo)
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review224772

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:26
(Diff revision 3)
> +// TODO(rpl): we should move this const into a file that is shared with the
> +// contextual identities internals so that we are sure that the user context id
> +// is reserved to this kind of usage and not reused for other purpose
> +// (and that is not allowed to be used on a opened tab).
> +const RESERVED_USER_CONTEXT_ID = -10;

This is a negative userContextId that I'm temporarily using to experiment the approach suggested in ashut comment to ensure that the indexedDB storage used as a browser.storage.local backend is not visible from the extension code.

The approach is working as expected, but we need to choose how to make sure that the "containers feature internals" are aware that these "user context ids" are reserved and should not be allowed elsewhere (e.g. it should not be allowed to use them for opened tabs, they should not be allowed in the WebExtensions APIs and the reserved user context ids should not be reused for different purposes) 

(I also need to add a new test case which verify that in an automated test, but in the meantime I verified that is working as expected using a small test extension)
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review224778

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:208
(Diff revision 3)
> +  async set(items, {serialize} = {}) {
> +    const global = this;
> +
> +    const changes = await this.runIDBTransactionTask(function* (objectStore, transaction) {
> +      const result = {};
> +
> +      for (let key of Object.keys(items)) {

It looks that we can't use async functions and idb requests wrapped in promises to read and write data in a single IndexedDB transaction, because the transaction is closed as soon as the async function is resumed once the first "promise-wrapped IDB request" is resolved.

I've currently opted to use generator functions to express the group of IDB operations as a sequence of IDB requests.

The runIDBTransactionTask helper method process the IDB request yielded by the generator and resume the generator as soon as the IDB request has been completed.

(Given that after I gradually rewritten the get/set/remove/clear methods to this approach the new version is now using the Promise-based API provided by the IndexedDB.jsm base class only in one place, I'm probably going to just remove it and turn ExtensionStorageLocalIDB into a class that doesn't extend any base class).

::: toolkit/components/extensions/ext-c-storage.js:76
(Diff revision 3)
> +          const hasListeners = await hasParentListeners();
> +          if (hasListeners) {
> +            await context.childManager.callParentAsyncFunction("storage.fireStorageLocalOnChanged", [
> +              changes,
> +            ]);
> +          }

In this new version the data is always read and updated in the child process, but the changes are still sent to the main process which route them to the other child processes.

If we could reuse an IndexedDB observer as suggested in ashut comment instead of re-invent a similar mechanisms just for this API it would be great.

In the meantime I've been thinking if there could be a short term strategy that we can put in place to reduce the amount of data that these event has to send in a messageManager event.

How about using another IndexedDB db in "temporary" storage mode, add the actual "changes" data to it and send the generated (autoIncrement) key across the processes, so that the listeners in the other processes can retrieve the changes from the "temporary" db?

::: toolkit/components/extensions/ext-storage.js:60
(Diff revision 3)
> +  getLocalIDBBackend(context, {deserialize, hasListeners}) {
> +    return {
> +      set: function(items) {
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:
> +        /*
> +        AsyncShutdown.profileBeforeChange.addBlocker(
> +          `Extension Storage IDB: Wait for extension ${extension.id} to save data`,
> +          setDataPromise);
> +
> +        return setDataPromise;*/
> +      },
> +      remove: function(keys) {
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:
> +      },
> +      clear: function() {
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:
> -        },
> +      },
> +    };

These set/remove/clear methods are currently all unused, because these are all implemented in the ext-c-storage.js.

I've not removed them yet only because I want to double-check if we still need to add AsyncShutdown blocker in the main process,  
as the JSONFile backend is doing to ensure that we wait the data to be stored when the browser is shutting down (e.g. I was wondering if this is something that we still have to do for the indexedDB transactions, which are now always happening in the child processes, or if it is something that the indexedDB is already doing internally).
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review224900

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:180
(Diff revision 3)
> +  getUsage() {
> +    return new Promise(resolve => {
> +      Services.qms.getUsageForPrincipal(
> +        this.params.storagePrincipal,
> +        request => resolve(request.result.usage));
> +    });
> +  }

This method is not currently used anywhere, I added it because:

- I was checking the usage of the `getUsageForPrincipal` to report the amount of data stored (e.g. because we could use it to provide the getBytesInUse API method, https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/StorageArea/getBytesInUse)

- I was briefly verifying if the db opened using the extension principal is limited by the quota (which seems that it is not currently enforced, but I've not yet investigated the underlying reasons).
Keywords: meta
Unfortunately, due to recent staffing changes, I think IndexedDB observers are off the table for now.
(In reply to Andrew Sutherland [:asuth] from comment #48)
> Unfortunately, due to recent staffing changes, I think IndexedDB observers
> are off the table for now.

Hi Andrew,
Thanks for pointing it out, I'm going to take it into account.

Thank you very much also for all the suggestions you provided in comment 33, they have been very helpful!
(and I've now started to discuss with :baku about where it would be better to add the reserved userContextId).
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

Hi Andrea,
During the past week I took a look at the ContextualIdentityService.jsm part of  "reserving a private userContextId for the WebExtensions storage.local IndexedDB backend" that we briefly discussed over IRC and I'd like to get a preliminary feedback on it.

The user context id is still hardcoded into the ExtensionStorageIDB.jsm module (which is mostly used in the child process where we can't load arbitrary file from the profile dir), but while I was looking into the migration of the containers json file I noticed that (besides giving a name to the private reserved userContextId) we should also ensure that the extension data is not cleared when ContextualIdentityService.jsm clears all the other identities.

I also added some additional assertions to the test_corrupted.js unit test (so that we check that the reserved identity is still available if the containers.json file is corrupted, and that the extension storage data is not cleared on the reserved identity, as well as a new test case for the migration of an old json file to the last version (and that the expected changes are applied, e.g. the new reserved identity should be available as well as any custom user-created identity that was part of the old containers.json file content).
Attachment #8954173 - Flags: feedback?(amarchesini)
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

https://reviewboard.mozilla.org/r/223334/#review229524

I would like to see a test for migration3to4

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:76
(Diff revision 1)
>        icon: "fingerprint",
>        color: "blue",
>        l10nID: "userContextPersonal.label",
>        accessKey: "userContextPersonal.accesskey",
>        telemetryId: 1,
> +      keepData: false,

this name is confusing, plus, it is used only in notifyAllContainersCleared().

Probably we should just ignore private containers in notifyAllContainersCleared() and delete data only for the public ones.
Attachment #8954173 - Flags: review+
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

https://reviewboard.mozilla.org/r/223334/#review229524

Definitely, it was missing from the patch because I forgot to "hg add" it :-)

I've just added it and pushed an update on this patch: https://reviewboard.mozilla.org/r/191228/diff/5-6/

> this name is confusing, plus, it is used only in notifyAllContainersCleared().
> 
> Probably we should just ignore private containers in notifyAllContainersCleared() and delete data only for the public ones.

Yeah, using the existing "public" attribute sounds great to me, I'm going to update the patch accordingly.
Attachment #8920222 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8920225 [details]
Bug 1406181 - Test storage.local JSONFile and IndexedDB backends.

https://reviewboard.mozilla.org/r/191236/#review229562


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:86
(Diff revision 6)
>    await extension.unload();
>  });
>  
>  add_task(async function test_single_initialization() {
>    // Grab access to this via the backstage pass to check if we're calling openConnection too often.
> -  const {FirefoxAdapter} = ChromeUtils.import("resource://gre/modules/ExtensionStorageSync.jsm", {});
> +  const {FirefoxAdapter} = Cu.import("resource://gre/modules/ExtensionStorageSync.jsm", {});

Error: Please use chromeutils.import instead of cu.import [eslint: mozilla/use-chromeutils-import]
Priority: -- → P1
See Also: → 1371255
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8920222 - Flags: feedback?(kmaglione+bmo)
Attachment #8920222 - Flags: feedback?
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review224778

> It looks that we can't use async functions and idb requests wrapped in promises to read and write data in a single IndexedDB transaction, because the transaction is closed as soon as the async function is resumed once the first "promise-wrapped IDB request" is resolved.
> 
> I've currently opted to use generator functions to express the group of IDB operations as a sequence of IDB requests.
> 
> The runIDBTransactionTask helper method process the IDB request yielded by the generator and resume the generator as soon as the IDB request has been completed.
> 
> (Given that after I gradually rewritten the get/set/remove/clear methods to this approach the new version is now using the Promise-based API provided by the IndexedDB.jsm base class only in one place, I'm probably going to just remove it and turn ExtensionStorageLocalIDB into a class that doesn't extend any base class).

Hey Kris,
I was re-looking if I could finally kill this `runIDBTransactionTask` hack and just using an async function to run multiple read/write requests in a single IndexedDB transaction now that Bug 1193394 has been fixed, and based on this Bug 1193394 Comment 164 it seems that I can definitely kill this hack now \o/

I'm going to give it a try asap (I have to rebase these patches on top of a more recent m-c first), if you didn't reached this part of the code yet in your review, keep it into account that this hack is going away.
Comment on attachment 8920225 [details]
Bug 1406181 - Test storage.local JSONFile and IndexedDB backends.

https://reviewboard.mozilla.org/r/191236/#review237040


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:86
(Diff revision 7)
>    await extension.unload();
>  });
>  
>  add_task(async function test_single_initialization() {
>    // Grab access to this via the backstage pass to check if we're calling openConnection too often.
> -  const {FirefoxAdapter} = ChromeUtils.import("resource://gre/modules/ExtensionStorageSync.jsm", {});
> +  const {FirefoxAdapter} = Cu.import("resource://gre/modules/ExtensionStorageSync.jsm", {});

Error: Please use chromeutils.import instead of cu.import [eslint: mozilla/use-chromeutils-import]
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review224778

> Hey Kris,
> I was re-looking if I could finally kill this `runIDBTransactionTask` hack and just using an async function to run multiple read/write requests in a single IndexedDB transaction now that Bug 1193394 has been fixed, and based on this Bug 1193394 Comment 164 it seems that I can definitely kill this hack now \o/
> 
> I'm going to give it a try asap (I have to rebase these patches on top of a more recent m-c first), if you didn't reached this part of the code yet in your review, keep it into account that this hack is going away.

Last week I have rebased all the patches on a recent mozilla-central tip, and then removed the runIDBTransationTask helper and changed the generators into async functions.

Besides that I've resolved the conflicts related to the tests converted from mochitest into xpcshell tests recently.
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review241058

::: toolkit/components/extensions/Extension.jsm:1213
(Diff revision 5)
>      this._optionalOrigins = null;
>      this.webAccessibleResources = null;
>  
>      this.emitter = new EventEmitter();
>  
> +    XPCOMUtils.defineLazyPreferenceGetter(this, "useExtensionStorageIDB", EXT_STORAGE_IDB_PREF);

Having this as a lazy getter on an extension instance makes no sense at all. The storage backend an instance uses should never change.

::: toolkit/components/extensions/Extension.jsm:1698
(Diff revision 5)
> +    const oldStoragePath = ExtensionStorage.getStorageFile(this.id);
> +    const oldStorageExists = await OS.File.exists(oldStoragePath);
> +
> +    if (!oldStorageExists) {
> +      return;
> +    }

Doing this check on every startup is unacceptable performance overhead.

::: toolkit/components/extensions/Extension.jsm:1749
(Diff revision 5)
> +    }
> +
> +    // If the new ExtensionStorageIDB is not empty anymore, then rename the
> +    // old file to prevent a new data migration to be tried on the next
> +    // extension startup.
> +    OS.File.move(oldStoragePath, `${oldStoragePath}.migrated`);

I'm pretty sure I asked you not to leave this file around if the migratino was successful...

::: toolkit/components/extensions/ExtensionChild.jsm:596
(Diff revision 5)
> +    // Preserve the same storage.local backend choosen by the extension
> +    // in the main process.

This comment is not useful.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:30
(Diff revision 5)
> +    const principal = Services.scriptSecurityManager.createCodebasePrincipal(
> +      extension.baseURI, {userContextId: RESERVED_USER_CONTEXT_ID});

Please don't duplicate this code.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:35
(Diff revision 5)
> +    request.onupgradeneeded = event => {
> +      let db = new this(request.result);
> +      db.createObjectStore(IDB_DATA_STORENAME);
> +    };
> +
> +    return new Promise((resolve, reject) => {
> +      request.onsuccess = () => resolve(new ExtensionStorageLocalIDB(request.result, {
> +        extension,
> +        storagePrincipal: principal,
> +      }));
> +      request.onerror = () => reject(request.error);
> +    });

Please don't duplicate this code from IndexedDB.js

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:64
(Diff revision 5)
> +    super();
> +    this.db = db;
> +    this.params = params;
> +  }
> +
> +  async loadData(keysParam, defaultValues) {

This should be called something like `get`. And since a `get` method exists, and is the only caller, this should just be rolled into that.

Also, please don't put 'Param' in the name of a parameter. We can see it's a param.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:75
(Diff revision 5)
> +        request.onerror = reject;
> +        request.onsuccess = (evt) => {
> +          const cursor = evt.target.result;
> +          if (cursor) {
> +            result[cursor.key] = cursor.value;
> +            cursor.continue();
> +          } else {
> +            // No more items to get.
> +            resolve(result);
> +          }
> +        };

The promise wrappers in the indexedDB classes exist for a reason. Please use them rather than using requests directly.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:92
(Diff revision 5)
> +    }
> +
> +    const objectStore = this.objectStore(IDB_DATA_STORENAME);
> +    for (let key of keysParam) {
> +      const storedValue = await objectStore.get(key);
> +      if (typeof(storedValue) === "undefined") {

No need for `typeof`. `if (storedValue === undefined)`

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:96
(Diff revision 5)
> +
> +        continue;
> +      }
> +
> +      result[key] = storedValue;

Please use an `else` rather than a continue.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:109
(Diff revision 5)
> +  }
> +
> +  async isEmpty() {
> +    const {cursor} = await this.objectStore(IDB_DATA_STORENAME, "readonly").openKeyCursor();
> +
> +    // If the cursor is undefined, then the DB is empty.

No, the cursor should never be undefined.

If it were, though, this should be something like `return !cursor;`

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:168
(Diff revision 5)
> +        newValue = serialize ? serialize(items[key]) : items[key];
> +
> +        // Using the value deserialized from the StructuredCloneHolder will make
> +        // the IndexedDB put operation faster (because it will not spent that much time
> +        // on the Xrays wrappers).
> +        const valueToStore = newValue && newValue.deserialize ?
> +                newValue.deserialize(global) : newValue;

This still gives us tons of extra structured clone overhead, even if it improves the X-ray overhead. I'd rather we extend the indexedDB APIs to handle cloning directly from/to the content compartment, like Andrew suggested.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:309
(Diff revision 5)
> +    // Clear the db connection when idle.
> +    ChromeUtils.idleDispatch(async () => {
> +      this.dbConnections.delete(extension);
> +    }, {timeout: IDB_IDLE_TIMEOUT});

No. Why would we do this?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:317
(Diff revision 5)
> +  async isEmpty(extension) {
> +    const db = await this._open(extension);
> +    const isEmpty = await db.isEmpty();
> +    await db.close();
> +
> +    return isEmpty;
> +  },

Please just have callers obtain an instance when they initialize the API and use that rather than wrapping every method to open a copy of the DB and then call a method on it.

::: toolkit/components/extensions/ext-storage.js:63
(Diff revision 5)
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:

No.

::: toolkit/components/extensions/ext-storage.js:65
(Diff revision 5)
> +        /*
> +        AsyncShutdown.profileBeforeChange.addBlocker(
> +          `Extension Storage IDB: Wait for extension ${extension.id} to save data`,
> +          setDataPromise);
> +
> +        return setDataPromise;*/
> +      },
> +      remove: function(keys) {
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:
> +      },
> +      clear: function() {
> +        // TODO: do we need ensure that we have a shutdown blocker active while the
> +        // indexeddb transaction are potentially still running:
> -        },
> +      },

Why does any of this need to exist?
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review241058

> Having this as a lazy getter on an extension instance makes no sense at all. The storage backend an instance uses should never change.

In the updated patch I completely reworked how the active storage.local backend for an extension is being detected (and the data migrated if needed), so that it happens when the storage.local API is used for the first time, and so this part is totally gone.

> Doing this check on every startup is unacceptable performance overhead.

I definitely agreed on that, and that was the reason why I never cleared the "comment with issue" https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c37 (or https://reviewboard.mozilla.org/r/191234/#review219444 on mozreview), because I wanted to discuss about it with you and evaluate other options.

Anyway, I briefly chatted about this with aswan last week, and then reworked the patch based on that discussion (basically deferring the data migration and any check related to it to the first usage of the storage.local API for a given extension).

> I'm pretty sure I asked you not to leave this file around if the migratino was successful...

In the updated patch the old migrated file is going to be removed (but if the file fails to be removed after the data has been migrated, it still uses the IDB backend).

As for the comment above: in https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c37 I was actually trying to discuss it in a bit more detail with you right after the previous round of review comments.

"Removing the migration file when the migration has been successful" sounds obviously reasonable to me too, but what I wanted was to double-check which were the other scenarios around the data migration and which storage.local backend to use as a result (e.g. keep using the JSONfile backend or still proceed with using the IndexedDB backend), and be sure that I wasn't missing some additional scenario that you could have in mind.

Anyway, even now that the migrated file is removed, I still feel that the data migration could use an additional brief discussion related to some of the possible scenarios (e.g. currently if an extension fails to migrate its existing data to the IDB backend, it logs a warning message in the Browser Console and then it does keep to use of the JSONFile backend, which could be nice during the testing phase, but to be sure that all the extensions are going to use the new backend we should avoid that at some point).

> This comment is not useful.

This part has been completely removed as part of the last round of changes.

> Please don't duplicate this code.

Duplicating this code wasn't actually the intention, this is part of the changes applied after your last review, and it did contain the draft code needed to open the indexedDB with the reserved userContextId (as it has been discussed https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c33, https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c33 and some of the other bugzilla comments above), which was meant to become part of the wrapper defined by IndexedDB.jsm after double-checking with you that we are ok with this strategy (I mean the "reserved"/"non-public" userContextId).

Anyway, I've added a new patch in this mozreview request, which contains a proposed set of changes to cover this directly from the IndexedDB.jsm wrapper.

> Please don't duplicate this code from IndexedDB.js

Same as above.

> This should be called something like `get`. And since a `get` method exists, and is the only caller, this should just be rolled into that.
> 
> Also, please don't put 'Param' in the name of a parameter. We can see it's a param.

Ouch, I should have rolled this back into the `get` method in the previous refactoring (which reworked the other method so that they don't need to load all the data anymore) and I forgot, I did it in the updated patch.

About the 'Param" in the name, it was there because this method was actually inside the `get` method and eslint was complaining of the "shadowing" the variable already declared in the scope (which was the actual `keys` array, while `keysParams` could have been both an array of keys of the items object).

Besides rolling `loadData` back inside the `get` method, I've also renamed `keysParam` into `keysOrItems` in the updated `get` method (so that it makes eslint happy, but it still make it clear that it may not be the array of the keys, and hopefully this new name does also sound better to you too ;-)).

> The promise wrappers in the indexedDB classes exist for a reason. Please use them rather than using requests directly.

Well, initially this was using directly the IndexedDB cursor request because of the issues with the Promises and IndexedDB transactions before Bug 1193394 (as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c68), and then I forgot to look into making the IndexedDB.jsm openCursor/openKeyCursor wrappers able to work correctly with a cursor that is being updated (e.g. using cursor.continue() as above), because the current version of the IndexedDB wrapper is wrapping the openCursor into a single promise, which can be resolved only once, on the contrary the `request.onsuccess` handler is going to be called again with the updated cursor when `cursor.continue()` (or the other cursor methods that update the cursor) are being called.

The new patch in this mozreview request contains a set of proposed changes to IndexedDB.jsm so that we can use the wrapped cursor instance here.

> No need for `typeof`. `if (storedValue === undefined)`

Done.

> Please use an `else` rather than a continue.

Done.

> No, the cursor should never be undefined.
> 
> If it were, though, this should be something like `return !cursor;`

The instance of the Cursor class (which wraps the real cursor) is always going to be created, it is the wrapped cursor that would be undefined if the database is empty:

https://searchfox.org/mozilla-central/source/toolkit/modules/IndexedDB.jsm#155-159

Anyway, I've changed this based on the proposed changes to the Cursor class defined by IndexedDB.jsm.

> This still gives us tons of extra structured clone overhead, even if it improves the X-ray overhead. I'd rather we extend the indexedDB APIs to handle cloning directly from/to the content compartment, like Andrew suggested.

I don't disagree, but I've not changed it yet (I have to look into the indexedDB internals in a bit more details to get a picture of the amount/kind of changes that this would need, and so I opted to work on the other pieces first).   
 
Should we block this on the implementation of that additional IndexedDB chromeOnly behaviors?
or it is something that we could improve in a follow up? 
(e.g. if it would not require another data migration once we do have such feature available).

> No. Why would we do this?

That was actually something that I wanted to ask to be sure, but that definitely answer it.

Removed.

> Please just have callers obtain an instance when they initialize the API and use that rather than wrapping every method to open a copy of the DB and then call a method on it.

Done.

> No.

Ok, removed inlined TODO comment and the non-needed methods in the parent process ext-storage.js.

> Why does any of this need to exist?

Same question as above, the above answer was already enough ;-)

Removed.
Attachment #8968193 - Flags: review?(kmaglione+bmo)
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8969284 - Flags: review?(kmaglione+bmo)
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8920226 - Flags: review?(aswan)
Attachment #8920227 - Flags: review?(aswan)
Attachment #8920228 - Flags: review?(aswan)
Attachment #8920229 - Flags: review?(aswan)
Comment on attachment 8920226 [details]
Bug 1406181 - Test storage.local telemetry on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191238/#review244772

This looks good, another possibility would be to add a helper to the extension xpchsell head.js that handles setting a pref (or prefs), running a test, then clearing the pref(s).  That would avoid the last cleanup task.  Looks like normandy has something similar for mochitest:
https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/toolkit/components/normandy/test/browser/head.js#219-230
Attachment #8920226 - Flags: review?(aswan) → review+
Comment on attachment 8920227 [details]
Bug 1406181 - Test storage.local cleanup on uninstall on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191240/#review244778

This is testing an extension directly using localStorage and indexedDB twice, why not separate that part and only do the browser.storage tests with the two backends?
Comment on attachment 8920228 [details]
Bug 1406181 - Test storage.local from content scripts on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191242/#review244782

Looks good, though the main test logic is here is repeated a few times, it would be a good candidate to move into a single function.
Also see the comment on the telemtry test patch.
Attachment #8920228 - Flags: review?(aswan) → review+
Comment on attachment 8920229 [details]
Bug 1406181 - Test storage.local from extension tab pages on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191244/#review244786

See the comment from the telemetry test patch
Attachment #8920229 - Flags: review?(aswan) → review+
See Also: → 1277612
Comment on attachment 8920227 [details]
Bug 1406181 - Test storage.local cleanup on uninstall on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191240/#review244778

yeah, I agree, I extracted the part the is different (basically the writeData and readData functions and the assertions) from the "test plan" function, this way we reuse the shared part without repeating the test for IndexedDB and localStorage data twice (this is the interdiff restricted to this change: https://reviewboard.mozilla.org/r/191240/diff/13-14/).

Thanks for your review comments!
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8969284 - Flags: review?(aswan)
Attachment #8920227 - Flags: review?(aswan)
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

This patch has been already r+ed by baku (https://bugzilla.mozilla.org/show_bug.cgi?id=1406181#c59), but bugzilla lost the r+ when I reordered the patches, and so I'm setting the r+ on bugzilla manually.
Attachment #8954173 - Flags: review+
(In reply to Andrew Swan [:aswan] from comment #123)
> Comment on attachment 8920226 [details]
> Bug 1406181 - Test storage.local telemetry on file and indexedDB backends.
> 
> https://reviewboard.mozilla.org/r/191238/#review244772
> 
> This looks good, another possibility would be to add a helper to the
> extension xpchsell head.js that handles setting a pref (or prefs), running a
> test, then clearing the pref(s).  That would avoid the last cleanup task. 
> Looks like normandy has something similar for mochitest:
> https://searchfox.org/mozilla-central/rev/
> 8f06c1b9a080b84435a2906e420fe102e1ed780b/toolkit/components/normandy/test/
> browser/head.js#219-230

yep, sounds like a good idea to me.

I've applied this change in all the xpcshell tests (and added a new `runWithPrefs` helper in https://reviewboard.mozilla.org/r/191236/diff/12-13/ and applied it to the test_ext_storage.js xpcshell test, then in the other patches I've used the new helper to the other xpcshell tests).
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:13
(Diff revision 3)
> +// This test file verify the following scenarios related to the data migration
> +// from the JSONFile backend to the IDB backend:
> +//
> +// - all the old data have been migrated successfully
> +// - the old data file was corrupted
> +// - some of the old data file entries fails to be stored in the IDB backend

nit: can you put descriptions of the individual test cases at each add_task() call instead?  (that would give room for a more detailed description)

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:33
(Diff revision 3)
> +}
>  
>  add_task(async function setup() {
> +  Services.prefs.setBoolPref(ExtensionStorageIDB.BACKEND_ENABLED_PREF, true);
> +
>    await ExtensionTestUtils.startAddonManager();

None of the test extensions have useAddonManager, why do you need to start the addon manager?

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:96
(Diff revision 3)
> -    FirefoxAdapter.openConnection = origOpenConnection;
> -  }
> -});
>  
> -add_task(async function test_config_flag_needed() {
> -  function background() {
> +  // Create the file under the expected directory tree.
> +  const {oldStorageFilename} = await createExtensionJSONFileWithData(EXTENSION_ID, {

why actually write a valid file here just to overwrite it below?

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:106
(Diff revision 3)
> -  }
> +  ExtensionStorage.jsonFilePromises.clear();
>  
> -  Preferences.set(STORAGE_SYNC_PREF, false);
> -  ok(!Preferences.get(STORAGE_SYNC_PREF));
> -  let extension = ExtensionTestUtils.loadExtension({
> -    manifest: {
> +  // Overwrite the json file with some invalid data.
> +  await OS.File.writeAtomic(oldStorageFilename, invalidData, {flush: true});
> +  equal(await OS.File.read(oldStorageFilename, {encoding: "utf-8"}),
> +        invalidData, "The old json file has been overwritted with invalid data");

nit: overwritten

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:157
(Diff revision 3)
> -          },
> -        });
>  
> -        await browser.test.assertRejects(
> -          storage.set({
> -            window,
> +  // Store a fake invalid value which is going to fail to be saved into IndexedDB
> +  // (because it can't be cloned and it is going to raise a DataCloneError).
> +  jsonFile.data.set("fake_invalid_key", new Error());

I don't understand what real world scenario this is meant to test.  Is this a proxy for some sort of IDB failure?
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

> nit: can you put descriptions of the individual test cases at each add_task() call instead?  (that would give room for a more detailed description)

Sure, that's a good idea.

> None of the test extensions have useAddonManager, why do you need to start the addon manager?

No need actually, not anymore at least (this was original part of another test and then I splitted it in one of the last updates).

Good catch, removed.

> why actually write a valid file here just to overwrite it below?

Just because it ensures that the actual directory tree is being created (without duplicating the same sequence of code here), the json data file is expected to be in a path like "<<profile_dir>>/browser-extension-data/<<extension_id>>/storage.js".

And so I opted to let the existent code to deal with creating the expected directory tree, and then write some valid data in it so that, if for any reason the test is not overwriting the file as expected, the test is going to fail because unexpected data is beind found in the migrated backend.

> I don't understand what real world scenario this is meant to test.  Is this a proxy for some sort of IDB failure?

yeah, I can't really think of anything that would be able to be stored successfully into the JSONFile backend and then not being able to be saved into the IDB backend during the migration.

But... still, I wanted to check explicitly that the behavior and final outcome would still be the one we expect/want (e.g. no partially migrated data stored in the new backend, the new backend initialized and enabled, and the original file still on disk, in case the user wants to inspect it and/or share it to investigate the issue).
Comment on attachment 8920227 [details]
Bug 1406181 - Test storage.local cleanup on uninstall on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191240/#review250484

r=me with comment addressed

::: toolkit/components/extensions/test/mochitest/test_ext_storage_cleanup.html:26
(Diff revision 15)
> +  // that the "keep uuid" logic works correctly.  Do the storage flag in
> +  // a separate prefEnv so we can pop it below, leaving the uuid flag set.

I'm having a hard time following this since you push twice but pop once and the callers who call pop also call push themeslves.  I think this actually works correctly right now, but it is difficult to reason through.

How about just pushing the uuid setting once at the beginning of the test file, and have test_uninstall()  push and pop the storage setting, but leave everything in the same state when it is finished.
Attachment #8920227 - Flags: review?(aswan) → review+
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

> Just because it ensures that the actual directory tree is being created (without duplicating the same sequence of code here), the json data file is expected to be in a path like "<<profile_dir>>/browser-extension-data/<<extension_id>>/storage.js".
> 
> And so I opted to let the existent code to deal with creating the expected directory tree, and then write some valid data in it so that, if for any reason the test is not overwriting the file as expected, the test is going to fail because unexpected data is beind found in the migrated backend.

You can also do that with `OS.File.makeDir()` and avoid doing other unnecessary work...

> yeah, I can't really think of anything that would be able to be stored successfully into the JSONFile backend and then not being able to be saved into the IDB backend during the migration.
> 
> But... still, I wanted to check explicitly that the behavior and final outcome would still be the one we expect/want (e.g. no partially migrated data stored in the new backend, the new backend initialized and enabled, and the original file still on disk, in case the user wants to inspect it and/or share it to investigate the issue).

Well anything we can store in JSON should also be storable in IDB.  The more likely scenario here seems like some sort of transient issue with IDB (eg some internal sqlite error, a disk I/O error, something like that...)  In one of those cases, are you sure that's the desired outcome?
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review241058

> I don't disagree, but I've not changed it yet (I have to look into the indexedDB internals in a bit more details to get a picture of the amount/kind of changes that this would need, and so I opted to work on the other pieces first).   
>  
> Should we block this on the implementation of that additional IndexedDB chromeOnly behaviors?
> or it is something that we could improve in a follow up? 
> (e.g. if it would not require another data migration once we do have such feature available).

I forgot to mention that I've removed the extra structured clone in one of the previous push to mozreview.
Comment on attachment 8920227 [details]
Bug 1406181 - Test storage.local cleanup on uninstall on file and indexedDB backends.

https://reviewboard.mozilla.org/r/191240/#review250484

> I'm having a hard time following this since you push twice but pop once and the callers who call pop also call push themeslves.  I think this actually works correctly right now, but it is difficult to reason through.
> 
> How about just pushing the uuid setting once at the beginning of the test file, and have test_uninstall()  push and pop the storage setting, but leave everything in the same state when it is finished.

yeah, I agree, it was already pretty tricky to read in its initial version, but the refactoring made it even harder to reason about it.

I also guess that the way the tests (I mean the add_tasks) and the test helpers were sorted in the file was also making it harder, I've just pushed an update that move the "keeps UUID" pref in its own prefEnv (the only was not balanced and active for the entire file), and every other pushEnv / popEnv always balanced in the same function.

These changes should make the "pref environment push and pop" logic much easier to reason.
Attachment #8968193 - Flags: review?(aswan)
Attachment #8954173 - Flags: review+
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8969284 - Flags: review?(aswan)
Attachment #8920222 - Flags: review?(aswan)
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

> You can also do that with `OS.File.makeDir()` and avoid doing other unnecessary work...

That's fair ;-) (updated version which uses `OS.File.makeDir` is coming).

> Well anything we can store in JSON should also be storable in IDB.  The more likely scenario here seems like some sort of transient issue with IDB (eg some internal sqlite error, a disk I/O error, something like that...)  In one of those cases, are you sure that's the desired outcome?

Well, evaluate the desidered (and ensure that it would be also "the actual") outcome is the main reason why I tried to add an explicit test case for that lines of code (like I said in my previous comment I also agree that everything that we can store in the json file should also be stored fine by IDB)

My initial feeling is that a "partial migrated storage data" is not going to make the user happier (or to work better) than "nothing has been migrated, but the old file is still on your disk", and it would probably generate just more confusing (and harder to investigate) bug reports.

What do you think?
Yeah, leaving partial migrated data isn't good.  But if the problem is about indexedDB generally (as opposed to a problem with the data), then it doesn't seem like we should switch over to it...
I'm having a hard time thinking about this without specific failure scenarios to consider.
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review250826

::: toolkit/modules/IndexedDB.jsm:274
(Diff revision 1)
> +  static openForPrincipal(principal, dbName, options, onupgradeneeded = null) {
> +    const request = indexedDB.openForPrincipal(principal, dbName, options);
> +    return this._wrapOpenRequest(request, onupgradeneeded, principal);
> +  }
>  
> +  static _wrapOpenRequest(request, onupgradeneeded = null, storagePrincipal = null) {

`storagePrincipal` isn't used here...
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review250842

I'm not all that familiar with IndexedDB, but the `awaitRequest()` api seems a little strange to me.  Is it practical to just make some of the wrapped methods (e.g. `continue()`) automatically do the promise wrapping so that a user of this module can just `await cursor.continue()` instead of `cursor.continue(); await cursor.awaitRequest();`?

::: toolkit/modules/IndexedDB.jsm:168
(Diff revision 1)
> -      return new CursorWithValue(cursor, this);
> +    return cursor.awaitRequest();
> -    });
>    }
>  
>    openKeyCursor(...args) {
> -    return wrapRequest(this.cursed.openKeyCursor(...args)).then(cursor => {
> +    const cursor = new Cursor(this.cursed.openCursor(...args), this);

looks like this should be openKeyCursor()
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review250826

> `storagePrincipal` isn't used here...

Thanks! I forgot to remove that parameter when I moved this method from ExtensionStorageIDB.jsm into the superclass defined in IndexedDB.jsm.
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review250842

I agree, I can't recall right now (it has been a bunch of time ago), but I'm pretty sure that it was also my original intention, but then I forgot to change `continue` and friends (after I verified that `cursor.awaitRequest` on its own was working like we wanted, and expected).

> looks like this should be openKeyCursor()

Thanks! Good catch! I wrote openCursor instead of openKeyCursor by mistake when I was rewriting both.
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review251462

::: toolkit/modules/IndexedDB.jsm:133
(Diff revision 2)
> +    await wrapRequest(this.cursorRequest).then(cursor => {
> +      this.cursor = cursor;
> +    });

Any reason not to just write this as
```
this.cursor = await wrapRequest(...);
return this;
```

::: toolkit/modules/IndexedDB.jsm:140
(Diff revision 2)
> +  async advance(...args) {
> +    const promise = this.awaitRequest();
> +    this.cursor.advance(...args);
> +    await promise;
> +  }

Since you use the same pattern 3 times, consider making a generic wrapper?
Also, you don't need an async function, something like this should work:
```
function whatever(...args) {
  const promise = this.awaitRequest();
  this.curor.whatever(...args);
  return promise;
}
```

::: toolkit/modules/IndexedDB.jsm:284
(Diff revision 2)
>    static open(dbName, options, onupgradeneeded = null) {
>      let request = indexedDB.open(dbName, options);
> +    return this._wrapOpenRequest(request, onupgradeneeded);
> +  }
> +
> +  static openForPrincipal(principal, dbName, options, onupgradeneeded = null) {

doc comment here?
Attachment #8968193 - Flags: review?(aswan) → review+
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review251468

::: toolkit/components/extensions/Extension.jsm:214
(Diff revision 11)
>      if (!uuid) {
>        return;
>      }
>  
>      if (!Services.prefs.getBoolPref(LEAVE_STORAGE_PREF, false)) {
> -      // Clear browser.local.storage
> +      // Clear browser.local.storage backends.

nit: browser.storage.local

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:23
(Diff revision 11)
> +// The reserved userContextID (related to the private "userContextIdInternal.webextStorageLocal"
> +// identity, unfortunately we can't get the identity using the ContextualIdentityService.jsm
> +// getPrivateIdentity method from the child process), See Bug 1406181 for a rationale.
> +const RESERVED_USER_CONTEXT_ID = -10;

This could use a better explanation, the bug you cite has over 170 comments and it isn't easy to find the relevant discussion about contextId)

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:38
(Diff revision 11)
> +    const storagePrincipal = Services.scriptSecurityManager.createCodebasePrincipal(
> +      extension.baseURI, {userContextId: RESERVED_USER_CONTEXT_ID});

How about extending `Exension.createPrincipal()` to take a contextId (or arbitrary origin attributes) and calling it from here?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:59
(Diff revision 11)
> +   * Asynchronously sets the values of the given storage items for the
> +   * given extension.

I found this comment confusing at first, since there isn't actually a "given extension", it is implicit for this instance of ExtensionStorageIDB.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:74
(Diff revision 11)
> +   * @returns {Promise<void|object>}
> +   *        Return a promise which resolves to the computed "changes" object
> +   *        when "returnChages" is true, otherwise it just resolves to undefined.

what is returnChanges?  Also, this appears to resolve to either null or an object (not void)

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:82
(Diff revision 11)
> +    const transaction = this.transaction(IDB_DATA_STORENAME, "readwrite");
> +    const objectStore = transaction.objectStore(IDB_DATA_STORENAME, "readwrite");

this can be done with `this.objectStore()` right?
I actually prefer this since its clearer while reading the code rather than having the transaction created inside a method for which the name does not suggest that it creates a transaction, but ideally `get()` and `set()` here would have the same basic structure...

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:97
(Diff revision 11)
> +        // exception is going to be raised to the caller, and it will be already
> +        // serialized to be sent across the processing in a message manager message.

I don't understand the last part of this comment

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:114
(Diff revision 11)
> +        transaction.abort();
> +        throw err;
> +      }
> +    }
> +
> +    return changed ? changes : null;

this is my ignorance of indexedDB, is it not necessary to explicitly commit the transaction?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:137
(Diff revision 11)
> +    if (Array.isArray(keysOrItems)) {
> +      keys = keysOrItems;
> +    } else if (keysOrItems && typeof(keysOrItems) === "object") {
> +      keys = Object.keys(keysOrItems);
> +      defaultValues = keysOrItems;
> +    }

The handling of different types of arguments to the extension-visible storage methods should be handled in ext-storage.js, otherwise we have to duplicate this code and risk having subtle differences between the different backends.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:318
(Diff revision 11)
> +  },
> +
> +  // Check if the IDB backend has been selected (and cache the result while the extension
> +  // is still running, an extension child context asks the main process once per child
> +  // process) and wait the data migration to be completed if needed.
> +  promiseIsSelectedBackend(context) {

This name is a little misleading, the fact that this can trigger a migration as a side effect is a surprise.  How about something like `promiseSelecteBackend()` ?  (and nit: I think putting promise in the name is probably unnecessary if instead there is a jsdoc comment documenting that it returns a Promise)

::: toolkit/components/extensions/child/ext-storage.js:16
(Diff revision 11)
>  const storageSetHistogram = "WEBEXT_STORAGE_LOCAL_SET_MS";
>  
>  this.storage = class extends ExtensionAPI {
> +  getLocalFileBackend(context, {deserialize, serialize}) {
> +    return {
> +      get: async function(keys) {

nit just use `async get(keys) { ...` ?

::: toolkit/components/extensions/child/ext-storage.js:18
(Diff revision 11)
>  this.storage = class extends ExtensionAPI {
> +  getLocalFileBackend(context, {deserialize, serialize}) {
> +    return {
> +      get: async function(keys) {
> +        const stopwatchKey = {};
> +        TelemetryStopwatch.start(storageGetHistogram, stopwatchKey);

Is it intentional that the telemetry for both json and idb backends gets put into the same histograms?  That seems like it would make the data very hard to work with.  Has anybody even looked at the data we already have?

Finally, about telemetry, there is a pattern that is repeated here over and over that should be abstracted away.  Something like:

```
async function measureOperation(histogram, fn) {
  const key = {};
  TelemetryStopwatch.start(histogram, key);
  try {
    let result = await fn();
    TelemetryStopwatch.finish(histogram, key);
    return result;
  } catch (e) {
    TelemetryStopwatch.cancel(histogram, key);
    throw e;
  }
  
}

...

get(keys) {
  return measureOperation(storageGetHistogram, () => {
    return context.childManager.callParentAsyncFunction(...);
  }
}
...
```

::: toolkit/components/extensions/child/ext-storage.js:55
(Diff revision 11)
> +      },
> +    };
> +  }
> +
> +  getLocalIDBBackend(context, {hasParentListeners, serialize}) {
> +    const dbPromise = ExtensionStorageIDB.open(context.extension);

Do we care about leaving this open when an extension is not actively using storage?
Comment on attachment 8968193 [details]
Bug 1406181 - Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm.

https://reviewboard.mozilla.org/r/236876/#review251462

> Any reason not to just write this as
> ```
> this.cursor = await wrapRequest(...);
> return this;
> ```

No reasons actually, and it looks better, I've applied this change (and it will be in the next round of updates pushed to mozreview)

> Since you use the same pattern 3 times, consider making a generic wrapper?
> Also, you don't need an async function, something like this should work:
> ```
> function whatever(...args) {
>   const promise = this.awaitRequest();
>   this.curor.whatever(...args);
>   return promise;
> }
> ```

Sure, making it a generic wrapper makes sense.

About the different (non async version) you suggest, there are actually 2 subtle differences between the two version:

- the one that return the `awaitRequest`'s promise will resolve to what the awaitRequest resolve to, on the contrary the async one that await on the promise and doesn't return nothing is going to always resolve to undefined (which in my opinion was more "consistent" to the original cursor.continue/advance/etc. with return undefined: https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor/advance)  

- the one written as async function always return a promise (which will be resolved or rejected), the one written as a regular function may also throw an error synchrously (e.g. in this code probably the only code that may throw is `this.cursor.whatever(...)` if cursor is null/undefined, which may happen if you call continue or advance once the cursor
can't be moved further, which to be fair is a scenario that we may want to raise or reject a better error for)

> doc comment here?

yeah, I should have added it when I moved it here.
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review251468

> This could use a better explanation, the bug you cite has over 170 comments and it isn't easy to find the relevant discussion about contextId)

yeah, I can't disagree, most of them are actually updates on the mozreview patches, nevertheless it is not that easy to get a rationale from there.

I've reworked the comment a bit to make it (hopefully) a bit more clear, I'm not clearing this mozreview issue yet, let me know if the new comment is clear enough.

> How about extending `Exension.createPrincipal()` to take a contextId (or arbitrary origin attributes) and calling it from here?

uhm... I'm not against moving it in a more "shared" place, unfortunately we need to get this principal on the child process side most of the times (the only time we actually running any of this code in the main process is during the data migration).

I think that we have some options to move this elsewhere, here is some that I'm currently thinking of:

- we create the principal we will use for the storage only one in the main process and then we just copy it into the other processes when they are spawned (like we do for the extension principal, which is part of the result of `Extension.prototype.serialize`: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/toolkit/components/extensions/Extension.jsm#1491)

- we define `createPrincipal` method also on `BrowserExtensionContent` (which represents the extension in the child processes, https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/toolkit/components/extensions/ExtensionChild.jsm#578), and maybe a new `storagePrincipal` getter on both `Extension` and `BrowserExtensionContent` classes

> I found this comment confusing at first, since there isn't actually a "given extension", it is implicit for this instance of ExtensionStorageIDB.

yeah, the comment was right in a previous version (where the extension was actually one of the given parameters), but it was outdated now.

Fixed.

> what is returnChanges?  Also, this appears to resolve to either null or an object (not void)

Same as above. Fixed.

> this can be done with `this.objectStore()` right?
> I actually prefer this since its clearer while reading the code rather than having the transaction created inside a method for which the name does not suggest that it creates a transaction, but ideally `get()` and `set()` here would have the same basic structure...

yes, the transaction is usually implicitly created by this.objectStore(), in the set method we explicitly get the transaction because we can abort the entire transaction as soon as the first operation fails.

I've added an inline comment near that line (`const transaction = ...`) to explicitly explain why we get the transaction there, I'm not sure if it is worth to do it for the get too, based on the current implementation the `transaction` object will be basically unused in the `get` method.

> I don't understand the last part of this comment

Actually that code is not needed anymore, I recall that it was needed in a previous version (very very previous version) of this method to preserve the behavior expected by a browser.storage.local.set API call as it was in the previous version (caught by failures in the existing automated tests), but I verified that it doesn't seem to be needed anymore.

I opted to remove the comment as well as the related code.

> this is my ignorance of indexedDB, is it not necessary to explicitly commit the transaction?

The transaction should be committed (or better, becomes "not active") as soon as no more operations are being added while it is still active (e.g. before Bug 1193394 the IDB operation couldn't be wrapped inside promises, because the promise was resolved too late and the transaction was already closed).

As an additional confirmation IDBTransaction doesn't have any other method besides `abort` and `objectStore` (https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction or https://searchfox.org/mozilla-central/source/dom/webidl/IDBTransaction.webidl)

> The handling of different types of arguments to the extension-visible storage methods should be handled in ext-storage.js, otherwise we have to duplicate this code and risk having subtle differences between the different backends.

I don't totally disagree (even if having multiple storage.local backends should not be a common thing to happen, besides now that we are migrating between two that are too different to live in the same JSM).

Let me think about it.

> This name is a little misleading, the fact that this can trigger a migration as a side effect is a surprise.  How about something like `promiseSelecteBackend()` ?  (and nit: I think putting promise in the name is probably unnecessary if instead there is a jsdoc comment documenting that it returns a Promise)

Approved ;-)
Changed applied.

> Is it intentional that the telemetry for both json and idb backends gets put into the same histograms?  That seems like it would make the data very hard to work with.  Has anybody even looked at the data we already have?
> 
> Finally, about telemetry, there is a pattern that is repeated here over and over that should be abstracted away.  Something like:
> 
> ```
> async function measureOperation(histogram, fn) {
>   const key = {};
>   TelemetryStopwatch.start(histogram, key);
>   try {
>     let result = await fn();
>     TelemetryStopwatch.finish(histogram, key);
>     return result;
>   } catch (e) {
>     TelemetryStopwatch.cancel(histogram, key);
>     throw e;
>   }
>   
> }
> 
> ...
> 
> get(keys) {
>   return measureOperation(storageGetHistogram, () => {
>     return context.childManager.callParentAsyncFunction(...);
>   }
> }
> ...
> ```

It was intentional while I was working on the proposed set of patches (especially because I could use the existent tests to ensure that the telemetry was working on both in a similar way), but that is definitely an open question (along with the general open question of the additional telemetry we may want around the backend selection and the result of the data migrations).

I've extracted the pattern and reused on both the JSONFile and IDB backends methods (as you suggested) in the meantime,
and I'm going to file a new issue to discuss (and apply the changes) about the open questions around telemetry.

> Do we care about leaving this open when an extension is not actively using storage?

I was in doubt too, and so in a previous version there was an idle callback that was clearing the db, but then I removed it after the last round of review comments from Kris (comment 84).
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

> Well, evaluate the desidered (and ensure that it would be also "the actual") outcome is the main reason why I tried to add an explicit test case for that lines of code (like I said in my previous comment I also agree that everything that we can store in the json file should also be stored fine by IDB)
> 
> My initial feeling is that a "partial migrated storage data" is not going to make the user happier (or to work better) than "nothing has been migrated, but the old file is still on your disk", and it would probably generate just more confusing (and harder to investigate) bug reports.
> 
> What do you think?

In the last update push to mozreview, I've changed the behavior expected when "the data has been successfully read from the old JSONFile backend but it failed to be saved in the new IDB backend" to not keep using the JSONFile backend and retry to migrate to the new backend on the next extension load (as Andrew and I have been discussed and agreed on last week).

I've also removed this test case, because it is forcing that error scenario, but in a way that isn't immediately clear, and it doesn't exactly reproduce the exact same error conditions.

(I've also rebased the patches to a more recent mozilla-central tip, but I'm pushing it to mozreview separately so that the interdiff of the part described above is still readable and separated from the changes related to the rebase).
As mentioned in Comment 193, the last set of mozreview-request comments above are all related to the rebase of the entire set of patches on a more recent mozilla-central tip.
Depends on: 1465120
(In reply to Luca Greco [:rpl] from comment #193)
> ... to not keep using the JSONFile backend and retry to migrate to the new backend on the next extension load...

typo: to KEEP using the JSONfile backend and retry to migrate to the new backend on the next extension load
Depends on: 1465129
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253882

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:32
(Diff revision 14)
> +// It should match the numeric ID reserved by ContextualIdentityService.jsm for the userContext
> +// named "userContextIdInternal.webextStorageLocal" (unfortunately we can't call the
> +// ContextualIdentityService.getPrivateIdentity method from the child processes because
> +// ContextualIndentityService.jsm is accessing files from disk, which is not allowed in child
> +// processes).
> +const RESERVED_USER_CONTEXT_ID = -10;

The comment helps, thank you.  Is there any place we could put this where it could be accessed both from here and from ContextualIdentifyService.jsm?  If there isn't a good candidate, I think this is probably better than creating a new .jsm or something just to hold this constant.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:44
(Diff revision 14)
> +    const storagePrincipal = Services.scriptSecurityManager.createCodebasePrincipal(
> +      extension.baseURI, {userContextId: RESERVED_USER_CONTEXT_ID});

You mentioned earlier that this needs to run in the child process so it can't be added to the Extension class, but how about adding something like `createExtensionPrincipal()` to ExtensionUtils.jsm or ExtensionCommon.jsm?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:368
(Diff revision 14)
> +        // running extension.
> +        promise = context.childManager.callParentAsyncFunction(
> +          "storage.local.IDBBackend.selectBackend", []);
> +      } else {
> +        // If migrating to the IDB backend is not enabled by the preference, then we
> +        // don't need to migrate any data and the new baclend is not enabled.

typo: backend

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:381
(Diff revision 14)
> +          // TODO: This should be a very unlikely scenario, some telemetry data
> +          // about it may be useful.

replace this with an explicit reference to bug 1465129

::: toolkit/components/extensions/child/ext-storage.js:31
(Diff revision 14)
> +
>  this.storage = class extends ExtensionAPI {
> +  getLocalFileBackend(context, {deserialize, serialize}) {
> +    return {
> +      get(keys) {
> +        return measureOp(storageGetHistogram, () => {

Should we be gathering these in the child process?  If we care about the storage performance and comparing the two backends, the IPC overhead should be the same for the two and including it here just adds noise...
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253888

At this point, we're down to nits and things that can happen in follow-ups.  Nice work!

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:325
(Diff revision 14)
> +  get isBackendEnabled() {
> +    return Services.prefs.getBoolPref(BACKEND_ENABLED_PREF, false);
> +  },

Use a lazy preference getter?
Attachment #8920222 - Flags: review?(aswan) → review+
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253882

> The comment helps, thank you.  Is there any place we could put this where it could be accessed both from here and from ContextualIdentifyService.jsm?  If there isn't a good candidate, I think this is probably better than creating a new .jsm or something just to hold this constant.

Ugh, follow-up to my own follow-up.  Of course there is also the migration code that runs in the parent process.  It would be nice to separate this to clearly denote the common/child-only/parent-only code, but I guess we'll just end up with a bunch of small files in that case...
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253882

> You mentioned earlier that this needs to run in the child process so it can't be added to the Extension class, but how about adding something like `createExtensionPrincipal()` to ExtensionUtils.jsm or ExtensionCommon.jsm?

Huh, I had another comment here that ended up somewhere in reviewboard purgatory.
I just read your suggestion about assigning Extension.storagePrincipal in the main process and then referencing it here, and I think that's a good idea.  It would also let you avoid loading ExtensionStorageIDB.jsm in the parent process just for cleanup (you could pretty easily inline `clearStorageForPrincipal(this.storagePrincipal)` in the storage cleanup code.  That still leaves migration of course...
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review253894
Attachment #8969284 - Flags: review?(aswan) → review+
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253882

> Ugh, follow-up to my own follow-up.  Of course there is also the migration code that runs in the parent process.  It would be nice to separate this to clearly denote the common/child-only/parent-only code, but I guess we'll just end up with a bunch of small files in that case...

Actually, a nice side-effect of "creating a storagePrincipal once from Extension.jsm and then propagate it to the other child processes" is that we can now retrieve the right userContextId directly from ContextualIdentifyService.getPrivateIdentity, which is supposed to be the more clean way to retrieve the numeric id of an internally reserved identity (e.g. the other "internal reserved" identity is used to create the thumbnails).

> Huh, I had another comment here that ended up somewhere in reviewboard purgatory.
> I just read your suggestion about assigning Extension.storagePrincipal in the main process and then referencing it here, and I think that's a good idea.  It would also let you avoid loading ExtensionStorageIDB.jsm in the parent process just for cleanup (you could pretty easily inline `clearStorageForPrincipal(this.storagePrincipal)` in the storage cleanup code.  That still leaves migration of course...

I've removed the clearStorage methods, and just opted to clear the storage calling Services.qms.clearStoragesForPrincipal:

- in the uninstall observer we already create the regular principal to clear the extension pages' IndexedDB, and we can also create the storagePrincipal from there using the userContextId retrived by calling `ContextualIdentityService.getPrivateIdentity(WEBEXT_STORAGE_USER_CONTEXT_ID)` (and Extension.jsm doesn't need to load ExtensionStorageIDB.jsm anymore)

- during the data migration we have access to the extension.storagePrincipal property (and we can call 
  just call Services.qms.clearStoragesForPrincipal using that)
  
(See https://reviewboard.mozilla.org/r/191228/diff/21-22/ for a interdiff which includes the changes described above)

> Should we be gathering these in the child process?  If we care about the storage performance and comparing the two backends, the IPC overhead should be the same for the two and including it here just adds noise...

uhm, I think that comparing the two backends from the same process is more fair, especially because the JSONFile storage.local backend was already measured this way, and changing that would change the measure in ways that doesn't take the IPC overhead anymore, and also because the two backends are a bit different from an IPC point fo view (e.g. the JSONFile backend serialize the data and send it in a messageManager message, while the IDBBackend use the IPC mechanisms of the underlying IndexedDB API, and in the main process the read and writes should happen in a separate IndexedDB thread instead of the main one).
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253888

> Use a lazy preference getter?

Unfortunately that will break the current testing stategy, which switch the enabled backend at runtime (and the lazy getter will keep the value set when is being accessed for the first time.
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253888

> Unfortunately that will break the current testing stategy, which switch the enabled backend at runtime (and the lazy getter will keep the value set when is being accessed for the first time.

It shouldn't -- lazy preference getters also attach an observer that watches for runtime changes:
https://searchfox.org/mozilla-central/rev/83a923ef7a3b95a516f240a6810c20664b1e0ac9/js/xpconnect/loader/XPCOMUtils.jsm#393-407
Comment on attachment 8920222 [details]
Bug 1406181 - Add ExtensionStorageIDB JSM module.

https://reviewboard.mozilla.org/r/191230/#review253888

> It shouldn't -- lazy preference getters also attach an observer that watches for runtime changes:
> https://searchfox.org/mozilla-central/rev/83a923ef7a3b95a516f240a6810c20664b1e0ac9/js/xpconnect/loader/XPCOMUtils.jsm#393-407

well, you are right, I just misread "lazy pref getter" as "lazy getter" in your previous comment.

I applied this change and updated the attached patch (interdiff: https://reviewboard.mozilla.org/r/191228/diff/22-23/)
Comment on attachment 8969284 [details]
Bug 1406181 - Test storage.local data migration from JSONFile to IDB backend.

https://reviewboard.mozilla.org/r/238008/#review248386

> In the last update push to mozreview, I've changed the behavior expected when "the data has been successfully read from the old JSONFile backend but it failed to be saved in the new IDB backend" to not keep using the JSONFile backend and retry to migrate to the new backend on the next extension load (as Andrew and I have been discussed and agreed on last week).
> 
> I've also removed this test case, because it is forcing that error scenario, but in a way that isn't immediately clear, and it doesn't exactly reproduce the exact same error conditions.
> 
> (I've also rebased the patches to a more recent mozilla-central tip, but I'm pushing it to mozreview separately so that the interdiff of the part described above is still readable and separated from the changes related to the rebase).

Follow up for this issue filed as "Bug 1465129 - Collect some telemetry data related to the storage.local "JSONFile to IDBBackend" data migration failures", in the meantime I'm clearing this issue from mozreview.
I've just changed one more time the creation of the storagePrincipal (and its propagation to the child processes) to be a bit more lazy, in particular to defer the lazy import of the ContextualIdentityService.jsm as much as possible (e.g. the previous version that was creating the storagePrincipal from the Extension class constructor was failing on "browser/base/content/test/performance/browser_startup.js" because ContextualIdentityService was "imported before handling the user events", which is prevented by the test for obvious performance reasons, link to the failure on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de10f21355b57794e440c6ca621db3827c210811&selectedJob=181176601)

The new version lazily creates the storagePrincipal (in the main process) when the IDB backend is being selected, and then it propagates the storagePrincipal to the child processes when asked for the enabled backend (and so the ContextualIdentityService.jsm is only loaded from Extension.jsm to clear the extension storages when an extension is uninstalled, and from ExtensionStorageIDB.jsm only when the new IDBBackend is being used for the first time from a storage.local API call).

To send the storagePrincipal across the processes, it is being serialized into a StructuredCloneHolder (related to a privileged JS global, otherwise it would not be allowed) and then deserialized back into an nsIPrincipal once received by the child processes.

(here is an interdiff of these last round of changes https://reviewboard.mozilla.org/r/191228/diff/21-26/, the changes to ContextualIdentityService.jsm are unrelated to the changes described above, those are actually fixing a wrong behavior of the `deleteContainerData` method that I notice from a android-only test failure in a previous push to try).
Attachment #8968193 - Flags: review?(kmaglione+bmo)
Attachment #8920222 - Flags: review?(kmaglione+bmo)
Attachment #8969284 - Flags: review?(kmaglione+bmo)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/fdac47b8ef20
Move serialize/deserialize helpers from ext-c-storage.js to ExtensionStorage.jsm. r=kmag
https://hg.mozilla.org/integration/autoland/rev/912a2eaf4d26
Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm. r=aswan
https://hg.mozilla.org/integration/autoland/rev/3241c2dfb296
Add identity reserved for storage.local extension API to ContextualIdentityService. r=baku
https://hg.mozilla.org/integration/autoland/rev/d34810cab822
Add ExtensionStorageIDB JSM module. r=aswan
https://hg.mozilla.org/integration/autoland/rev/62e9126ecd0d
Test storage.local data migration from JSONFile to IDB backend. r=aswan
https://hg.mozilla.org/integration/autoland/rev/db783c96c076
Test storage.local JSONFile and IndexedDB backends. r=kmag
https://hg.mozilla.org/integration/autoland/rev/4f387b4a76a9
Test storage.local telemetry on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/b52c2fb70ae1
Test storage.local cleanup on uninstall on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/fd61d9faedf0
Test storage.local from content scripts on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/06461ddb2699
Test storage.local from extension tab pages on file and indexedDB backends. r=aswan
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d3c72079cfd
Backed out 10 changesets as per developers request.
(In reply to Pulsebot from comment #280)
> Backout by csabou@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/3d3c72079cfd
> Backed out 10 changesets as per developers request.

The patches were successfully landed on autoland without any new test failures, but I asked to backout them because in the meantime I’ve noticed another issue with the reserved userContextId that we didn’t caught during the reviews.

In Bug 1406181 Comment 33 ashut suggested to use a negative userContextId for our reserved userContextId, and using the negative userContextId seems to work correctly at a first glance, e.g the IndexedDB database is definitely not directly accessible to the extension code.

Unfortunately there are also some “not immediately visible” issues, that we should handle right now, especially given that reserving the userContextId requires a data migration on the containers.json state file:

In the originAttributes webidl, the userContextId is defined as an "unsigned long" (https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/chrome-webidl/ChromeUtils.webidl#367), which means that when we create a principal with a negative userContextId, that userContextId will be automatically converted into a unsigned number and that makes the following check:

     const expectedUserContextId = ContextualIdentityService.getPrivateIdentity(
       "userContextIdInternal.webextStorageLocal").userContextId;

     originAttributes.userContextId === expectedUserContextId

to be false, when we would expect it to be true.

Andrea, what would be the best way to fix this issue from your point of view?
e.g. "Explicitly allow the originAttributes userContextId to be a signed number (as per webidl)" is something that you consider a viable option and you would be open to?

If using a negative userContextId in the originAttributes is not a viable option in practice, then the definition in ContextualIdentityService.jsm for the reserved userContextId should also be explicitly an unsigned number (to avoid any confusion, and bugs that can come from that), and the logic around lastUserContextId changed to take it into account.

As an example, when userContextId -10 is converted into a non-negative number, it comes 4294967286, and then we should also make sure that:

  - the `lastUserContextId` (which is also part of the containers.json) is still computed correctly (currently it is computed by sorting the array of all the defined identities and picking the last one, which still gives 5 instead of 4294967286 just because ContextualIdentityService.jsm is using sort() without any parameter to sort the array, and by default it sorts the array according to the string conversion of each element of the array, and so it gives `[1,2,3,4,4294967286,5].pop() => 5`)

  - ContextIdentityService.create should not be allowed to create an identity which uses the same userContextId reserved for the “extension storage.local data” by any chance
(and that method doesn’t currently check explicitly if the picked userContextId is reserved, it just increments the last used one and assuming that it is not being used, 4294967286 is pretty far from 5, but it still something that should not be allowed explicitly)
Flags: needinfo?(amarchesini)
Iteration: --- → 62.4 - Jul 2
Andrea and I have discussed about the issue with negative userContextId described in Comment 281 and we agreed on changing the patch included in this bug to reserve 4294967295 (which is UINT32_MAX) for the private "userContextIdInternal.webextStorageLocal" identity.

I'm clearing the needinfo assigned to Android, I'm going to update the patch accordingly (and add some additional assertion to the test cases) and ask Andrea to review the additional changes applied in the next push to mozreview.
Flags: needinfo?(amarchesini)
typo :-(
s/assigned to Android/assigned to Andrea/
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

Hi Andrea,
Do you mind to review the additional changes applied to this patch based on our brief discussion?

The following is a link to the interdiff restricted to this last round of changes:
https://reviewboard.mozilla.org/r/191228/diff/29-30/
Attachment #8954173 - Flags: review?(amarchesini)
Attachment #8954173 - Flags: review?(amarchesini)
Comment on attachment 8954173 [details]
Bug 1406181 - Add identity reserved for storage.local extension API to ContextualIdentityService.

https://reviewboard.mozilla.org/r/223334/#review255754
Attachment #8954173 - Flags: review?(amarchesini) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2174226b26dc
Move serialize/deserialize helpers from ext-c-storage.js to ExtensionStorage.jsm. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e98870938f1b
Add openForPrincipal static method and changes to IDB cursor wrapping in IndexedDB.jsm. r=aswan
https://hg.mozilla.org/integration/autoland/rev/5851a229516d
Add identity reserved for storage.local extension API to ContextualIdentityService. r=baku
https://hg.mozilla.org/integration/autoland/rev/7ca1fccbe7b7
Add ExtensionStorageIDB JSM module. r=aswan
https://hg.mozilla.org/integration/autoland/rev/0f5748ff685d
Test storage.local data migration from JSONFile to IDB backend. r=aswan
https://hg.mozilla.org/integration/autoland/rev/85a31c13073a
Test storage.local JSONFile and IndexedDB backends. r=kmag
https://hg.mozilla.org/integration/autoland/rev/645c311bf4ca
Test storage.local telemetry on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/077af955cd73
Test storage.local cleanup on uninstall on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/73030596b51f
Test storage.local from content scripts on file and indexedDB backends. r=aswan
https://hg.mozilla.org/integration/autoland/rev/de1544d92b89
Test storage.local from extension tab pages on file and indexedDB backends. r=aswan
Hi :rpl

A patch in this bug, more specifically this one,

Bug 1406181 - Add ExtensionStorageIDB JSM module. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca1fccbe7b7

Appears to have broken my raptor performance framework web extension [1].

More specifically, after my web extension does a tab.update, my web extension's content script fails to read the local extension storage (never returns from a browser.storage.local.get call). It works the first time the tab is updated to a URL after an empty tab is created, but the next time after that, when the tab is updated, the content script can no longer read the local storage.

Raptor is a Firefox desktop performance framework that is under development (running locally, and on try on OSX but not in production yet).

Can you please take a look? Any help would be greatly appreciated. :)

[1] https://searchfox.org/mozilla-central/source/testing/raptor/webext/raptor
Flags: needinfo?(lgreco)
Depends on: 1467720
(In reply to Robert Wood [:rwood] from comment #314)
> Hi :rpl
> 
> A patch in this bug, more specifically this one,
> 
> Bug 1406181 - Add ExtensionStorageIDB JSM module. r=aswan
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca1fccbe7b7
> 
> Appears to have broken my raptor performance framework web extension [1].
> 
> More specifically, after my web extension does a tab.update, my web
> extension's content script fails to read the local extension storage (never
> returns from a browser.storage.local.get call). It works the first time the
> tab is updated to a URL after an empty tab is created, but the next time
> after that, when the tab is updated, the content script can no longer read
> the local storage.
> 
> Raptor is a Firefox desktop performance framework that is under development
> (running locally, and on try on OSX but not in production yet).
> 
> Can you please take a look? Any help would be greatly appreciated. :)

Hi Robert,
Thanks for having quickly identified and reported this regression, I took a look at it immediately.

The underlying issue should be the same one also reported by Bug 1467720, which has a similar STR: in raptor the issue is triggered after a content script context is being destroyed, and the extension mentioned in Bug 1467720 after an extension popup has been destroyed.

I've briefly tried the raptor extension locally on a Nightly built with the patch attached to Bug 1467720 applied, and Raptor's content script seems to be working as expected with the fix applied (browser.storage.local.get doesn't get stuck when called from the content script after a page navigation).

Let me know if you have a chance to confirm that, on a Nightly built with the patch from Bug 1467720 applied, the Raptor's webextension functionalities are fully restored or if you see other regressions that are preventing it from working as expected.
Flags: needinfo?(lgreco)
Depends on: 1467727
(In reply to Luca Greco [:rpl] from comment #315)

> Let me know if you have a chance to confirm that, on a Nightly built with
> the patch from Bug 1467720 applied, the Raptor's webextension
> functionalities are fully restored or if you see other regressions that are
> preventing it from working as expected.

Hey Luca,

Thanks for the speedy reply! I applied the patch in Bug 1467720 and did a nightly build, and yes indeed that patch does also fix Raptor's web extension. Thank you! :)

Rob
Product: Toolkit → WebExtensions
Hello.

I've received a report of a bug in my add-on in devedition. hg bisect also showed this commit:
changeset:   421613:7ca1fccbe7b7
user:        Luca Greco <lgreco@mozilla.com>
date:        Tue Oct 17 04:12:15 2017 +0200
summary:     Bug 1406181 - Add ExtensionStorageIDB JSM module. r=aswan

As far as I can see, patch from Bug 1467720 is in mozilla-central but the problem still exists.

Problem code (running in browser action popup):
    checkbox.onchange = () => { browser.storage.local.set({enabled: checkbox.checked}); window.close(); };

I can easily fix the issue by doing like this:
    checkbox.onchange = () => { browser.storage.local.set({enabled: checkbox.checked}).then(() => window.close()); };

However, I doubt that lost call to browser.storage.local.set() even in such case is valid behaviour.
Depends on: 1474557
Attached image Bug1406181.png
This issue is verified as fixed on Firefox 63.0a1 (20180716221418) and Firefox 62.0b9 (20180713213322) under Win 7 64-bit and Mac OS X 10.13.3.

The preference “extensions.webextensions.ExtensionStorageIDB.enabled” is set to false by default.

After Bug1474562 is fixed, the preference will be set to true by default on Firefox 63.

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
Iteration: 62.4 - Jul 2 → 63.3 - Aug 6
Blocks: ss-SM
No longer blocks: ss-SM

relnote: New backend for the storage.local WebExtensions API, providing I/O performance improvements when the extension updates a small subset of the stored data [1]

[1] The JSONFile backend that we currently use is always serializing all the data stored on every update, whereas the IndexedDB backend is able to write on disk just the updated keys/values pairs.

relnote-firefox: --- → ?

Adding this as a release note for 66 Beta. There are studies coming up to help determine whether this is ready to ride to release in 66.

I believe the studies are done and the change is made in bug 1488825.

Blocks: IndexedDB-SM
No longer blocks: Session_managers
You need to log in before you can comment on or make changes to this bug.