Bug 1554029: Downgrade detection fails for changes from non-minor to minor versions. r=froydnj a=RyanVM
authorDave Townsend <dtownsend@oxymoronical.com>
Fri, 24 May 2019 14:16:55 -0700
changeset 536528 89da9d90490a421de0dc706a17000fbfbb580455
parent 536527 e380696a22dd3cb1e71546c5c805c33128dd1238
child 536529 466ee338b43b6ab11c7a4b35da2ca28539faf77f
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, RyanVM
bugs1554029
milestone68.0
Bug 1554029: Downgrade detection fails for changes from non-minor to minor versions. r=froydnj a=RyanVM We build compatibility versions as `<appversion>_<appbuildid>/<platformbuildid>`. where the build ID's by default are a numeric representation of the date that the build happened. Previously we attempted to mangle this into a valid toolkit version by converting to `<appversion>.<appbuildid>.<platformbuildid>` if the build IDs were the expected length. We also split each build ID into two version parts since the version comparator couldn't handle their full size. This mangling fails when comparing a major version with a new patch version: `<majorversion>.<appbuildid>.<platformbuildid>` gets compared to `<majorversion>.1.<appbuildid>.<platformbuildid>`. Unless that patch version (1 here) was greater than the build ID (currently in the tens of millions) the patch release would appear to be older than the previous version. This changes this so rather than attempting to mangle the entire string into a single toolkit version we split out the string into their components. First we compare the app versions using the version comparator which is definitely the correct thing to do. For the build IDs we check if they are entirely numeric (which is the case for the date based default) and if so just compare them numerically. We assume that if the IDs aren't numeric then they are something defined by a custom build of Firefox, we can't know what form that takes so I've chosen to fallback to just using the version comparator since it will handle things such as `"build1" < "build10"` and it should be straightforward for anything more complex to choose a form that works. Differential Revision: https://phabricator.services.mozilla.com/D32552
toolkit/xre/moz.build
toolkit/xre/nsAppRunner.cpp
toolkit/xre/nsAppRunner.h
toolkit/xre/test/gtest/TestCompatVersionCompare.cpp
toolkit/xre/test/gtest/moz.build
--- a/toolkit/xre/moz.build
+++ b/toolkit/xre/moz.build
@@ -256,8 +256,11 @@ FINAL_TARGET_PP_FILES += [
 if CONFIG['CC_TYPE'] in ('clang', 'gcc'):
     CXXFLAGS += ['-Wno-error=shadow']
 
 if CONFIG['MOZ_IPDL_TESTS']:
     DEFINES['MOZ_IPDL_TESTS'] = True
 
 if CONFIG['MOZ_PROFILE_GENERATE']:
     DEFINES['MOZ_PROFILE_GENERATE'] = True
+
+if CONFIG['ENABLE_TESTS']:
+    DIRS += ['test/gtest']
--- a/toolkit/xre/nsAppRunner.cpp
+++ b/toolkit/xre/nsAppRunner.cpp
@@ -2329,72 +2329,150 @@ static ReturnAbortOnError CheckDowngrade
   }
 
   // Cancel
   return NS_ERROR_ABORT;
 }
 #endif
 
 /**
- * The version string used in compatibility.ini is in the form
- * <appversion>_<appbuildid>/<aplatformbuildidid>. The build IDs are in the form
- * <year><month><day><hour><minute><second>. We need to be able to turn this
- * into something that can be compared by the version comparator. Build IDs are
- * numeric so normally we could just use those as part of the version but they
- * are larger than 32-bit integers so we must split them into two parts.
- * So we generate a version string of the format:
- * <appversion>.<appbuilddate>.<appbuildtime>.<platformbuilddate>.<platformbuildtime>
- * This doesn't compare correctly so
- * we have to make the build ids separate version parts instead. We also have
- * to split up the build ids as they are too large for the version comparator's
- * brain.
+ * Extracts the various parts of a compatibility version string.
+ *
+ * Compatibility versions are of the form
+ * "<appversion>_<appbuildid>/<platformbuildid>". The toolkit version comparator
+ * can only handle 32-bit numbers and in the normal case build IDs are larger
+ * than this. So if the build ID is numeric we split it into two version parts.
  */
