1381297 - Store the installDate of an extension as a number in the extension-settings.json file

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[ngriswold]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170715030206

Steps to reproduce:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_url_overrides to override the "newtab" page like the example. It stopped working with 7/15/17 56.0a1.

I tested 7/14 56.0a1 and 54.0.1 with same profile and "newtab" override works again.


Actual results:

The newtab page instead opens to the normal setting (top sites or blank page).


Expected results:

The newtab page should have been overridden.

Comment 1

[Bob Silverberg [:bsilverberg]]

I just tried to reproduce this in today's Nightly and was not able to. I installed an extension which uses the chrome_url_overrides.newtab manifest entry and it overrode the new tab functionality. I tested restarting Firefox and the new tab was still overridden by the extension.

ngriswold, could you please attach an extension to the bug which reproduces this behaviour for you, and describe the exact steps you perform to reproduce the behaviour?

Comment 2

[ngriswold]

I just tried again with a new windows profile (so also new firefox profile) on the same machine and the newtab page works fine on the latest Nightly. Seems to be a mix of something in my firefox profile and the latest Nightly builds causing the issue as I can revert back to Nightly builds from last week and using the same firefox profile, it works. I tried with no add-ons enabled and just loading the temp. newtab addon in about:debugging, but it still doesn't work on the latest with my profile. I'll dig around some more. I'll attached what I'm testing with.

Comment 3

[ngriswold]

Attachment: newtab.zip

Comment 4

[ngriswold]

Here is how I am able to reproduce the issue with the attached test add-on:

Fresh new unused Firefox profile, open for the first time:
  1. about:debugging and load temporary add-on
  2. click new tab button, add-on works as expected
  3. close Firefox
  4. reopen Firefox (same profile), about:debugging and load temporary add-on again
  5. new tab button now opens to default setting (top sites).

It seems to only work on a fresh profile the first time it is loaded. Note this testing was done on two different windows computers with different local windows and Firefox profiles and is only an issue with 7/15+ Nightly builds.

Comment 5

[ngriswold]

I found that if I delete extension-settings.json in the Firefox profile directory, restart Nightly, load add-on again in about:debugging, it then works. It won't work again though until extension-settings.json is deleted and Nightly restarted.

Comment 6

[ngriswold]

Is extension-settings.json new as of last week? This file isn't generated with earlier builds and seems to be causing the issue on new builds.

Comment 7

[ngriswold]

I was able to workaround this issue by manually setting an application ID with the manifest.json.

Comment 8

[Bob Silverberg [:bsilverberg]]

Thanks for the updates, ngriswold. I was able to reproduce the problem, and I plan on doing some more debugging of it tomorrow. extension-settings.json is a file that holds information about settings controlled by extensions and it's been around for some time, but will only be created if an extension needs it.

Comment 9

[Bob Silverberg [:bsilverberg]]

There are two issues contributing to this problem:

1. We are storing the installDate of each extension in the settings store as a stringified date, which causes problems when it is compared to a date that was added in the current session. This will be addressed by storing these as numeric dates rather than strings. I am going to convert this bug to being about that fix.

2. When an extension is temporarily loaded, it is automatically uninstalled when the browser shuts down, but due to a bug the setting is not removed. This problem is already logged as bug 1359558, and I will be working on a fix for that as well.

Comment 10

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

https://reviewboard.mozilla.org/r/159238/#review165482

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:63
(Diff revision 1)
> +  let initStore = new JSONFile({
> +    path: STORE_PATH,
> +  });
> +  initStore.ensureDataReady();
> +  _store = initStore;

please don't copy the guts of `getStore()`.  just set `_store` to null here (and call `getStore()` directly if necessary)

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:109
(Diff revision 1)
> +// Converts a stringified date into a numeric date.
> +// Used to support settings that were stored when we were still storing
> +// stringified dates.
> +function ensureNumeric(installDate) {
> +  return (typeof installDate != "number") ? new Date(installDate).valueOf() : installDate;
> +}

lets just do this once when we read the data from disk rather than keeping the old format indefinitely.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:431
(Diff revision 1)
> +
> +  /**
> +   * Test-only method to force reloading of the JSON file.
> +   *
> +   */
> +  async reloadFile() {

no need for async/await here.  also maybe put a leading underscore on the name to emphasize that regular callers shouldn't be using this.

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

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

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

https://reviewboard.mozilla.org/r/159238/#review165482

> please don't copy the guts of `getStore()`.  just set `_store` to null here (and call `getStore()` directly if necessary)

I still need to call `finalize()`, but just setting `_store` to `null` works after doing that.

> lets just do this once when we read the data from disk rather than keeping the old format indefinitely.

Ok, I added a dataPostProcessor to do that. After writting the nested looping code I suspected you would not like it, so I tried writing it with `map`s, but that was even uglier.

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

https://reviewboard.mozilla.org/r/159238/#review165894

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:69
(Diff revision 2)
> +function dataPostProcessor(json) {
> +  for (let storeType in json) {
> +    for (let setting in json[storeType]) {
> +      for (let extData of json[storeType][setting].precedenceList) {
> +        if (typeof extData.installDate != "number") {
> +          extData.installDate = new Date(extData.installDate).valueOf();
> +        }
> +      }
> +    }
> +  }
> +  return json;
> +}

We could avoid doing this every time we read the file by adding a `version` property to the json but I don't feel strongly either way.

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

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

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

https://reviewboard.mozilla.org/r/159238/#review165894

> We could avoid doing this every time we read the file by adding a `version` property to the json but I don't feel strongly either way.

Good idea. I've made that change.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ce634e4577e
Store the installDate of an extension as a number in the extension-settings.json file, r=aswan

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ce634e4577e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I *think* this goes back to 54 if my archaeology is correct? Is this something we should consider backporting to 55 before we build the RC on Monday or can it ride the 56 train?

Comment 20

[Bob Silverberg [:bsilverberg]]

Yes, ideally this would be uplifted, but there is a series of patches that would need to be uplifted, as this relies on some other changes as well. I'll see if I can compile the list, test them against beta, and then request uplifts.

Thanks for noticing this, Ryan.

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

Approval Request Comment
[Feature/Bug causing the regression]: 1320736
[User impact if declined]: This addresses an issue with multiple extensions being installed which manage the same setting. If installed in separate browser sessions the second install will not take control of the setting as expected.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: 1330494 (Part 1)
[Is the change risky?]: No
[Why is the change risky/not risky?]: These are minor changes to the ExtensionSettingsStore.jsm which are covered by tests.
[String changes made/needed]: None

Comment 22

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=530777b3ff8d4060c0cef262265b0720c620a2de

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8888283 [details]
Bug 1381297 - Store the installDate of an extension as a number in the extension-settings.json file,

I'll let this ride 56, doesn't sound like it'd affect too many users and it's really late in 55