1358336 - Updater reports that 52.0.2 must be installed even though running 53.0

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Kenneth Russell]

Attachment: Screen Shot 2017-04-20 at 6.58.32 PM.png

My Firefox installation on macOS is busted. Every time I launch the browser it prompts me to upgrade to 52.0.2, even though I have upgraded to 53.0. I have deleted the following directories:

/Applications/Firefox.app
~/Library/Application Support/Firefox
~/Library/Application Support/Mozilla
/Library/Application Support/Mozilla

and done a clean re-install of the application. It still prompts for updates every time.

Attached is a screenshot of the Update History preferences pane.

Where is this state stored so that I can delete it?

Comment 1

[Kelsey Gilbert [:jgilbert]]

[Tracking Requested - why for this release]:
Updater failure.

Comment 2

[Robert Strong (they/them - no direct email)]

Can you attach a screenshot of the prompt? Thanks!

Comment 3

[Kenneth Russell]

Attachment: Screen shot of update request dialog

Comment 4

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

I'm on 10.12.4 (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0) and I don't see this problem when I restart Firefox 53. 

Andrei can your team try to replicate this issue?

Comment 5

[Kenneth Russell]

I'm 99% sure I'm the only user seeing this.

Note that I hadn't launched Firefox in a long time -- possibly not at all during the duration the Firefox 52 release was live.

My main question is where on disk the updater's state is stored so I can manually clean it up. Afterward, I can confirm that this fixed the issue.

Comment 6

[Robert Strong (they/them - no direct email)]

I see what the problem is and this has existed for quite some time. It should be fairly easy to fix.

Kenneth, to resolve this issue on your system please remove the files under
~/Library/Caches/Mozilla/updates/<absolute path to Firefox install>

Comment 7

[Kenneth Russell]

Thanks Robert. Confirmed that moving aside that directory works around the problem.

Happy to try to test fixes by moving that directory back in place.

Comment 8

[Robert Strong (they/them - no direct email)]

I already have tests for this so local testing shouldn't be necessary.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: client patch rev1

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: Test patch rev1

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8be3de57f7ed1879e51a698517d2c46aa00a045

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: test patch rev2

Missed an eslint error in the test

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 8862235 [details] [diff] [review]
client patch rev1

Linux tests are passing... requesting review before the rest finish.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 8862235 [details] [diff] [review]
client patch rev1

Going to add a couple of more checks

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Since these are likely to be uplifted I added some checks to be more conservative.

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: test patch rev1

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4cb90d32c4483ce6e566e426597ff36945e0a86

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 8862265 [details] [diff] [review]
patch rev1

Matt, I think this first hunk should also check for STATE_PENDING_ELEVATE

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

I went ahead and added STATE_PENDING_ELEVATE as well.

Comment 18

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8862287 [details] [diff] [review]
patch rev1

