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

VERIFIED FIXED in Firefox 63

Status

enhancement
P1
normal
VERIFIED FIXED
11 months ago
11 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(2 attachments)

Assignee

Description

11 months ago
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)
Assignee

Updated

11 months ago
Assignee: nobody → lgreco
Blocks: 1474562
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
Assignee

Updated

11 months ago
Blocks: 1470213
Depends on: 1474557
Assignee

Comment 2

11 months ago
mozreview-review
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

11 months ago
mozreview-review
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});
}
Attachment #8991690 - Flags: review?(mixedpuppy)
Assignee

Comment 4

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review
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
Attachment #8991690 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

11 months ago
Pushed by luca.greco@alcacoop.it:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fafb8e07051
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 14

11 months ago
Posted image 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.

Updated

11 months ago
Status: RESOLVED → VERIFIED
Depends on: 1476929
You need to log in before you can comment on or make changes to this bug.