Updater reports that 52.0.2 must be installed even though running 53.0

RESOLVED FIXED in Firefox -esr52

Status

()

Toolkit
Application Update
--
blocker
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Kenneth Russell, Assigned: rstrong)

Tracking

53 Branch
mozilla55
x86_64
Mac OS X
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr5254+ fixed, firefox53+ wontfix, firefox54 fixed, firefox55 fixed)

Details

Attachments

(9 attachments, 4 obsolete attachments)

68.55 KB, image/png
Details
94.26 KB, image/png
Details
7.26 KB, patch
mhowell
: review+
Details | Diff | Splinter Review
3.94 KB, patch
mhowell
: review+
Details | Diff | Splinter Review
1.38 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
4.27 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
7.28 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
4.27 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
7.56 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 months ago
Created attachment 8860243 [details]
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?
[Tracking Requested - why for this release]:
Updater failure.
Severity: major → blocker
tracking-firefox53: --- → ?

Updated

6 months ago
Component: Installer → Application Update
Product: Firefox → Toolkit
Can you attach a screenshot of the prompt? Thanks!
Flags: needinfo?(kbrussel)
(Reporter)

Comment 3

6 months ago
Created attachment 8860464 [details]
Screen shot of update request dialog
Flags: needinfo?(kbrussel)
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?
status-firefox53: --- → affected
tracking-firefox53: ? → +
Flags: needinfo?(andrei.vaida)
(Reporter)

Comment 5

6 months ago
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.
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>
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(andrei.vaida)
(Reporter)

Comment 7

6 months ago
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.
I already have tests for this so local testing shouldn't be necessary.
status-firefox54: --- → affected
status-firefox55: --- → affected
Created attachment 8862235 [details] [diff] [review]
client patch rev1
Created attachment 8862236 [details] [diff] [review]
Test patch rev1

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8be3de57f7ed1879e51a698517d2c46aa00a045
Created attachment 8862237 [details] [diff] [review]
test patch rev2

Missed an eslint error in the test
Attachment #8862236 - Attachment is obsolete: true
Comment on attachment 8862235 [details] [diff] [review]
client patch rev1

Linux tests are passing... requesting review before the rest finish.
Attachment #8862235 - Flags: review?(mhowell)
Attachment #8862237 - Flags: review?(mhowell)
Comment on attachment 8862235 [details] [diff] [review]
client patch rev1

Going to add a couple of more checks
Attachment #8862235 - Attachment is obsolete: true
Attachment #8862235 - Flags: review?(mhowell)
Created attachment 8862265 [details] [diff] [review]
patch rev1

Since these are likely to be uplifted I added some checks to be more conservative.
Attachment #8862237 - Attachment is obsolete: true
Attachment #8862237 - Flags: review?(mhowell)
Created attachment 8862266 [details] [diff] [review]
test patch rev1

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4cb90d32c4483ce6e566e426597ff36945e0a86
Comment on attachment 8862265 [details] [diff] [review]
patch rev1

Matt, I think this first hunk should also check for STATE_PENDING_ELEVATE
Created attachment 8862287 [details] [diff] [review]
patch rev1

I went ahead and added STATE_PENDING_ELEVATE as well.
Attachment #8862265 - Attachment is obsolete: true
Attachment #8862287 - Flags: review?(mhowell)
Attachment #8862266 - Flags: review?(mhowell)
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?
Attachment #8862287 - Flags: review?(mhowell) → review+

Updated

6 months ago
Attachment #8862266 - Flags: review?(mhowell) → review+
(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.
(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

6 months ago
Pushed by rstrong@mozilla.com:
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

6 months ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e48a8ed30e7
followup test only fix to handle the different staging location on OS X. r=me
Created attachment 8863082 [details] [diff] [review]
Followup test only fix for OS X

The test needed to account for the different staging location on OS X.
Attachment #8863082 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d9df81790143
https://hg.mozilla.org/mozilla-central/rev/2c6d94e3cabd
https://hg.mozilla.org/mozilla-central/rev/8e48a8ed30e7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: in-testsuite+
Created attachment 8863552 [details] [diff] [review]
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
Attachment #8863552 - Flags: review+
Attachment #8863552 - Flags: approval-mozilla-beta?
Created attachment 8863553 [details] [diff] [review]
test patch for beta

This also needs the test changes from bug 1354850
Attachment #8863553 - Flags: review+
I have also built beta with this patch and all update tests pass.
Created attachment 8863554 [details] [diff] [review]
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.
Attachment #8863554 - Flags: review+
Attachment #8863554 - Flags: approval-mozilla-esr52?
Created attachment 8863555 [details] [diff] [review]
test patch for esr52

This also needs the test changes from bug 1354850
Attachment #8863555 - Flags: review+
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → ?
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.
Attachment #8863552 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Attachment #8863552 - Attachment description: client patch for beta → client patch for beta - rstrong will land this with other patches
status-firefox53: affected → wontfix
Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/3b6617c56ac3c4a8bc786867c0f4c9dea39cc40d
https://hg.mozilla.org/releases/mozilla-beta/rev/4969ec22e2b513ece3d0d0cc68d20f31c4a8303f
status-firefox54: affected → fixed

Comment 33

6 months ago
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+
Attachment #8863554 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Updated

6 months ago
tracking-firefox-esr52: ? → 54+
Pushed to mozilla-esr52
https://hg.mozilla.org/releases/mozilla-esr52/rev/aab0d0823210a157941c945a5234ccbf0dd7898c
https://hg.mozilla.org/releases/mozilla-esr52/rev/6c8b3865e0baa862a9c6d9207eaf3280418c226e
status-firefox-esr52: affected → fixed
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.