Closed Bug 1272446 Opened 6 years ago Closed 6 years ago

The add-ons DB should be rebuilt when launching a profile with a new Firefox version with an updated addon DB schema

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: kmag, Assigned: rhelmer)

References

Details

(Whiteboard: triaged)

Attachments

(4 files, 2 obsolete files)

Different versions of Firefox process add-on metadata differently, and often in ways unsupported by other versions of Firefox. When loading a profile with a different version of Firefox than it last launched with, we should rebuild the add-on DB to make sure it has the expected metadata for the current version.
Component: WebExtensions → Add-ons Manager
Depends on: 1216243
Priority: -- → P3
Whiteboard: triaged
This is definitely higher than P3. It prevents us from making changes to parts of the manifest that are stored in the add-ons DB without breaking them on upgrade, which is a pretty big deal.
Priority: P3 → --
Whiteboard: triaged
Priority: -- → P2
Whiteboard: triaged
Blocks: 1273611
See also bug 1285866 (and indirectly, bug 1286785) - if for some reason Firefox starts with a missing or corrupt database, it should also be rebuilt at that time. Depending on the implementation of this bug it may actually come for free, but it's still worth pointing out.
Duplicate of this bug: 1291545
Duplicate of this bug: 1292683
Assignee: nobody → kmaglione+bmo
This is hampering e10s rollout in bug 1314429 (need to honor a new property for already-installed add-ons), taking.
Assignee: kmaglione+bmo → rhelmer
Blocks: 1314429
Status: NEW → ASSIGNED
Summary: The add-ons DB should be rebuilt when launching a profile with a new Firefox version → The add-ons DB should be rebuilt when launching a profile with a new Firefox version or addon DB schema
Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

https://reviewboard.mozilla.org/r/94806/#review95052

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3825
(Diff revision 1)
>          try {
>            extensionListChanged = XPIDatabaseReconcile.processFileChanges(manifests,
>                                                                           aAppChanged,
>                                                                           aOldAppVersion,
> -                                                                         aOldPlatformVersion);
> +                                                                         aOldPlatformVersion,
> +                                                                         true);

Shouldn't this be something like `updateReasons.includes("schemaChanged")`? Otherwise, we'll wind up re-reading all manifests for a single add-on being changed.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1945
(Diff revision 1)
>              }
>  
>              // The add-on has changed if the modification time has changed, or
>              // we have an updated manifest for it. Also reload the metadata for
>              // add-ons in the application directory when the application version
>              // has changed

Maybe update this comment?

::: toolkit/mozapps/extensions/test/xpcshell/test_schema_change.js:7
(Diff revision 1)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +BootstrapMonitor.init();
> +
> +const PREF_DB_SCHEMA                  = "extensions.databaseSchema";

Nit: Gratuitous whitespace...

::: toolkit/mozapps/extensions/test/xpcshell/test_schema_change.js:59
(Diff revision 1)
> +  // Make sure the timestamp is unchanged, so it is not re-scanned for that reason.
> +  let timestamp = file.lastModifiedTime;
> +  xpiFile.moveTo(profileDir, `${ID}.xpi`);
> +
> +  file.lastModifiedTime = timestamp;

Can we maybe restart once after installing the file but before changing the schema version, and checking that the metadata isn't updated, then bump it and check that it is?

Oh. I see. Never mind, then.
Attachment #8813401 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

