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.

Differential Revision: https://phabricator.services.mozilla.com/D33702

--HG--
extra : rebase_source : bb506c4ba4d75a68976bb114015d53cd41b4d4c3

toolkit/xre/nsAppRunner.cpp

@@ -2361,7 +2361,11 @@ static void ExtractCompatVersionInfo(const nsACString& aCompatVersion,
   aPlatformBuildID = Substring(aCompatVersion, slashPos + 1);
 }
 
-static bool IsNewIDLower(nsACString& oldID, nsACString& newID) {
+/**
+ * Compares two build IDs. Returns 0 if they match, < 0 if newID is considered
+ * newer than oldID and > 0 if the oldID is considered newer than newID.
+ */
+static int32_t CompareBuildIDs(nsACString& oldID, nsACString& newID) {
   // For Mozilla builds the build ID is a numeric date string. But it is too
   // large a number for the version comparator to handle so try to just compare
   // them as integer values first.
@@ -2393,16 +2397,22 @@ static bool IsNewIDLower(nsACString& oldID, nsACString& newID) {
 
   if (isNumeric) {
     nsresult rv;
-    uint64_t oldVal;
-    uint64_t newVal;
-    oldVal = oldID.ToInteger64(&rv);
+    CheckedInt<uint64_t> oldVal = oldID.ToInteger64(&rv);
 
-    if (NS_SUCCEEDED(rv)) {
-      newVal = newID.ToInteger64(&rv);
+    if (NS_SUCCEEDED(rv) && oldVal.isValid()) {
+      CheckedInt<uint64_t> newVal = newID.ToInteger64(&rv);
 
-      if (NS_SUCCEEDED(rv)) {
+      if (NS_SUCCEEDED(rv) && newVal.isValid()) {
         // We have simple numbers for both IDs.
-        return newVal < oldVal;
+        if (oldVal.value() == newVal.value()) {
+          return 0;
+        }
+
+        if (oldVal.value() > newVal.value()) {
+          return 1;
+        }
+
+        return -1;
       }
     }
   }
@@ -2411,8 +2421,8 @@ static bool IsNewIDLower(nsACString& oldID, nsACString& newID) {
   // distribution could have modified the build ID in some way. We don't know
   // what format this may be so let's just fall back to assuming that it's a
   // valid toolkit version.
-  return Version(PromiseFlatCString(newID).get()) <
-         Version(PromiseFlatCString(oldID).get());
+  return CompareVersions(PromiseFlatCString(oldID).get(),
+                         PromiseFlatCString(newID).get());
 }
 
 /**
@@ -2421,13 +2431,11 @@ static bool IsNewIDLower(nsACString& oldID, nsACString& newID) {
  * The aDowngrade parameter is set to true if the old version is "newer" than
  * the new version.
  */
-bool CheckCompatVersions(const nsACString& aOldCompatVersion,
-                         const nsACString& aNewCompatVersion,
-                         bool* aIsDowngrade) {
+int32_t CompareCompatVersions(const nsACString& aOldCompatVersion,
+                              const nsACString& aNewCompatVersion) {
   // Quick path for the common case.
   if (aOldCompatVersion.Equals(aNewCompatVersion)) {
-    *aIsDowngrade = false;
-    return true;
+    return 0;
   }
 
   // The versions differ for some reason so we will only ever return false from
@@ -2437,8 +2445,7 @@ bool CheckCompatVersions(const nsACString& aOldCompatVersion,
   // cannot tell if this is a downgrade or not so just assume it isn't and let
   // the user proceed.
   if (aOldCompatVersion.EqualsLiteral("Safe Mode")) {
-    *aIsDowngrade = false;
-    return false;
+    return -1;
   }
 
   nsCString oldVersion;
@@ -2454,24 +2461,18 @@ bool CheckCompatVersions(const nsACString& aOldCompatVersion,
                            newPlatformBuildID);
 
   // In most cases the app version will differ and this is an easy check.
-  if (Version(newVersion.get()) < Version(oldVersion.get())) {
-    *aIsDowngrade = true;
-    return false;
+  int32_t result = CompareVersions(oldVersion.get(), newVersion.get());
+  if (result != 0) {
+    return result;
   }
 
   // Fall back to build ID comparison.
-  if (IsNewIDLower(oldAppBuildID, newAppBuildID)) {
-    *aIsDowngrade = true;
-    return false;
+  result = CompareBuildIDs(oldAppBuildID, newAppBuildID);
+  if (result != 0) {
+    return result;
   }
 
-  if (IsNewIDLower(oldPlatformBuildID, newPlatformBuildID)) {
-    *aIsDowngrade = true;
-    return false;
-  }
-
-  *aIsDowngrade = false;
-  return false;
+  return CompareBuildIDs(oldPlatformBuildID, newPlatformBuildID);
 }
 
 /**
@@ -2505,7 +2506,9 @@ static bool CheckCompatibility(nsIFile* aProfileDir, const nsCString& aVersion,
     return false;
   }
 
-  if (!CheckCompatVersions(aLastVersion, aVersion, aIsDowngrade)) {
+  int32_t result = CompareCompatVersions(aLastVersion, aVersion);
+  if (result != 0) {
+    *aIsDowngrade = result > 0;
     return false;
   }
 

toolkit/xre/nsAppRunner.h

@@ -60,9 +60,14 @@ nsresult AppInfoConstructor(nsISupports* aOuter, const nsID& aIID,
 // Exported for gtests.
 void BuildCompatVersion(const char* aAppVersion, const char* aAppBuildID,
                         const char* aToolkitBuildID, nsACString& aBuf);
-bool CheckCompatVersions(const nsACString& aOldCompatVersion,
-                         const nsACString& aNewCompatVersion,
-                         bool* aIsDowngrade);
+
+/**
+ * Compares the provided compatibility versions. Returns 0 if they match,
+ * < 0 if the new version is considered an upgrade from the old version and
+ * > 0 if the new version is considered a downgrade from the old version.
+ */
+int32_t CompareCompatVersions(const nsACString& aOldCompatVersion,
+                              const nsACString& aNewCompatVersion);
 
 /**
  * Create the nativeappsupport implementation.

toolkit/xre/test/gtest/TestCompatVersionCompare.cpp

@@ -12,11 +12,10 @@ void CheckCompatVersionCompare(const nsCString& aOldCompatVersion,
                                bool aExpectedSame, bool aExpectedDowngrade) {
   printf("Comparing '%s' to '%s'.\n", aOldCompatVersion.get(), aNewCompatVersion.get());
 
-  bool isDowngrade = false;
-  bool isSame = CheckCompatVersions(aOldCompatVersion, aNewCompatVersion, &isDowngrade);
+  int32_t result = CompareCompatVersions(aOldCompatVersion, aNewCompatVersion);
 
-  ASSERT_EQ(aExpectedSame, isSame) << "Version sameness check should match.";
-  ASSERT_EQ(aExpectedDowngrade, isDowngrade) << "Version downgrade check should match.";
+  ASSERT_EQ(aExpectedSame, result == 0) << "Version sameness check should match.";
+  ASSERT_EQ(aExpectedDowngrade, result > 0) << "Version downgrade check should match.";
 }
 
 void CheckExpectedResult(
@@ -34,6 +33,7 @@ void CheckExpectedResult(
                             aExpectedSame, aExpectedDowngrade);
 }
 
+// clang-format off
 TEST(CompatVersionCompare, CompareVersionChange) {
   // Identical
   CheckExpectedResult(
@@ -124,13 +124,32 @@ TEST(CompatVersionCompare, CompareVersionChange) {
     "67.0.5", "20190523030228","20190523030228",
     false, false);
   CheckExpectedResult(
-    "67.0.5", "20190523030228","20190523030228",
+    "67.0.5", "20190523030228", "20190523030228",
     "67.0",   "20190516215225", "20190516215225",
     false, true);
 
+  // A newer or equal version should not go on to test the build IDs (bug 1556612).
+  CheckExpectedResult(
+    "65.0",   "30000000000000", "20000000000000",
+    "66.0",   "20000000000000", "20000000000000",
+    false, false);
+  CheckExpectedResult(
+    "65.0",   "20000000000000", "30000000000000",
+    "66.0",   "20000000000000", "20000000000000",
+    false, false);
+  CheckExpectedResult(
+    "66.0",   "30000000000000", "20000000000000",
+    "65.0",   "20000000000000", "20000000000000",
+    false, true);
+  CheckExpectedResult(
+    "66.0",   "20000000000000", "30000000000000",
+    "65.0",   "20000000000000", "20000000000000",
+    false, true);
+
   // Check that if the last run was safe mode then we consider this an upgrade.
   CheckCompatVersionCompare(
     NS_LITERAL_CSTRING("Safe Mode"),
     NS_LITERAL_CSTRING("67.0.1_20000000000000/20000000000000"),
     false, false);
 }
+// clang-format on

xpcom/base/nsVersionComparator.h

@@ -40,9 +40,18 @@
 
 namespace mozilla {
 
+/**
+ * Compares the version strings provided.
+ *
+ * Returns 0 if the versions match, < 0 if aStrB > aStrA and > 0 if
+ * aStrA > aStrB.
+ */
 int32_t CompareVersions(const char* aStrA, const char* aStrB);
 
 #ifdef XP_WIN
+/**
+ * As above but for wide character strings.
+ */
 int32_t CompareVersions(const char16_t* aStrA, const char16_t* aStrB);
 #endif