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

RESOLVED FIXED in Firefox 56

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: ngriswold, Assigned: bsilverberg)

Tracking

({regression})

54 Branch
mozilla56
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: investigate)

Attachments

(2 attachments)

Reporter

Description

2 years ago
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

Updated

2 years ago
Assignee: nobody → bob.silverberg
Whiteboard: investigate
Assignee

Comment 1

2 years ago
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)
Reporter

Comment 2

2 years ago
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)
Reporter

Comment 3

2 years ago
Posted file newtab.zip
Reporter

Comment 4

2 years ago
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.
Reporter

Comment 5

2 years ago
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.
Reporter

Comment 6

2 years ago
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.
Reporter

Comment 7

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

Comment 8

2 years ago
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.

Updated

2 years ago
Priority: -- → P3
Assignee

Comment 9

2 years ago
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
Assignee

Updated

2 years ago
Blocks: 1381573
Assignee

Updated

2 years ago
No longer blocks: 1381573

Comment 11

2 years ago
mozreview-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

::: 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 hidden (mozreview-request)
Assignee

Comment 13

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-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

::: 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 hidden (mozreview-request)
Assignee

Comment 16

2 years ago
mozreview-review-reply
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

2 years ago
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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ce634e4577e
Status: NEW → RESOLVED
Last Resolved: 2 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)
Assignee

Comment 20

2 years ago
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.
Assignee

Comment 21

2 years ago
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-

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.