Review of attachment 8862287 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1849,5 @@
> +         status == STATE_APPLIED || status == STATE_APPLIED_SERVICE ||
> +         status == STATE_PENDING_ELEVATE)) {
> +      if (Services.vc.compare(update.appVersion, Services.appinfo.version) < 0 ||
> +          Services.vc.compare(update.appVersion, Services.appinfo.version) == 0 &&
> +          update.buildID == Services.appinfo.appBuildID) {

Shouldn't this be <= for the buildID?

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to Matt Howell [:mhowell] from comment #18)
> Comment on attachment 8862287 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8862287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1849,5 @@
> > +         status == STATE_APPLIED || status == STATE_APPLIED_SERVICE ||
> > +         status == STATE_PENDING_ELEVATE)) {
> > +      if (Services.vc.compare(update.appVersion, Services.appinfo.version) < 0 ||
> > +          Services.vc.compare(update.appVersion, Services.appinfo.version) == 0 &&
> > +          update.buildID == Services.appinfo.appBuildID) {
> 
> Shouldn't this be <= for the buildID?
The format of the Build ID has changed in the past (iirc without it being communicated prior to the change) and there has been discussion as recently as last week of changing it again for dev edition. Because of this I think the risk of a breaking change abandoning clients isn't worth the value it might provide so I only use it for equality checks.

Comment 20

[Molly Howell (she/her) (no longer active)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #19)
> The format of the Build ID has changed in the past (iirc without it being
> communicated prior to the change) and there has been discussion as recently
> as last week of changing it again for dev edition. Because of this I think
> the risk of a breaking change abandoning clients isn't worth the value it
> might provide so I only use it for equality checks.

Okay, fair enough.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9df81790143
Client code - Bug 1358336 - app update tries to install staged updates for older versions. r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6d94e3cabd
Test code - Bug 1358336 - app update tries to install staged updates for older versions. r=mhowell

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e48a8ed30e7
followup test only fix to handle the different staging location on OS X. r=me

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: Followup test only fix for OS X

The test needed to account for the different staging location on OS X.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d9df81790143
https://hg.mozilla.org/mozilla-central/rev/2c6d94e3cabd
https://hg.mozilla.org/mozilla-central/rev/8e48a8ed30e7

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: client patch for beta - rstrong will land this with other patches

Approval Request Comment
[Feature/Bug causing the regression]:
Update staging
[User impact if declined]:
They can end up in a state where they are repeatedly prompted to update if they have an update stage and they manually install a newer version.
[Is this code covered by automated tests?]:
Yes
[Has the fix been verified in Nightly?]:
I have verified it on Nightly
[Needs manual test from QE? If yes, steps to reproduce]: 
No
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
Not very much.
[Why is the change risky/not risky?]:
It uses the same checks as used for regular updates which has been around a very long time for staged updates.
[String changes made/needed]:
None

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: test patch for beta

This also needs the test changes from bug 1354850

Comment 27

[Robert Strong (they/them - no direct email)]

I have also built beta with this patch and all update tests pass.

Comment 28

[Robert Strong (they/them - no direct email)]

Attachment: client patch for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Updates can be broken in a state where they are unable to update.
User impact if declined: 
They can end up in a state where they are repeatedly prompted to update if they have an update stage and they manually install a newer version.
[Is this code covered by automated tests?]:
Yes
Fix Landed on Version:
55
Risk to taking this patch (and alternatives if risky): 
Not very much. It uses the same checks as used for regular updates which has been around a very long time for staged updates.
String or UUID changes made by this patch: 
None

I have also built esr52 with this patch and all update tests pass.

Comment 29

[Robert Strong (they/them - no direct email)]

Attachment: test patch for esr52

This also needs the test changes from bug 1354850

Comment 30

[Gerry Chang [:gchang]]

Comment on attachment 8863552 [details] [diff] [review]
client patch for beta - rstrong will land this with other patches

Fix a repeatedly prompted to update issue. Beta54+. Should be in 54 beta 5.

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 8863552 [details] [diff] [review]
client patch for beta - rstrong will land this with other patches

I will land this with other patches

Comment 32

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/3b6617c56ac3c4a8bc786867c0f4c9dea39cc40d
https://hg.mozilla.org/releases/mozilla-beta/rev/4969ec22e2b513ece3d0d0cc68d20f31c4a8303f

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8863554 [details] [diff] [review]
client patch for esr52

Fixes a problem in the update logic, a core scenario for us, ESR52.2+

Comment 34

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-esr52
https://hg.mozilla.org/releases/mozilla-esr52/rev/aab0d0823210a157941c945a5234ccbf0dd7898c
https://hg.mozilla.org/releases/mozilla-esr52/rev/6c8b3865e0baa862a9c6d9207eaf3280418c226e

Comment 35

[Andrei Vaida [:avaida]]

(In reply to Robert Strong PTO 5/5 [:rstrong] (use needinfo to contact me) from comment #25)
> [Is this code covered by automated tests?]:
> Yes
> [Has the fix been verified in Nightly?]:
> I have verified it on Nightly
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No

Setting qe-verify- based on Robert's assessment on manual testing needs and the fact that this fix has automated coverage.