Closed Bug 1381297 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Frontend, defect, P3)

54 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: ngriswold, Assigned: bsilverberg)

References

Details

(Keywords: regression, Whiteboard: investigate)

Attachments

(2 files)

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.
Component: Untriaged → WebExtensions: Frontend
Keywords: regression
Product: Firefox → Toolkit
Assignee: nobody → bob.silverberg
Whiteboard: investigate
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?
Flags: needinfo?(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.
Flags: needinfo?(ngriswold)
Attached file newtab.zip
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.
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.
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.
I was able to workaround this issue by manually setting an application ID with the manifest.json.
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.
Priority: -- → P3
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: chrome_url_overrides newtab stopped working → Store the installDate of an extension as a number in the extension-settings.json file
Blocks: 1381573
No longer blocks: 1381573
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.
Attachment #8888283 - Flags: review?(aswan) → review-
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 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.
Attachment #8888283 - Flags: review?(aswan) → review+
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.
Pushed by bsilverberg@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2ce634e4577e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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?
Flags: needinfo?(bob.silverberg)
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 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
Flags: needinfo?(bob.silverberg)
Attachment #8888283 - Flags: approval-mozilla-beta?
Blocks: 1320736
Version: 56 Branch → 54 Branch
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
Attachment #8888283 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.