-
-// Build ID dates are 8 digits, build ID times are 6 digits.
-#define BUILDID_DATE_LENGTH 8
-#define BUILDID_TIME_LENGTH 6
-#define BUILDID_LENGTH BUILDID_DATE_LENGTH + BUILDID_TIME_LENGTH
-
-static void GetBuildIDVersions(const nsACString& aFullVersion, int32_t aPos,
-                               nsACString& aBuildVersions) {
-  // Extract the date part then the time part.
-  aBuildVersions.Assign(
-      Substring(aFullVersion, aPos, BUILDID_DATE_LENGTH) +
-      NS_LITERAL_CSTRING(".") +
-      Substring(aFullVersion, aPos + BUILDID_DATE_LENGTH, BUILDID_TIME_LENGTH));
-}
-
-static Version GetComparableVersion(const nsCString& aVersionStr) {
-  // We'll find the position of the '_' and '/' characters. The length from the
-  // '_' character to the '/' character will be the length of a build ID plus
-  // one for the '_' character. Similarly the length from the '/' character to
-  // the end of the string will be the same.
-  const uint32_t kExpectedLength = BUILDID_LENGTH + 1;
-
-  int32_t underscorePos = aVersionStr.FindChar('_');
-  int32_t slashPos = aVersionStr.FindChar('/');
+static void ExtractCompatVersionInfo(const nsACString& aCompatVersion,
+                                     nsACString& aAppVersion,
+                                     nsACString& aAppBuildID,
+                                     nsACString& aPlatformBuildID) {
+  int32_t underscorePos = aCompatVersion.FindChar('_');
+  int32_t slashPos = aCompatVersion.FindChar('/');
+
   if (underscorePos == kNotFound || slashPos == kNotFound ||
-      (slashPos - underscorePos) != kExpectedLength ||
-      (aVersionStr.Length() - slashPos) != kExpectedLength) {
+      slashPos < underscorePos) {
     NS_WARNING(
         "compatibility.ini Version string does not match the expected format.");
-    return Version(aVersionStr.get());
-  }
-
-  nsCString appBuild, platBuild;
-  NS_NAMED_LITERAL_CSTRING(dot, ".");
-
-  const nsACString& version = Substring(aVersionStr, 0, underscorePos);
-
-  GetBuildIDVersions(aVersionStr, underscorePos + 1, /* outparam */ appBuild);
-  GetBuildIDVersions(aVersionStr, slashPos + 1, /* outparam */ platBuild);
-
-  const nsACString& fullVersion = version + dot + appBuild + dot + platBuild;
-
-  return Version(PromiseFlatCString(fullVersion).get());
+
+    // Fall back to just using the entire string as the version.
+    aAppVersion = aCompatVersion;
+    aAppBuildID.Truncate(0);
+    aPlatformBuildID.Truncate(0);
+    return;
+  }
+
+  aAppVersion = Substring(aCompatVersion, 0, underscorePos);
+  aAppBuildID = Substring(aCompatVersion, underscorePos + 1, slashPos - (underscorePos + 1));
+  aPlatformBuildID = Substring(aCompatVersion, slashPos + 1);
+}
+
+static bool IsNewIDLower(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.
+
+  // ToInteger64 succeeds if the strings contain trailing non-digits so first
+  // check that all the characters are digits.
+  bool isNumeric = true;
+  const char* pos = oldID.BeginReading();
+  const char* end = oldID.EndReading();
+  while (pos != end) {
+    if (!IsAsciiDigit(*pos)) {
+      isNumeric = false;
+      break;
+    }
+    pos++;
+  }
+
+  if (isNumeric) {
+    pos = newID.BeginReading();
+    end = newID.EndReading();
+    while (pos != end) {
+      if (!IsAsciiDigit(*pos)) {
+        isNumeric = false;
+        break;
+      }
+      pos++;
+    }
+  }
+
+  if (isNumeric) {
+    nsresult rv;
+    uint64_t oldVal;
+    uint64_t newVal;
+    oldVal = oldID.ToInteger64(&rv);
+
+    if (NS_SUCCEEDED(rv)) {
+      newVal = newID.ToInteger64(&rv);
+
+      if (NS_SUCCEEDED(rv)) {
+        // We have simple numbers for both IDs.
+        return newVal < oldVal;
+      }
+    }
+  }
+
+  // If either could not be parsed as a number then something (likely a Linux
+  // 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());
+}
+
+/**
+ * Checks whether the compatibility versions from the previous and current
+ * application match. Returns true if there has been no change, false otherwise.
+ * 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) {
+  // Quick path for the common case.
+  if (aOldCompatVersion.Equals(aNewCompatVersion)) {
+    *aIsDowngrade = false;
+    return true;
+  }
+
+  // The versions differ for some reason so we will only ever return false from
+  // here onwards. We just have to figure out if this is a downgrade or not.
+
+  nsCString oldVersion;
+  nsCString oldAppBuildID;
+  nsCString oldPlatformBuildID;
+  ExtractCompatVersionInfo(aOldCompatVersion, oldVersion, oldAppBuildID,
+                           oldPlatformBuildID);
+
+  nsCString newVersion;
+  nsCString newAppBuildID;
+  nsCString newPlatformBuildID;
+  ExtractCompatVersionInfo(aNewCompatVersion, newVersion, newAppBuildID,
+                           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;
+  }
+
+  // Fall back to build ID comparison.
+  if (IsNewIDLower(oldAppBuildID, newAppBuildID)) {
+    *aIsDowngrade = true;
+    return false;
+  }
+
+  if (IsNewIDLower(oldPlatformBuildID, newPlatformBuildID)) {
+    *aIsDowngrade = true;
+    return false;
+  }
+
+  *aIsDowngrade = false;
+  return false;
 }
 
 /**
  * Checks the compatibility.ini file to see if we have updated our application
  * or otherwise invalidated our caches. If the application has been updated,
  * we return false; otherwise, we return true. We also write the status
  * of the caches (valid/invalid) into the return param aCachesOK. The aCachesOK
  * is always invalid if the application has been updated. aIsDowngrade is set to
@@ -2418,20 +2496,17 @@ static bool CheckCompatibility(nsIFile* 
   nsresult rv = parser.Init(file);
   if (NS_FAILED(rv)) return false;
 
   rv = parser.GetString("Compatibility", "LastVersion", aLastVersion);
   if (NS_FAILED(rv)) {
     return false;
   }
 
-  if (!aVersion.Equals(aLastVersion)) {
-    Version current = GetComparableVersion(aVersion);
-    Version last = GetComparableVersion(aLastVersion);
-    *aIsDowngrade = last > current;
+  if (!CheckCompatVersions(aLastVersion, aVersion, aIsDowngrade)) {
     return false;
   }
 
   nsAutoCString buf;
   rv = parser.GetString("Compatibility", "LastOSABI", buf);
   if (NS_FAILED(rv) || !aOSABI.Equals(buf)) return false;
 
   rv = parser.GetString("Compatibility", "LastPlatformDir", buf);
@@ -2470,22 +2545,28 @@ static bool CheckCompatibility(nsIFile* 
   if (aFlagFile) {
     aFlagFile->Exists(&purgeCaches);
   }
 
   *aCachesOK = !purgeCaches && *aCachesOK;
   return true;
 }
 
-static void BuildVersion(nsCString& aBuf) {
-  aBuf.Assign(gAppData->version);
+void BuildCompatVersion(const char* aAppVersion, const char* aAppBuildID,
+                        const char* aToolkitBuildID, nsACString& aBuf) {
+  aBuf.Assign(aAppVersion);
   aBuf.Append('_');
-  aBuf.Append(gAppData->buildID);
+  aBuf.Append(aAppBuildID);
   aBuf.Append('/');
-  aBuf.Append(gToolkitBuildID);
+  aBuf.Append(aToolkitBuildID);
+}
+
+static void BuildVersion(nsCString& aBuf) {
+  BuildCompatVersion(gAppData->version, gAppData->buildID, gToolkitBuildID,
+                     aBuf);
 }
 
 static void WriteVersion(nsIFile* aProfileDir, const nsCString& aVersion,
                          const nsCString& aOSABI, nsIFile* aXULRunnerDir,
                          nsIFile* aAppDir, bool invalidateCache) {
   nsCOMPtr<nsIFile> file;
   aProfileDir->Clone(getter_AddRefs(file));
   if (!file) return;
--- a/toolkit/xre/nsAppRunner.h
+++ b/toolkit/xre/nsAppRunner.h
@@ -52,16 +52,23 @@ extern nsString gAbsoluteArgv0Path;
 
 extern bool gIsGtest;
 
 namespace mozilla {
 nsresult AppInfoConstructor(nsISupports* aOuter, const nsID& aIID,
                             void** aResult);
 }  // namespace mozilla
 
+// 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);
+
 /**
  * Create the nativeappsupport implementation.
  *
  * @note XPCOMInit has not happened yet.
  */
 nsresult NS_CreateNativeAppSupport(nsINativeAppSupport** aResult);
 already_AddRefed<nsINativeAppSupport> NS_GetNativeAppSupport();
 
