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

Status: VERIFIED FIXED

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

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Kris Maglione [:kmag]]

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.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 5

[Robert Helmer [:rhelmer]]

This is hampering e10s rollout in bug 1314429 (need to honor a new property for already-installed add-ons), taking.

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: Bug 1272446 - rebuild addons DB from manifests when schema has changed

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

Comment 7

[Kris Maglione [:kmag]]

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.

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

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

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

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

Comment 10

[Kris Maglione [:kmag]]

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?

Comment 11

[Robert Helmer [:rhelmer]]

(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 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

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

Comment 13

[Kris Maglione [:kmag]]

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 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94806/diff/4-5/

Comment 15

[Kris Maglione [:kmag]]

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 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8813401 [details]
Bug 1272446 - rebuild addons DB from manifests when schema has changed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94806/diff/5-6/

Comment 17

[Robert Helmer [:rhelmer]]

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.

Comment 18

[Robert Helmer [:rhelmer]]

(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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ea3ad1ce937
rebuild addons DB from manifests when schema has changed r=kmag

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea3ad1ce937

Comment 21

[Kris Maglione [:kmag]]

\o/

Comment 22

[Andy McKay]

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.

Comment 23

[Robert Helmer [:rhelmer]]

Attachment: approval request

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

Comment 24

[Gerry Chang [:gchang]]

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.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c97b8ceefee

Comment 26

[Carsten Book [:Tomcat]]

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')

Comment 27

[Carsten Book [:Tomcat]]

backed out from aurora for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4320678&repo=mozilla-aurora

Comment 28

[Robert Helmer [:rhelmer]]

Attachment: Patch for beta

Patch for beta

Comment 29

[Robert Helmer [:rhelmer]]

Attachment: Patch for aurora

Comment 30

[Robert Helmer [:rhelmer]]

Attachment: Patch for beta

Comment 31

[Robert Helmer [:rhelmer]]

Attachment: Patch for aurora

Comment 32

[Robert Helmer [:rhelmer]]

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5458331f5a1

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af2cb10a3309

Comment 35

[Carsten Book [:Tomcat]]

np Rob :) seems Ryan took over checkin honors :)

Comment 36

[Vasilica Tamas, QA [:vtamas]]

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?

Comment 37

[Andy McKay]

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.

Comment 38

[Vasilica Tamas, QA [:vtamas]]

(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?

Comment 39

[Andy McKay]

(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.

Comment 40

[Mark Hammond [:markh] [:mhammond]]

(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?

Comment 41

[Vasilica Tamas, QA [:vtamas]]

(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.

Comment 42

[Robert Helmer [:rhelmer]]

(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.

Comment 43

[Robert Helmer [:rhelmer]]

Filed bug 1323594 specifically about recovering from a missing or corrupt addons DB.