1439600 - Add-on not upgrading when moving to Firefox 59b9

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Kaply [:mkaply]]

The Ecosia Add-on is not properly migrating from a Legacy add-on to a WebExtension with Firefox 59b9.

To recreate.

Install Firefox 56.0.2.

Install version 3.4.5 of the Ecosia add-on from here:

https://addons.mozilla.org/en-US/firefox/addon/ecosia-the-green-search/versions/

Close Firefox.

Start Firefox 59b9 with the same profile.

Go to about:Addons.

Notice there is no Ecosia add-on.

Click on the "legacy" link.

Ecosia is in the legacy add-ons section.

It does not update until you right click and select "find-updates"

It should have updated at the same time as Firefox.

Comment 1

[Andrew Swan [:aswan]]

Does this happen in a clean profile?  In particular, if you have extension updates disabled, we don't do an automatic update in this case.

If it does, since you have an environment for testing already set up, can you flip the preference extensions.logging.enabled to true (before the upgrade) and then paste the logs from the startup of 59 here in an attachment?

Comment 2

[Mike Kaply [:mkaply]]

This is the log after startup:

https://pastebin.mozilla.org/9078241

It is a fresh profile.

Ecosia has the ID {d04b0b40-3dab-4f0b-97a6-04ec3eddbfb0}

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

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

Comment 4

[dominik.henter]

Thank you Mike for spotting this! It's really appreciated.

When Quantum was first introduced we did extensive testing for the upgrade case and everything worked fine so I think this probably is a regression introduced in a newer version.

Andrew, I'm confused by the proposed code change as on a first glance it looks unrelated. Or does this mean that our manifest has become incompatible? We do add some extra keys in there that make code-sharing between our Chrome and Firefox extension easier, is that related?

Comment 5

[Andrew Swan [:aswan]]

(In reply to dominik.henter from comment #4)
> When Quantum was first introduced we did extensive testing for the upgrade
> case and everything worked fine so I think this probably is a regression
> introduced in a newer version.

Yes, this regressed in 59.  For what its worth, this only affects users going directly from a version of 56 or earlier to a version of 59 or later.

> Andrew, I'm confused by the proposed code change as on a first glance it
> looks unrelated. Or does this mean that our manifest has become
> incompatible? We do add some extra keys in there that make code-sharing
> between our Chrome and Firefox extension easier, is that related?

That's code that runs at startup and is looking at the old extension.  It is supposed to look at the old extension, decide that it is incompatible, and check for an update which causes your webextension to get downloaded.  But the exception causes it to bail out and skip the update check.  But mkaply reports it isn't actually working, I need to look into it more closely.

Comment 6

[Mike Kaply [:mkaply]]

Would it make more sense to back out the inline options fix for now? Or do you think this is an easy fix?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8952516 [details]
Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

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

Comment 8

[Andrew Swan [:aswan]]

(In reply to Mike Kaply [:mkaply] from comment #6)
> Would it make more sense to back out the inline options fix for now? Or do
> you think this is an easy fix?

I'm still leaning toward easy fix.  I had a small goof in the first patch but I just updated and it seems to work for me.  Can you give it a try too?

Comment 9

[Andrew Swan [:aswan]]

Mossop, heads up that I intend to send r? for this patch your way.  But first, any thoughts on testing?  The triggering event here is that a manifest that parsed successfully in old versions no longer parses in new versions.  Within a single test we can't recreate that scenario.  We could change the manifest on disk between the shutdown and subsequent startup of the addons manager, but that sends us down a different code path.
The simple thing would be to just go with the manual testing that's already been done and call that enough.  What do you think?

Comment 10

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #9)
> Mossop, heads up that I intend to send r? for this patch your way.  But
> first, any thoughts on testing?  The triggering event here is that a
> manifest that parsed successfully in old versions no longer parses in new
> versions.  Within a single test we can't recreate that scenario.  We could
> change the manifest on disk between the shutdown and subsequent startup of
> the addons manager, but that sends us down a different code path.

Why do we go down a different code path? Is it because the timestamp changes? We can fix that!

Other alternatives I can think of include keeping a copy of a profile (or enough of the files from it that matter) that exhibit the behaviour and using that. Might be a pain though.

Comment 11

[Andrew Swan [:aswan]]

(In reply to Dave Townsend [:mossop] from comment #10)
> Why do we go down a different code path? Is it because the timestamp
> changes? We can fix that!

Yes.  You mean fix that by manually resetting the timestamp?  I thought about that but I'm worried that we're constructing something that is pretty different from the original problem.  I'm probably overthinking it though and this is what we should do.  I'll take a stab at the test now.

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8952516 [details]
Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

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

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8952516 [details]
Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

https://reviewboard.mozilla.org/r/221732/#review229642

Sorry for not getting to this sooner, thanks for the test!

Comment 14

[Mike Kaply [:mkaply]]

Attachment: Console log

So the replacement is no working (I get upgraded to the new Ecosia), but I still a bootstrap error:

1519765649848	addons.xpi	WARN	Error loading bootstrap.js for {d04b0b40-3dab-4f0b-97a6-04ec3eddbfb0}: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/mkaply/AppData/Roaming/Mozilla/Firefox/Profiles/2ixig8g1.eeeeee/extensions/%7Bd04b0b40-3dab-4f0b-97a6-04ec3eddbfb0%7D.xpi!/bootstrap.js :: <TOP_LEVEL> :: line 9"  data: no] Stack trace: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/mkaply/AppData/Roaming/Mozilla/Firefox/Profiles/2ixig8g1.eeeeee/extensions/%7Bd04b0b40-3dab-4f0b-97a6-04ec3eddbfb0%7D.xpi!/bootstrap.js:9 < loadBootstrapScope()@resource://gre/modules/addons/XPIProvider.jsm:4268 < callBootstrapMethod()@resource://gre/modules/addons/XPIProvider.jsm:4335 < startInstall/<()@resource://gre/modules/addons/XPIInstall.jsm:1790

Comment 15

[Andrew Swan [:aswan]]

The bootstrap error is related to trying to call uninstall() on the old extension and is safe to ignore.

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8952516 [details]
Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1414406

[User impact if declined]:
Users with legacy extensions that used inline settings who migrate directly from an old (<57) version of Firefox directly to 59 will not get updates to their legacy extension.

[Is this code covered by automated tests?]:
Yes, included in the patch

[Has the fix been verified in Nightly?]:
Yes, manually verified by aswan and by mkaply

[Needs manual test from QE? If yes, steps to reproduce]: 
If QE wants to do an additional pass, the STR are in comment 0

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
not particularly

[Why is the change risky/not risky?]:
This is a small and contained change.  It is in code that has extensive automated tests that should catch regressions and the specific problem in this bug is covered by a new automated test.

[String changes made/needed]:
none

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64dd755b940c
Mark addons as incompatible if we can no longer parse the manifest r=mossop

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/64dd755b940c

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8952516 [details]
Bug 1439600 Mark addons as incompatible if we can no longer parse the manifest

This sounds generally useful users to be able to upgrade their legacy extensions and also good to support Ecosia in particular.
Thanks for adding new tests.

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ebc3f8a7357

Comment 21

[marius.santa]

Attachment: ecosia upgrade.gif

Issue reproduced in Firefox 60.0a1 (20180201100326).
Retested and verified in Firefox 60.0a1 (20180305100134), Firefox 59.0b14 (20180301022608) on Windows 10 64Bit, Mac OS 10.13.2.