Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Description • 6 years ago

This is a follow up of Bug 1465129, which is adding a new telemetry histogram to keep track of the incidence of the storage.local data migration failures.

Like briefly described in Bug 1465129 comment 3, I'd like to also collect some additional "per-addon" telemetry in the environment's "addons.activeAddons.<addon-id>" objects, in particular:

- a new "addons.activeAddons.<addon-id>.storageLocalIDBBackend" (which can be true or false, and that we could use to check that the most commonly active extensions are migrating successfully or not to the new storage.local backend)

- addons.activeAddons.<addon-id>.storageLocalMigrateError (which could contain the name, message and code properties from the last raised error that prevented the extension from migrating to the new backend)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 1 • 6 years ago

Hi :chutten,
This is a follow up of Bug 1465129 (as we briefly discussed in Bug 1465129 comment 3 and Bug 1465129 comment 6), related to the additional per-addon telemetry for the storage.local data migrations.

	Chris H-C :chutten
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review258834

This will require an update to the in-tree documentation for the shape of the addonDetails portion of the "main" ping https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/main-ping.html#addondetails (the source for which lives in toolkit/components/telemetry/docs/data/main-ping.rst)


I would also like to see the JSONFile-version tested. (Are all err.message undefined in tests? If not, I'd love to see a test that ensures the correct non-undefined value for err.message as well)

	Chris H-C :chutten
	Comment 4 • 6 years ago

Comment on attachment 8987019 [details]
request-data-review-bug-1470213-perAddon-storageLocal.md

Do we know exactly what might be present in the error messages we are adding to the addonDetails block? I am worried about their size (could have budget impact) and their contents (could contain user-supplied identifiers, which we do not want). I would be more comfortable if the errors could be categorized to a known list e.g. {permissions, fileTooLarge, ..., other} of strings we would control at build time.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 5 • 6 years ago

(In reply to Chris H-C :chutten from comment #3)
> Comment on attachment 8987020 [details]
> Bug 1470213 - Collect some per-addon telemetry related to the storage.local
> data migration results.
> 
> https://reviewboard.mozilla.org/r/252268/#review258834
> 
> This will require an update to the in-tree documentation for the shape of
> the addonDetails portion of the "main" ping
> https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
> telemetry/data/main-ping.html#addondetails (the source for which lives in
> toolkit/components/telemetry/docs/data/main-ping.rst)

Ah, yeah, very good point, I should have thought about it,
because it is exactly the doc that I looked to know where these additional
data could have been collected.

I'm going to add the additional details to the docs as part of the next update on this patch.

> 
> I would also like to see the JSONFile-version tested. (Are all err.message
> undefined in tests? If not, I'd love to see a test that ensures the correct
> non-undefined value for err.message as well)

Sorry, I forgot to mention to you that I've not yet included the failure test in these patches 
(because we are not too happy about the trick currently being used to trigger this failure scenario),
but I'm keeping it updated in Bug 1470208 (here is a link to the diff applied on the existent tests
to cover the failure scenario: https://reviewboard.mozilla.org/r/252074/diff/2#index_header).

I would also really like to actually land the test for the "failure scenario" and so I added the
r? on it in the meantime.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 6 • 6 years ago

(In reply to Chris H-C :chutten from comment #4)
> Comment on attachment 8987019 [details]
> request-data-review-bug-1470213-perAddon-storageLocal.md
> 
> Do we know exactly what might be present in the error messages we are adding
> to the addonDetails block? I am worried about their size (could have budget
> impact) and their contents (could contain user-supplied identifiers, which
> we do not want). I would be more comfortable if the errors could be
> categorized to a known list e.g. {permissions, fileTooLarge, ..., other} of
> strings we would control at build time.

That is also something that has concerned (and still is concerning) me while I've been
looking into how much detail it was reasonable to put into the information collected
(e.g. I thought to avoid to collect the stacktrace from the error basically for the same
reasons you mentioned above, but I was a bit unsure about the error message).

Based on our discussion while reviewing the data migration code in Bug 1406181,
we are more interested in the kind of error that IndexedDB may raise when we
are storing the old data into the new IndexedDB backend (which is also the only
error condition that we are currently considering fatal and it would keep the
JSONFile backend active, and then retry to migrate to the IndexedDB backend again
the next time the extension is going to be started).

And so I'm thinking that collecting the DOMException's name property 
(https://developer.mozilla.org/en-US/docs/Web/API/DOMException/name) may also be enough, 
because it will allow us to know the kind of error raised by IndexedDB 
(e.g. DataCloneError, QuotaExceededError etc) and if the error was more likely a totally
unexpected error which is clearly unrelated to that (e.g. if it is a generic Error, or a SyntaxError etc.). 

This way we are going to be also sure that this property size is not grow too much.

How that would sounds to you?

	Chris H-C :chutten
	Comment 7 • 6 years ago

I'm glad you were concerned!

DOMException names can be provided in the DOMException constructor[1]. I'm not sure how likely it is we might get an author-constructed DOMException in the IDB init code... probably impossible? Can we be sure it is impossible?

If we can be sure it's impossible, then we're definitely in the clear. The DOMException names come from [2] and are universally unparameterized. In Gecko they're just type names, so they're safe.

If we can make a reasoned argument about why it -should- be impossible, then I'm still probably giving datareview+ for this.

If we can't be sure, or we find evidence that it could happen, we may need to write some code to check if the DOMException's name is in the list of ones that come with the browser, or if we have to shunt it to an "Other" bucket.

Can you tell from the code how likely this is?

[1]: https://developer.mozilla.org/en-US/docs/Web/API/DOMException/DOMException
[2]: https://searchfox.org/mozilla-central/source/dom/base/domerr.msg

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252266/#review259308

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:35
(Diff revisions 1 - 2)
> +/**
> + * Get the DOMException error name for the error object, or "OtherError" if the error is not a DOMException instance.
> + * If the error name longer than 80 chars, then a sliced version of the string is returned. Returns undefined if the
> + * error parameter is not defined.
> + *
> + * @param {Error | undefined} error
> + *        The Error object to convert into a string, or undefined if there was no error.
> + *
> + * @returns {string | undefined}
> + *          The DOMException error name (sliced to a maximum of 80 chars),
> + *          "OtherError" if the error object is not a DOMException instance,
> + *          or undefined if there wasn't an error.
> + */
> +function getMigrateErrorName(error) {
> +  if (!error) {
> +    return undefined;
> +  }
> +
> +  if (error instanceof DOMException) {
> +    if (error.name.length > 80) {
> +      return `${error.name.slice(0, 77)}...`;
> +    }
> +
> +    return error.name;
> +  }
> +
> +  return "OtherError";
> +}

Hi :chutten,
I've been thinking about this and, the source of the error we are going to log in the telemetry details should always been coming from our own code (in particular from the IndexedDB implementation, or the JSONFile / ExtensionStorage / ExtensionStorageIDB jsm files) and third party code should be able to raise an error that would be collected instead of a native one (the data migration method is called internally in response of the first storage.local API call generated by the extension).

Nevertheless, it sounds to me that it would be still better to check the string length and slice it so that it can't be longer than 80 chars.

I've updated the patch to include and use this `getMigrateErrorName` helper function on the collected error (which ensure that the string is not longer than 80 chars and also return 'OtherError' for any error object that is not an instance of DOMException).

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:314
(Diff revisions 1 - 2)
>        XPIProvider.addTelemetry(extension.id, {
>          storageLocal: {
>            backend: "JSONFile",
> -          dataMigrateError: err.message,
> +          dataMigrateError: getMigrateErrorName(err),
>          },
>        });

I'm wondering if it could be better to collect this telemetry data using a Telemetry Event (on which the event value could be the addon_id and the properties which contains the selected backend and the error name could become `extra_vars` of the recorded event) instead of the environment's addonDetails object.

After all this is actually an event (e.g. these properties will be set again after the extension has been migrated) and by using a telemetry event we can describe it more esplicily in the Events.yaml file (e.g. including an expire version, the related bug numbers and notification email).

what do you think?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 10 • 6 years ago

I've update the patch based on your last review comments, and in comment 9 I've added some details about the changes and some additional questions (in particular I'm wondering if it would be better to send this telemetry data using a telemetry event instead of the environment's addonDetails).

	Chris H-C :chutten
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review259358

I applaud your instincts to prefer a managed telemetry collection mechanism instead of putting things in addonDetails.

An event may indeed be appropriate, especially if you care -when- the error happens relative to other events. And it allows us to avail ourselves of standardized tooling and expiry. I will happily review a patch that sends this information as events.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 12 • 6 years ago

Hi :chutten,
here is the new data review request, changed to mention the new telemetry events, their expected properties (included the extra_keys) and their expiry version.

Let me know how it looks to you.

(I'm only going to push the related changes on the patch attached to this issue, as well as the patch for the failure test case attached to Bug 1470208).

	Chris H-C :chutten
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review260030

I have one question, but aside from that it appears to be a fine use of Telemetry APIs. The logic of it and its placement in addons code I did not review.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:90
(Diff revisions 2 - 3)
> +      const {
> +        extensionId,
> +      } = telemetryData;
> +
> +      this.lazyInit();
> +      Services.telemetry.recordEvent("extensions.data", "migrateStart", "storageLocal", extensionId);

What is the maximum length of extensionId? If it is longer than 80 bytes, we will truncate it for your but warn to the console. We don't want to flood the console if we can avoid it.

	Chris H-C :chutten
	Comment 15 • 6 years ago

Comment on attachment 8988093 [details]
request-data-review-bug-1470213-perAddon-storageLocal-v2.md

A question before we begin: If the histogram from bug 1465129 isn't getting the job done, should it be removed?

And a note: Event Telemetry doesn't show on telemetry.mozilla.org. You will need to look into another means of analysing event data (possibly by writing some SQL at sql.telemetry.mozilla.org, or by finding an analyst to help you out).

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

N/A, expires in 70

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 1, technical.

    Is the data collection request for default-on or default-off?

Default on.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

Not new, but it does include the extension id. They are already collected and reported in addonDetails and activeAddons in the main ping.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :lgreco will be responsible for renewing or removing the Event collection before it expires.

---
Result: datareview+

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 16 • 6 years ago

(In reply to Chris H-C :chutten from comment #14)
> Comment on attachment 8987020 [details]
> Bug 1470213 - Collect some per-addon telemetry related to the storage.local
> data migration results.
> 
> https://reviewboard.mozilla.org/r/252268/#review260030
> 
> I have one question, but aside from that it appears to be a fine use of
> Telemetry APIs. The logic of it and its placement in addons code I did not
> review.

Thanks! Shane is going to review that part (I told him to wait for the confirmation that the approach was correct
from a telemetry point of view).

> 
> ::: toolkit/components/extensions/ExtensionStorageIDB.jsm:90
> (Diff revisions 2 - 3)
> > +      const {
> > +        extensionId,
> > +      } = telemetryData;
> > +
> > +      this.lazyInit();
> > +      Services.telemetry.recordEvent("extensions.data", "migrateStart", "storageLocal", extensionId);
> 
> What is the maximum length of extensionId? If it is longer than 80 bytes, we
> will truncate it for your but warn to the console. We don't want to flood
> the console if we can avoid it.

yeah, good point.

The super short answer is: an extension id longer than 80 bytes can be a valid extension id for Firefox (and for a submission on AMO).

All the auto-generated extension ids are going to be shorter than 80 bytes:

- 38 chars long, generated uuid in curly brackets format ("{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}"), when the extension is submitted on AMO and it doesn't have an extension id in the manifest

- 56 chars long, generated unique email-like format ("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@temporary-addon"), when the extension is loaded temporarily in Firefox and it doesn't have an extension id in the manifest

Nevertheless the extension developer can assign an extension id explicitly and that could be longer than 80 chars (the id need to match one of these two formats, "uuid in curly brackets" or the "email-like" format, but both Firefox and the addons-linter do not check how long is the string, and so an email-like extension id longer than 80 characters loads perfectly fine in Firefox and pass the addons-linter validation).

I'm not sure how many of the extensions submitted/signed on AMO have an id longer than 80 chars (but we could check that to get an idea), anyway I would not be against to trim the extension id string upfront when we are going to record the event.

Thanks for pointing it out!

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 17 • 6 years ago

(In reply to Chris H-C :chutten from comment #15)
> Comment on attachment 8988093 [details]
> request-data-review-bug-1470213-perAddon-storageLocal-v2.md
> 
> A question before we begin: If the histogram from bug 1465129 isn't getting
> the job done, should it be removed?

That is definitely something I was also wondering while I've been changing the patch
to use the telemetry event.

I do agree that in the end these events should provide us the same information of the
histogram we added in Bug 1465129 and even more then that (e.g. we should be able to
also compute the time spent for a migration, and we are going to have more details
about the result of the migration).

To me, the only advantages of the histogram seems to be that:
- it is already there in Firefox beta  
- his reporting is immediately available to us on the telemetry.mozilla.org dashboard
  and it is going to tell us how often the migration are failing, without the need to 
  write custom SQL queries and/or custom dashboard.

Once I've got some experience on retrieving and presenting the telemetry event data 
and we are confident that we have that data available without looking at the histogram,
I think that we could expire the Bug 1465129's histogram earlier.

Does this sound like a good approach to you?

> And a note: Event Telemetry doesn't show on telemetry.mozilla.org. You will
> need to look into another means of analysing event data (possibly by writing
> some SQL at sql.telemetry.mozilla.org, or by finding an analyst to help you
> out).

Thanks for mentioning it, that was definitely a doubt that I had 
and I was going to ask you about it.

	Chris H-C :chutten
	Comment 18 • 6 years ago

(In reply to Luca Greco [:rpl] from comment #16)
> I'm not sure how many of the extensions submitted/signed on AMO have an id
> longer than 80 chars (but we could check that to get an idea), anyway I
> would not be against to trim the extension id string upfront when we are
> going to record the event.
> 
> Thanks for pointing it out!

Please do truncate it to avoid triggering the warning. I think there are people who keep an eye on the browser console and wouldn't like to see it drowned with "value too long" messages :)

(In reply to Luca Greco [:rpl] from comment #17)
> Once I've got some experience on retrieving and presenting the telemetry
> event data 
> and we are confident that we have that data available without looking at the
> histogram,
> I think that we could expire the Bug 1465129's histogram earlier.
> 
> Does this sound like a good approach to you?

That sounds like an excellent approach.

	Code Review Bot [:reviewbot]
	Comment 20 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review260450


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_idb_data_migration.js:15
(Diff revision 4)
> -ChromeUtils.import("resource://gre/modules/ExtensionStorageIDB.jsm");
> +ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
> +
> +const {
> +  ExtensionStorageIDB,
> +  DataMigrationTelemetry,
> +} = ChromeUtils.import("resource://gre/modules/ExtensionStorageIDB.jsm");

Error: Cu.import imports into variables and into global scope. [eslint: mozilla/no-import-into-var-and-global]

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 23 • 6 years ago

Hi Shane,
last week I've been also looking for additional failure scenarios to test, and I found out how to force the Quota Manager to raise a QuotaExceededError during the migration tests (it is the same testing strategy also used by some of the existing IndexedDB tests).

This has also unveiled two related additional error scenarios triggered when QuotaExceeededError has been raised (and some additional changes needed to handle them as follows):

- there is a JSONFile to migrate, and in that case the extension does not migrate to the new IndexedDB backend

- there isn't any JSONFile to migrate, and in that case (based on how we are handling the other error scenarios) it seems that we may still want to allow the extension to migrate to the new IndexedDB backend (instead of keeping the JSONFile backend active)

And so, when there is no existing JSONFile, the applied changes are still allowing the extension to migrate to the IndexedDB backend, but then every call to the storage API will raise a QuotaExceededError.

To be fair, it could be better if we may be allowed (e.g. by passing some additional chromeOnly when we are opening the storage.local IndexedDB database) to still open the DB in some kind of "temporary read-only mode", so that the extension can still read the stored data (without being allowed to store or change it).

Another option could be to more explicitly notify the user about the issue with the disk quota.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 26 • 6 years ago

I've just added an additional patch in the last push to mozreview (attachment 8989252 [details]),
which is actually a fix for a wrong behavior that we didn't notice in the previous reviews.

Given that it is actually a fix for the storage.local API IndexedDB backend, we may prefer it to be filed as its own issue, let me know if you think that we should move it out of this issue and I will file a separate bug and move the patch there.

The issue being fixed is related to the dbPromise cached by `getLocalIDBBackend` in "child/ext-storage.js",
from which every one of the storage.local methods retrieve the IndexedDB db.
If ExtensionStorageIDB.open returns a rejected promise (e.g. because of the QuotaExceededError described in comment 23), then all the next storage.local API calls from the same extension context are going to fail because of that previous rejection (and if that context was the background page, then all the storage.local API calls are going to fail until the extension is restarted).

This behavior is actually wrong, e.g. the QuotaExceededError may not be raised anymore once some disk space has been made available by the user and at that point the extension should be able to successfully call the storage.local API without any need to restart the entire extension.

The last interdiff shows the changes applied to "child/ext-storage.js" and how it is being tested by the `test_storage_local_data_migration_quota_exceeded_no_jsonfile` test case (which is part of attachment 8987020 [details]):

- https://reviewboard.mozilla.org/r/252266/diff/6-7/

	Shane Caraveo (:mixedpuppy)
	Comment 27 • 6 years ago
mozreview-review

Comment on attachment 8989252 [details]
Bug 1470213 - Clear the dbPromise cached by ext-storage when IndexedDB raises an exception while opening the db.

https://reviewboard.mozilla.org/r/254302/#review261270

	Shane Caraveo (:mixedpuppy)
	Comment 28 • 6 years ago
mozreview-review

Comment on attachment 8989252 [details]
Bug 1470213 - Clear the dbPromise cached by ext-storage when IndexedDB raises an exception while opening the db.

https://reviewboard.mozilla.org/r/252266/#review261282

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:147
(Diff revision 7)
> +    try {
> +      const {
> +        backend,
> +        dataMigrated,
> +        extensionId,
> +        error,

this is sometimes a nonFatalError, but "error" in report data may be mis-interpreted in the future.  Is it possible make a distinction between the two?

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:385
(Diff revision 7)
> +    DataMigrationTelemetry.recordStart({extensionId: extension.id});
> +
> +    oldStoragePath = ExtensionStorage.getStorageFile(extension.id);
> +    oldStorageExists = await OS.File.exists(oldStoragePath);

I think the change that moved these here should be undone since this is always called on the first api use.  We also shoudn't do anything with telemetry until we know migration is necessary.  If openForPrincipal throws the quota error, the db is not empty.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:398
(Diff revision 7)
> +      DataMigrationTelemetry.recordResult({
> +        backend: "IndexedDB",
> +        dataMigrated: false,
> +        hasJSONFile: oldStorageExists,
> +        extensionId: extension.id,
> +        histogramCategory: "success",
> +      });

This should be removed since it would always be called.  With the following comments, the fact that the old storage still exists will get recorded at the apropriate time.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:462
(Diff revision 7)
> -  histogram.add("success");
> +  DataMigrationTelemetry.recordResult({
> +    backend: "IndexedDB",
> +    dataMigrated,
> +    extensionId: extension.id,
> +    error: nonFatalError,
> +    hasJSONFile: oldStorageExists,
> +    hasOldData,
> +    histogramCategory: "success",
> +  });

Move this after the file removal below.  If the file cannot be removed, oldStorageExists should remain true, and maybe nonFatalError should be the exception  from removal (unless a prior error is more important)

	Shane Caraveo (:mixedpuppy)
	Comment 30 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review261816

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:387
(Diff revision 8)
> -  let histogram = Services.telemetry.getHistogramById(IDB_MIGRATE_RESULT_HISTOGRAM);
> +  let dataMigrateStarted = false;
> +  let dataMigrateCompleted = false;
> +  let hasOldData = false;
>  
>    try {
>      idbConn = await ExtensionStorageLocalIDB.openForPrincipal(storagePrincipal);

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:404
(Diff revision 8)
>      oldStorageExists = await OS.File.exists(oldStoragePath);
> +    dataMigrateStarted = true;
>  
> +    // 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) {

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:429
(Diff revision 8)
> -      // If the data has been read successfully and it has been failed to be stored
> -      // into the IndexedDB backend, then clear any partially stored data and reject
> +    // The data migration has never started (e.g. because an exception has been raise when opening the db),
> +    // we still need to check if there was a JSONFile and prevent the extension from switching to the
> +    // IndexedDB backend if it exists.
> +    if (!dataMigrateStarted) {
> +      oldStoragePath = ExtensionStorage.getStorageFile(extension.id);
> +      oldStorageExists = await OS.File.exists(oldStoragePath).catch(fileErr => {

Couldn't this logic be done above where you check for  the file existing?  Then set dataMigrateStarted = oldStorageExists && !QuotaExceeded.  I'm not certain we need the log entry at all.

::: toolkit/components/extensions/ExtensionStorageIDB.jsm:476
(Diff revision 8)
> -
> -  // If the IDB backend has been enabled, try to remove the old storage.local data file,
> +  // do not prevent the extension from switching to the IndexedDB backend if it fails to be removed.
> +  if (oldStorageExists && dataMigrateCompleted) {
> -  // but keep using the selected backend even if it fails to be removed.
> -  if (oldStorageExists && migrated) {
>      try {
>        await OS.File.remove(oldStoragePath);

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.

	Shane Caraveo (:mixedpuppy)
	Comment 34 • 6 years ago
mozreview-review

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review264010

::: toolkit/components/extensions/child/ext-storage.js:194
(Diff revision 10)
> -        return backend[method](...args);
> +          const result = await backend[method](...args);
> +          return result;

Just return?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 35 • 6 years ago
mozreview-review-reply

Comment on attachment 8987020 [details]
Bug 1470213 - Collect some per-addon telemetry related to the storage.local data migration results.

https://reviewboard.mozilla.org/r/252268/#review264010

> Just return?

we can't just return that result here because we want that the errors rejected by that call to be caught by the try...catch and converted into an ExtensionUtils.ExtensionError to be accessible to the extension code (e.g. storage.local.set may get a DataCloneError, a QuotaExceededError etc., and it would be a rejection of that call, and if we just return the result promise then it would be converted into "An unexpected error occurred").

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 38 • 6 years ago

eh... one of those times when the three-way merge says "the rebased changes have been applied cleanly and you don't need to resolve any conflicts manually", but it really means "I'm going to mess with this yaml file, and I'm not telling you about it, so that you can get some fun by find out what happened"

The last push ([1]) fixes the mess that the automatic conflict resolution introduced in Events.yaml in the last rebase of this patch ([2]).  

[1]: https://reviewboard.mozilla.org/r/252268/diff/11-12/
[2]: https://reviewboard.mozilla.org/r/252268/diff/9-10/

	Pulsebot
	Comment 40 • 6 years ago

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/6ad1a1dfdf4b
Collect some per-addon telemetry related to the storage.local data migration results. r=chutten,mixedpuppy

	Natalia Csoregi [:nataliaCs]
	Comment 41 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6ad1a1dfdf4b

	CosminB
	Comment 42 • 6 years ago

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

Please see the attached video.