new file mode 100644
--- /dev/null
+++ b/toolkit/xre/test/gtest/TestCompatVersionCompare.cpp
@@ -0,0 +1,123 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+#include "gtest/gtest.h"
+#include "nsAppRunner.h"
+#include "nsString.h"
+
+void CheckExpectedResult(
+  const char* aOldAppVersion, const char* aOldAppID, const char* aOldToolkitID,
+  const char* aNewAppVersion, const char* aNewAppID, const char* aNewToolkitID,
+  bool aExpectedSame, bool aExpectedDowngrade) {
+
+  nsCString oldCompatVersion;
+  BuildCompatVersion(aOldAppVersion, aOldAppID, aOldToolkitID, oldCompatVersion);
+
+  nsCString newCompatVersion;
+  BuildCompatVersion(aNewAppVersion, aNewAppID, aNewToolkitID, newCompatVersion);
+
+  printf("Comparing '%s' to '%s'.\n", oldCompatVersion.get(), newCompatVersion.get());
+
+  bool isDowngrade = false;
+  bool isSame = CheckCompatVersions(oldCompatVersion, newCompatVersion, &isDowngrade);
+
+  ASSERT_EQ(aExpectedSame, isSame) << "Version sameness check should match.";
+  ASSERT_EQ(aExpectedDowngrade, isDowngrade) << "Version downgrade check should match.";
+}
+
+TEST(CompatVersionCompare, CompareVersionChange) {
+  // Identical
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000000",
+    "67.0",   "20000000000000", "20000000000000",
+    true, false);
+
+  // Build ID changes
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000001",
+    "67.0",   "20000000000000", "20000000000000",
+    false, true);
+  CheckExpectedResult(
+    "67.0",   "20000000000001", "20000000000000",
+    "67.0",   "20000000000000", "20000000000000",
+    false, true);
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000000",
+    "67.0",   "20000000000000", "20000000000001",
+    false, false);
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000000",
+    "67.0",   "20000000000001", "20000000000000",
+    false, false);
+
+  // Version changes
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000000",
+    "68.0",   "20000000000000", "20000000000000",
+    false, false);
+  CheckExpectedResult(
+    "68.0",   "20000000000000", "20000000000000",
+    "67.0",   "20000000000000", "20000000000000",
+    false, true);
+  CheckExpectedResult(
+    "67.0",   "20000000000000", "20000000000000",
+    "67.0.1", "20000000000000", "20000000000000",
+    false, false);
+  CheckExpectedResult(
+    "67.0.1", "20000000000000", "20000000000000",
+    "67.0",   "20000000000000", "20000000000000",
+    false, true);
+  CheckExpectedResult(
+    "67.0.1", "20000000000000", "20000000000000",
+    "67.0.1", "20000000000000", "20000000000000",
+    true, false);
+  CheckExpectedResult(
+    "67.0.1", "20000000000000", "20000000000000",
+    "67.0.2", "20000000000000", "20000000000000",
+    false, false);
+  CheckExpectedResult(
+    "67.0.2", "20000000000000", "20000000000000",
+    "67.0.1", "20000000000000", "20000000000000",
+    false, true);
+
+  // Unexpected ID formats
+  CheckExpectedResult(
+    "67.0.1", "build1", "build1",
+    "67.0.1", "build2", "build2",
+    false, false);
+  CheckExpectedResult(
+    "67.0.1", "build10", "build10",
+    "67.0.1", "build2", "build2",
+    false, true);
+  CheckExpectedResult(
+    "67.0.1", "1", "1",
+    "67.0.1", "10", "10",
+    false, false);
+  CheckExpectedResult(
+    "67.0.1", "10", "10",
+    "67.0.1", "1", "1",
+    false, true);
+
+  // These support an upgrade case from a normal-style build ID to the one
+  // we're suggesting Ubuntu use.
+  CheckExpectedResult(
+    "67.0.1", "20000000000000", "20000000000000",
+    "67.0.1", "1build1", "1build1",
+    false, false);
+  CheckExpectedResult(
+    "67.0.1", "1build1", "1build1",
+    "67.0.1", "20000000000000", "20000000000000",
+    false, true);
+
+  // The actual case from bug 1554029:
+  CheckExpectedResult(
+    "67.0",   "20190516215225", "20190516215225",
+    "67.0.5", "20190523030228","20190523030228",
+    false, false);
+  CheckExpectedResult(
+    "67.0.5", "20190523030228","20190523030228",
+    "67.0",   "20190516215225", "20190516215225",
+    false, true);
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/xre/test/gtest/moz.build
@@ -0,0 +1,13 @@
+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
+# vim: set filetype=python:
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, you can obtain one at http://mozilla.org/MPL/2.0/.
+
+Library('xretest')
+
+UNIFIED_SOURCES = [
+    'TestCompatVersionCompare.cpp',
+]
+
+FINAL_LIBRARY = 'xul-gtest'