1475306 - Improve the detection of extensions already migrated to ExtensionStorageIDB and rename the successfully storage.local JSONFile instead of removing it

Status: VERIFIED FIXED

(WebExtensions :: Storage, enhancement, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This bugzilla issue has been splitted from Bug 1470213, which is actually about recording some telemetry events related to the data migration.

During the review on Bug 1470213, we have discussed with Shane about the behavior of the data migration and the possible error scenarios that we may met when we are migrating the extension to the new backend or when we are checking if the extension has been already migrated.

We agreed that it may be good to apply the following additional changes to the data migration:

- to change the way we detect if an extension has been already migrated successfully, so that we do not wrongly fallback to the JSONFile backend if the extension has been previously migrated to the IndexedDB backend but we fail to detect that the ExtensionStorageIDB database for the extension is not empty because a QuotaExceededError (or another similar exception) is raised while we are opening the IndexedDB connection

- to rename a successfully migrated JSONFile instead of removing it (so that it still possible to provide some information to the users that may want to recover the old stored data if something goes wrong during the data migration and it has not been detected)

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

This patch applies the following changes to the storage.local data migration behaviors:

- An about:config preference is set when an extension has been migrated successfully to the
  storage.local IndexedDB backend (cleared automatically if the addon is uninstalled).
- If the above about:config preference is set, the storage.local IndexedDB backend is enabled
  without attempting to open an IndexedDB connection for the new backend.
- While migrating an extension, if we fail to open the IndexedDB connection, the data migration
  is cancelled and the storage.local API is going to fallback to the storage.local JSONFile backend
  (until the next extension startup, when a new data migration is going to be tried).
- When a migration is completed successfully, the old JSONFile is renamed (by appending ".migrated"
  to its original file name) instead of being removed.

Review commit: https://reviewboard.mozilla.org/r/256632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256632/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

https://reviewboard.mozilla.org/r/256630/#review263472

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:250
(Diff revision 1)
> +  if (isMigratedExtension) {
> +    return;
> +  }
>  
>    try {
>      idbConn = await ExtensionStorageLocalIDB.openForPrincipal(storagePrincipal);

Last Shane's review comment on the previous version of this patch (from https://reviewboard.mozilla.org/r/252268/#comment336356): 

> I'm concerned that QuotaExceeded can mean low disk space or that the quota is actually exceeded.  In the later case, if some unforseen problem occurred and the json file stuck around, we could cause data loss.  I'm not certain what is the best path here, but I would lean towards being ultra conservative and consider QuoataExceeded as meaning the database is not empty, even if the json file exists.  If QuotaExceed && fileExists, we could log an error that migration was cancelled due to possible low disk space.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:250
(Diff revision 1)
> +  if (isMigratedExtension) {
> +    return;
> +  }
>  
>    try {
>      idbConn = await ExtensionStorageLocalIDB.openForPrincipal(storagePrincipal);

Last review comment from Shane on a previous version of this patch (from https://reviewboard.mozilla.org/r/252268/#comment336356):

> I'm concerned that QuoataExceeded can mean low disk space or that the quoata is actually exceeded.  In the later case, if some unforseen problem occurred and the json file stuck around, we could cause data loss.  I'm not certain what is the best path here, but I would lean towards being ultra conservative and consider QuoataExceeded as meaning the database is not empty, even if the json file exists.  If QuotaExceed && fileExists, we could log an error that migration was cancelled due to possible low disk space.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:283
(Diff revision 1)
> +      return false;
> +    });
>  
> +    // Migrate any data stored in the JSONFile backend (if any), and remove the old data file
> +    // if the migration has been completed successfully.
>      if (oldStorageExists) {

Last review comment from Shane on a previous version of this patch (from https://reviewboard.mozilla.org/r/252268/#comment336370):

> Given my quota comments above and the one below where you check file exists again, this would be oldStorageExists && dataMigrateStarted

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:326
(Diff revision 1)
> -  if (oldStorageExists && migrated) {
> +  if (oldStorageExists && dataMigrateCompleted) {
>      try {
> -      await OS.File.remove(oldStoragePath);
> +      // Only migrate the file when it actually exists (e.g. the file name is not going to exist
> +      // when it is corrupted, because JSONFile internally rename it to `.corrupt`.
> +      if (await OS.File.exists(oldStoragePath)) {
> +        let openInfo = await OS.File.openUnique(`${oldStoragePath}.migrated`, {humanReadable: true});
> +        await openInfo.file.close();
> +        await OS.File.move(oldStoragePath, openInfo.path);
> +      }
>      } catch (err) {
>        extension.logWarning(err.message);
>      }
>    }

Last review comment from Shane on a previous version of this patch (from https://reviewboard.mozilla.org/r/252268/#comment336354)

> Lets not remove the old data for a while and just rename it, at least until after the next esr that gets this change.  Maybe we should set a pref (hasMigrationBackup=true ?) to flag that we should check for and remove it at that time (rather than always checking.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

https://reviewboard.mozilla.org/r/256632/#review263516

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:244
(Diff revision 1)
> -  let oldDataRead = false;
> -  let migrated = false;
>    let histogram = Services.telemetry.getHistogramById(IDB_MIGRATE_RESULT_HISTOGRAM);
> +  let dataMigrateCompleted = false;
> +
> +  const isMigratedExtension = Services.prefs.getBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, false);

I'm re-thinking...It is nice to avoid any of the other stuff if migration has happened.  However, we dont really need a pref.  Once the extension is migrated we shouldn't run into any issue.  The only reason I can think for a pref is if there is enough startup overhead that this is avoiding.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:328
(Diff revision 1)
> -      await OS.File.remove(oldStoragePath);
> +      // Only migrate the file when it actually exists (e.g. the file name is not going to exist
> +      // when it is corrupted, because JSONFile internally rename it to `.corrupt`.
> +      if (await OS.File.exists(oldStoragePath)) {
> +        let openInfo = await OS.File.openUnique(`${oldStoragePath}.migrated`, {humanReadable: true});
> +        await openInfo.file.close();
> +        await OS.File.move(oldStoragePath, openInfo.path);
> +      }

This seems like a lot of stuff we dont need.  move should throw if the file doesn't exist.  Is there a reason we cannot just:

if (oldStorageExists && dataMigrateCompleted) {
  await OS.File.move(oldStoragePath, `${oldStoragePath}.migrated`, {noOverwrite: true}).
  catch(reason => {log});
}

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

https://reviewboard.mozilla.org/r/256632/#review263516

> I'm re-thinking...It is nice to avoid any of the other stuff if migration has happened.  However, we dont really need a pref.  Once the extension is migrated we shouldn't run into any issue.  The only reason I can think for a pref is if there is enough startup overhead that this is avoiding.

If we don't want it to be a pref, then we need to think of another solution, but the issue is not related to the startup overhead, we need to avoid that an extension may switch back to the JSONFile backend by mistake because of transient IndexedDB issues like the QuotaExceededError.

Here is the error scenarios that we were discussing in Bug 1470213: 

`idbConn = await ExtensionStorageLocalIDB.openForPrincipal(storagePrincipal);`

may raise an exception (e.g. the QuotaExceededError if the disk is full, etc.) and so we may never reach `hasEmptyIDB = await idbConn.isEmpty();`, and this basically means that we cannot determine if the extension has been already switched to the IndexedDB backend by just checking if it has data stored in the IndexedDB backend.

So, based on our discussions on the topic in Bug 1470213 and over IRC, we would actually want that:

- if the extension has never been switched to the IndexedDB backend, and we get a rejection when we are opening the IDB connection, then we should fall back to the JSONFile (and retry to migrate the extension the next time it is started)

- if the extension has been previously migrated to the IndexedDb backend, it doesn't matter if we get rejections when we open the IDB connection, we should never fall back to the JSONFile (otherwise the user will definitely lose some data, e.g. by switching between the two backends every time we get a rejection when we are opening the IndexedDB connection)

> This seems like a lot of stuff we dont need.  move should throw if the file doesn't exist.  Is there a reason we cannot just:
> 
> if (oldStorageExists && dataMigrateCompleted) {
>   await OS.File.move(oldStoragePath, `${oldStoragePath}.migrated`, {noOverwrite: true}).
>   catch(reason => {log});
> }

Yes, there is an actual reason why we can't assume that the file still exists after the data migration:

the file may exist when we started the migration, but it may not be there anymore because it has been renamed internally by JSONFile (and it is not an actual error condition for the data migration, we should just not trying to rename a file that doesn't exist anymore):

https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/modules/JSONFile.jsm#205-210

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

https://reviewboard.mozilla.org/r/256632/#review263516

> If we don't want it to be a pref, then we need to think of another solution, but the issue is not related to the startup overhead, we need to avoid that an extension may switch back to the JSONFile backend by mistake because of transient IndexedDB issues like the QuotaExceededError.
> 
> Here is the error scenarios that we were discussing in Bug 1470213: 
> 
> `idbConn = await ExtensionStorageLocalIDB.openForPrincipal(storagePrincipal);`
> 
> may raise an exception (e.g. the QuotaExceededError if the disk is full, etc.) and so we may never reach `hasEmptyIDB = await idbConn.isEmpty();`, and this basically means that we cannot determine if the extension has been already switched to the IndexedDB backend by just checking if it has data stored in the IndexedDB backend.
> 
> So, based on our discussions on the topic in Bug 1470213 and over IRC, we would actually want that:
> 
> - if the extension has never been switched to the IndexedDB backend, and we get a rejection when we are opening the IDB connection, then we should fall back to the JSONFile (and retry to migrate the extension the next time it is started)
> 
> - if the extension has been previously migrated to the IndexedDb backend, it doesn't matter if we get rejections when we open the IDB connection, we should never fall back to the JSONFile (otherwise the user will definitely lose some data, e.g. by switching between the two backends every time we get a rejection when we are opening the IndexedDB connection)

After vidyo conversation, I think the pref is the right thing to do.

> Yes, there is an actual reason why we can't assume that the file still exists after the data migration:
> 
> the file may exist when we started the migration, but it may not be there anymore because it has been renamed internally by JSONFile (and it is not an actual error condition for the data migration, we should just not trying to rename a file that doesn't exist anymore):
> 
> https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/modules/JSONFile.jsm#205-210

again, after vidyo this is fine.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

https://reviewboard.mozilla.org/r/256632/#review263728

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256632/diff/1-2/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256632/diff/2-3/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256632/diff/3-4/

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256632/diff/4-5/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8991690 [details]
Bug 1475306 - Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256632/diff/5-6/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fafb8e07051
Changes to the detection of extensions already migrated to ExtensionStorageIDB and rename successfully migrated JSONFile. r=mixedpuppy

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1fafb8e07051

Comment 14

[CosminB]

Attachment: Bug1475306.png

This issue is verified as fixed on Firefox 63.0a1 (20180719100142) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached screenshot.