1556832 - Version downgrade check doesn't bail out when it has found an upgrade.

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Dave Townsend [:mossop]]

Rather than returning early when we know one part indicates an upgrade we continue checking later parts. This means that even if the main version is newer we go on to check the build IDs and if they are older we consider it a downgrade.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1556832: Comparing compatibility versions goes on to check the build IDs when the version is newer. r=froydnj

When comparing compatibility versions we first compare the version part. If that
shows us to be a downgrade then we stop checking at that point. But we must also
stop checking if it shows us to be an upgrade since we don't need to check the
build IDs at that point. This also applies to the check for the app build ID.

This means that a newer version with an older build id will appear to be a
downgrade.

This is problematic for safe mode because when safe mode runs it sets the
compatibility version to "Safe Mode" (bug 1556831) to ensure that caches are
invalidated on next startup. On the next run the Firefox version is considered
as newer than "Safe Mode" so we would move on to comparing the build IDs. But
the Firefox build ID gets version compared to "" (since there isn't a build ID
part in "Safe Mode"). Since build ID's are larger than 32-bit numbers we trigger
bug 1556829 and the actual comparison depends on the value of the build ID
truncated to 32-bits. This seems to often be negative and so lower than the
apparent previous build ID causing us to think this is a downgrade.

Cue nfroydnj saying I told you so because if I'd written this as a more
traditional compare function as he suggested I probably would have caught this.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

We may want to keep this on the radar in the event of another 67 dot release.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ccf46823323
Comparing compatibility versions goes on to check the build IDs when the version is newer. r=froydnj

Comment 4

[Daniel Varga [:dvarga]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta (and Release if you feel it appropriate) approval when you get a chance.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 9069770 [details]
Bug 1556832: Comparing compatibility versions goes on to check the build IDs when the version is newer. r=froydnj

Beta/Release Uplift Approval Request

• User impact if declined: In some cases (most likely linux distro builds) the user may see a profile downgrade when there wasn't one.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This does make changes to the profile compatibility comparison code which in the worst case can deny a user access to their profile, but it does have good automated tests.
• String changes made/needed: None

Comment 7

[Julien Cristau [:jcristau]]

Attachment: Bug 1556832 (followup): update comment for CompareCompatVersions. r?mossop

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9069770 [details]
Bug 1556832: Comparing compatibility versions goes on to check the build IDs when the version is newer. r=froydnj

fix version comparisons, approved for 68.0b10

Comment 9

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/06b935ea69b8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ca2ee4eebf7
(followup): update comment for CompareCompatVersions. r=mossop

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8ca2ee4eebf7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

We're not planning another 67 dot release at this point. Probably best to let this ride with 68 regardless.