https://reviewboard.mozilla.org/r/94806/#review95108

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1950
(Diff revisions 1 - 3)
> -            // has changed
> +            // or schema version has changed.
>              let newAddon = loadedManifest(installLocation, id);
>              if (newAddon || oldAddon.updateDate != xpiState.mtime ||
>                  (aUpdateCompatibility && (installLocation.name == KEY_APP_GLOBAL ||
>                                            installLocation.name == KEY_APP_SYSTEM_DEFAULTS)) ||
> -                (aSchemaChange)) {
> +                (aSchemaChange && !aUpdateCompatibility)) {

Hm. Won't this prevent us from re-reading the metadata when the schema changes as the result of an app update?
(In reply to Kris Maglione [:kmag] from comment #10)
> Comment on attachment 8813401 [details]
> Bug 1272446 - rebuild addons DB from manifests when schema has changed
> 
> https://reviewboard.mozilla.org/r/94806/#review95108
> 
> ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1950
> (Diff revisions 1 - 3)
> > -            // has changed
> > +            // or schema version has changed.
> >              let newAddon = loadedManifest(installLocation, id);
> >              if (newAddon || oldAddon.updateDate != xpiState.mtime ||
> >                  (aUpdateCompatibility && (installLocation.name == KEY_APP_GLOBAL ||
> >                                            installLocation.name == KEY_APP_SYSTEM_DEFAULTS)) ||
> > -                (aSchemaChange)) {
> > +                (aSchemaChange && !aUpdateCompatibility)) {
> 
> Hm. Won't this prevent us from re-reading the metadata when the schema
> changes as the result of an app update?

Right, thanks for catching that! I dug into this and I think the most straightforward way to do this would be to provide a way to optionally load metadata during the compatibility check, only if the schema version has been updated. As a bonus we don't need to add more on to this massive `if`, we can do a much more readable `else if (updateCompatibility || schemaChange)` branch.

I think we should do some refactoring to make reloading metadata from manifests less painful and there's a lot of similar code in here already (looks how many callers there are for `syncLoadManifestFromFile()` in `XPIProviderUtils` for instance), however as we discussed before I'd like to keep this patch small and do that in a follow-up bug.

Going to make sure it passes try this time before pushing to review.
Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

https://reviewboard.mozilla.org/r/94806/#review95314

I like this approach.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1845
(Diff revisions 3 - 4)
>      }
> +
> +    // May be updating from a version of the app that didn't support all the
> +    // properties of the currently-installed add-ons.
> +    if (aReloadMetadata) {
> +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);

Nit: Can we add a nsFile Components.Constructor? We do this a lot of times in this file...

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1849
(Diff revisions 3 - 4)
> +      manifest.syncGUID = aOldAddon.syncGUID;
> +      manifest.foreignInstall = aOldAddon.foreignInstall;
> +      manifest.visible = aOldAddon.visible;
> +      manifest.active = aOldAddon.active;
> +      manifest.userDisabled = aOldAddon.userDisabled;
> +      manifest.applyBackgroundUpdates = aOldAddon.applyBackgroundUpdates;
> +      manifest.sourceURI = aOldAddon.sourceURI;
> +      manifest.releaseNotesURI = aOldAddon.releaseNotesURI;
> +      manifest.targetApplications = aOldAddon.targetApplications;

Nit: Can we use a list of properties and a for-of loop?

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1857
(Diff revisions 3 - 4)
> +      manifest.active = aOldAddon.active;
> +      manifest.userDisabled = aOldAddon.userDisabled;
> +      manifest.applyBackgroundUpdates = aOldAddon.applyBackgroundUpdates;
> +      manifest.sourceURI = aOldAddon.sourceURI;
> +      manifest.releaseNotesURI = aOldAddon.releaseNotesURI;
> +      manifest.targetApplications = aOldAddon.targetApplications;

Hmmm... I bet there are cases where we'd actually want to re-parse targetApplications (in fact, I can think of one migration where it was necessary...)... But meh. Maybe just add a comment and we can deal with that later?

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1859
(Diff revisions 3 - 4)
> +      manifest.applyBackgroundUpdates = aOldAddon.applyBackgroundUpdates;
> +      manifest.sourceURI = aOldAddon.sourceURI;
> +      manifest.releaseNotesURI = aOldAddon.releaseNotesURI;
> +      manifest.targetApplications = aOldAddon.targetApplications;
> +
> +      copyProperties(manifest, PROP_JSON_FIELDS, aOldAddon);

Or maybe just pass a smaller list than `PROP_JSON_FIELDS` to `copyProperties`? That would make it a bit clearer what's actually happening.

::: toolkit/mozapps/extensions/test/xpcshell/test_schema_change.js:77
(Diff revisions 3 - 4)
>    addon.uninstall();
>    yield waitUninstall;
>  });
>  
>  /**
> + *  App update and a schema change causes a reload of the manifest.

Maybe also add a test for app update without schema change, just for the hell of it?
Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

https://reviewboard.mozilla.org/r/94806/#review95332

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1848
(Diff revisions 4 - 5)
>      }
>  
>      // May be updating from a version of the app that didn't support all the
>      // properties of the currently-installed add-ons.
>      if (aReloadMetadata) {
> -      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      let file = new nsIFile(aAddonState.descriptor);

Hm. Does this actually work when we pass it to initWithPath rather than assigning it to persistentDescriptor?
Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

https://reviewboard.mozilla.org/r/94806/#review95332

> Hm. Does this actually work when we pass it to initWithPath rather than assigning it to persistentDescriptor?

It does work, but I'll modify it for clarity.
(In reply to Mark Hammond [:markh] from comment #2)
> See also bug 1285866 (and indirectly, bug 1286785) - if for some reason
> Firefox starts with a missing or corrupt database, it should also be rebuilt
> at that time. Depending on the implementation of this bug it may actually
> come for free, but it's still worth pointing out.

It's possible that this patch will help, since we'll now rebuild the DB on every schema version bump. I expect that we'll be doing that soon for bug 1314429.

I'll follow up in those bugs though, there is preexisting code at least makes an attempt to handle corruption so it'd be ideal to fix or remove that if it's not working.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ea3ad1ce937
rebuild addons DB from manifests when schema has changed r=kmag
https://hg.mozilla.org/mozilla-central/rev/7ea3ad1ce937
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
\o/
We'll need to uplift this to 51 to allow bug 1314429 to land. Rob want to prepare some uplift patches... and thank you for working on this, a cause of many a bug.

Also Krupa, we should probably do some QA on this.
Flags: needinfo?(rhelmer)
Keywords: qawanted
Attached patch approval requestSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, feature that bug 1314429 is relying on
[User impact if declined]: This is blocking e10srollout for add-ons that have not explicitly opted out of multiprocess support.
[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]: bug 1314429
[Is the change risky?]: no
[Why is the change risky/not risky?]: this feature re-builds the add-ons DB only if the schema version is bumped in the code, it should do nothing new otherwise and has unit tests to demonstrate this.
[String changes made/needed]: none
Flags: needinfo?(rhelmer)
Attachment #8815018 - Flags: approval-mozilla-beta?
Attachment #8815018 - Flags: approval-mozilla-aurora?
Comment on attachment 8815018 [details] [diff] [review]
approval request

We need this patch for bug 1314429. Beta51+ and Aurora52+. Should be in 51 beta 5.
Attachment #8815018 - Flags: approval-mozilla-beta?
Attachment #8815018 - Flags: approval-mozilla-beta+
Attachment #8815018 - Flags: approval-mozilla-aurora?
Attachment #8815018 - Flags: approval-mozilla-aurora+
has problems to apply to beta:

grafting 377553:9c97b8ceefee "Bug 1272446 - rebuild addons DB from manifests when schema has changed r=kmag a=gchang"
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
merging toolkit/mozapps/extensions/internal/XPIProviderUtils.js
merging toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
warning: conflicts while merging toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(rhelmer)
Attached patch Patch for beta (obsolete) — Splinter Review
Patch for beta
Flags: needinfo?(rhelmer)
Attachment #8815325 - Attachment description: bug1272446-beta-2.diff → Patch for beta
Attachment #8815018 - Attachment description: Patch for mozilla-beta (existing patch should apply to -aurora cleanly) → approval request
Attached patch Patch for aurora (obsolete) — Splinter Review
Attached patch Patch for betaSplinter Review
Attachment #8815325 - Attachment is obsolete: true
Attached patch Patch for auroraSplinter Review
Attachment #8815328 - Attachment is obsolete: true
Sorry about that, there were a few problems with my uplift patch:

* patch did not include unit test file
* the unit test file uses newer test functions that don't yet exist on aurora/beta

Also I was wrong about not needing a separate aurora patch - I've made a separate one for beta and aurora now.
Flags: needinfo?(cbook)
np Rob :) seems Ryan took over checkin honors :)
Flags: needinfo?(cbook)
Performed a bit of manual testing around this issue focusing on the scenario described in Bug 1291545.

Confirm that Options section ( http://screencast.com/t/UqVIQ7T2EUGL ) is successfully displayed while upgrading from:
  - Firefox 48.0a1 (2016-04-01) to Firefox 53.0a1 (2016-12-11)
  - Firefox 48.0a2 (2016-05-04) to Firefox 52.0a2 (2016-12-11) 
  - Firefox 48.0b6 to Firefox 51.0b6
Tested under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1.


Is there any other way to manually verify this bug?
Flags: needinfo?(amckay)
Keywords: qawanted
Other scenarios include (and are hopefully covered) are:
* if the add-ons profile is completely corrupted e.g.: comment 2
* if the you downgrade a profile with a different schema and then upgrade back up to the latest schema

If the first one is covered with this patch, it should be easy to test. The second case I'm not so sure.
Flags: needinfo?(amckay)
(In reply to Andy McKay [:andym] from comment #37)
> Other scenarios include (and are hopefully covered) are:
> * if the add-ons profile is completely corrupted e.g.: comment 2

First scenario is verified and seems to work as expected across all 3 fixed versions (Nightly, Aurora and Beta) under Windows 10 64-bit and Ubuntu 16.04 32-bit. Tested following 2 methods:
 - using a corrupted addons.json
 - without an addons.json (was deleted). 

> * if the you downgrade a profile with a different schema and then upgrade
> back up to the latest schema

I’m not quite sure If I correctly understand the second scenario. Should I modify, for instance the description section from addons.json using Latest Nightly -> Downgrade to Firefox 48.0a1 -> Upgrade back to Latest Nightly -> the initial data should be displayed.  

Is this correct?
Flags: needinfo?(amckay)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #38)
> First scenario is verified and seems to work as expected across all 3 fixed
> versions (Nightly, Aurora and Beta) under Windows 10 64-bit and Ubuntu 16.04
> 32-bit. Tested following 2 methods:
>  - using a corrupted addons.json
>  - without an addons.json (was deleted). 

Not sure how to test the second one either. But if this one works, I would call it verified and don't worry about it.
Flags: needinfo?(amckay)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #38)
> (In reply to Andy McKay [:andym] from comment #37)
> > Other scenarios include (and are hopefully covered) are:
> > * if the add-ons profile is completely corrupted e.g.: comment 2
> 
> First scenario is verified and seems to work as expected across all 3 fixed
> versions (Nightly, Aurora and Beta) under Windows 10 64-bit and Ubuntu 16.04
> 32-bit. Tested following 2 methods:
>  - using a corrupted addons.json
>  - without an addons.json (was deleted). 

Comment 2 refers to bug 1285866, which seems to still exist on central - ie, if I delete addons.json then start the browser, I do see addons.json recreated (which was bug 1286785), but it does not list any addons (which is bug 1285866).

However, comment 18 implies bug 1285866 is *not* expected to be fixed in this bug, so ISTM that the steps above didn't actually verify anything related to this bug at all?
Flags: needinfo?(vasilica.mihasca)
(In reply to Mark Hammond [:markh] from comment #40)
> Comment 2 refers to bug 1285866, which seems to still exist on central - ie,
> if I delete addons.json then start the browser, I do see addons.json
> recreated (which was bug 1286785), but it does not list any addons (which is
> bug 1285866).
> 
> However, comment 18 implies bug 1285866 is *not* expected to be fixed in
> this bug, so ISTM that the steps above didn't actually verify anything
> related to this bug at all?

I tested following the cases mentioned in Bug 1286785 because only this one was closed. I will keep tracking of Bug 1285866 and also verify it as soon as will be fixed.  

(In reply to Andy McKay [:andym] from comment #39) 
> Not sure how to test the second one either. But if this one works, I would
> call it verified and don't worry about it.

Based on my testing and Andy's confirmation I am marking this bug as Verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(vasilica.mihasca)
(In reply to Mark Hammond [:markh] from comment #40)
> (In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #38)
> > (In reply to Andy McKay [:andym] from comment #37)
> > > Other scenarios include (and are hopefully covered) are:
> > > * if the add-ons profile is completely corrupted e.g.: comment 2
> > 
> > First scenario is verified and seems to work as expected across all 3 fixed
> > versions (Nightly, Aurora and Beta) under Windows 10 64-bit and Ubuntu 16.04
> > 32-bit. Tested following 2 methods:
> >  - using a corrupted addons.json
> >  - without an addons.json (was deleted). 
> 
> Comment 2 refers to bug 1285866, which seems to still exist on central - ie,
> if I delete addons.json then start the browser, I do see addons.json
> recreated (which was bug 1286785), but it does not list any addons (which is
> bug 1285866).
> 
> However, comment 18 implies bug 1285866 is *not* expected to be fixed in
> this bug, so ISTM that the steps above didn't actually verify anything
> related to this bug at all?

I agree - this fix should only apply when the schema version has been changed. I think the automated test coverage shows this pretty well. I am going to adjust the summary a little, since we decided to only rebuild on schema change, not on every Firefox app update (since this seems like an unnecessary startup perf hit)

It's certainly possible that the DB corruption recovery has been fixed incidentally in the meantime which is why the QA test is passing.

I'll file a followup bug specifically about recovering from a corrupt DB, if it's working now then we can at least add a unit test that will fail if this is ever not the case.
Summary: The add-ons DB should be rebuilt when launching a profile with a new Firefox version or addon DB schema → The add-ons DB should be rebuilt when launching a profile with a new Firefox version with an updated addon DB schema
Filed bug 1323594 specifically about recovering from a missing or corrupt addons DB.
You need to log in before you can comment on or make changes to this bug.