Bug 1389344 - Avoid checking for distribution.ini existence on every startup. r=mkaply
☠☠ backed out by 8acec43298ab ☠ ☠
authorFelipe Gomes <felipc@gmail.com>
Tue, 19 Sep 2017 14:41:06 -0300
changeset 433851 b9d7cdb3e163e1b421f67c49a304368b786e9a62
parent 433850 4a9a6475bf7d0763049fba2a244a64a4e5af5ebd
child 433852 7b7658f89b02fac91203715f283c7bab3466e16d
push id8114
push userjlorenzo@mozilla.com
push dateThu, 02 Nov 2017 16:33:21 +0000
treeherdermozilla-beta@73e0d89a540f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkaply
bugs1389344
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1389344 - Avoid checking for distribution.ini existence on every startup. r=mkaply We can do this check only once after an update, and cache the value until the next app update. MozReview-Commit-ID: 1yxiKnHIAi2
browser/components/distribution.js
browser/components/tests/unit/test_distribution.js
browser/components/tests/unit/test_distribution_cachedexistence.js
browser/components/tests/unit/xpcshell.ini
--- a/browser/components/distribution.js
+++ b/browser/components/distribution.js
@@ -7,55 +7,87 @@ this.EXPORTED_SYMBOLS = [ "DistributionC
 var Ci = Components.interfaces;
 var Cc = Components.classes;
 var Cr = Components.results;
 var Cu = Components.utils;
 
 const DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC =
   "distribution-customization-complete";
 
+const PREF_CACHED_FILE_EXISTENCE  = "distribution.iniFile.exists.value";
+const PREF_CACHED_FILE_APPVERSION = "distribution.iniFile.exists.appversion";
+
+Cu.import("resource://gre/modules/AppConstants.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
                                   "resource://gre/modules/Preferences.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 
 this.DistributionCustomizer = function DistributionCustomizer() {
-  // For parallel xpcshell testing purposes allow loading the distribution.ini
-  // file from the profile folder through an hidden pref.
-  let loadFromProfile = Services.prefs.getBoolPref("distribution.testing.loadFromProfile", false);
-  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
-               getService(Ci.nsIProperties);
-  try {
-    let iniFile = loadFromProfile ? dirSvc.get("ProfD", Ci.nsIFile)
-                                  : dirSvc.get("XREAppDist", Ci.nsIFile);
-    if (loadFromProfile) {
-      iniFile.leafName = "distribution";
-    }
-    iniFile.append("distribution.ini");
-    if (iniFile.exists())
-      this._iniFile = iniFile;
-  } catch (ex) {}
 }
 
 DistributionCustomizer.prototype = {
-  _iniFile: null,
+  get _iniFile() {
+    // For parallel xpcshell testing purposes allow loading the distribution.ini
+    // file from the profile folder through an hidden pref.
+    let loadFromProfile = Services.prefs.getBoolPref("distribution.testing.loadFromProfile", false);
+    let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
+                 getService(Ci.nsIProperties);
+
+    let iniFile;
+    try {
+      iniFile = loadFromProfile ? dirSvc.get("ProfD", Ci.nsIFile)
+                                : dirSvc.get("XREAppDist", Ci.nsIFile);
+      if (loadFromProfile) {
+        iniFile.leafName = "distribution";
+      }
+      iniFile.append("distribution.ini");
+    } catch (ex) {}
+
+    this.__defineGetter__("_iniFile", () => iniFile);
+    return iniFile;
+  },
+
+  get _hasDistributionIni() {
+    if (Services.prefs.prefHasUserValue(PREF_CACHED_FILE_EXISTENCE)) {
+      let knownForVersion = Services.prefs.getStringPref(PREF_CACHED_FILE_APPVERSION, "unknown");
+      if (knownForVersion == AppConstants.MOZ_APP_VERSION) {
+        return Services.prefs.getBoolPref(PREF_CACHED_FILE_EXISTENCE);
+      }
+    }
+
+    let fileExists = this._iniFile.exists();
+    Services.prefs.setBoolPref(PREF_CACHED_FILE_EXISTENCE, fileExists);
+    Services.prefs.setStringPref(PREF_CACHED_FILE_APPVERSION, AppConstants.MOZ_APP_VERSION);
+
+    this.__defineGetter__("_hasDistributionIni", () => fileExists);
+    return fileExists;
+  },
 
   get _ini() {
     let ini = null;
     try {
-      if (this._iniFile) {
+      if (this._hasDistributionIni) {
         ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].
               getService(Ci.nsIINIParserFactory).
               createINIParser(this._iniFile);
       }
     } catch (e) {
-      // Unable to parse INI.
-      Cu.reportError("Unable to parse distribution.ini");
+      if (e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
+        // We probably had cached the file existence as true,
+        // but it no longer exists. We could set the new cache
+        // value here, but let's just invalidate the cache and
+        // let it be cached by a single code path on the next check.
+        Services.prefs.clearUserPref(PREF_CACHED_FILE_EXISTENCE);
+      } else {
+        // Unable to parse INI.
+        Cu.reportError("Unable to parse distribution.ini");
+      }
     }
     this.__defineGetter__("_ini", () => ini);
     return this._ini;
   },
 
   get _locale() {
     const locale = Services.locale.getRequestedLocale() || "en-US";
     this.__defineGetter__("_locale", () => locale);
--- a/browser/components/tests/unit/test_distribution.js
+++ b/browser/components/tests/unit/test_distribution.js
@@ -81,16 +81,17 @@ function run_test() {
 
 do_register_cleanup(function() {
   // Remove the distribution dir, even if the test failed, otherwise all
   // next tests will use it.
   let distDir = gProfD.clone();
   distDir.append("distribution");
   distDir.remove(true);
   Assert.ok(!distDir.exists());
+  Services.prefs.clearUserPref("distribution.testing.loadFromProfile");
 });
 
 add_task(async function() {
   // Force distribution.
   let glue = Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIObserver)
   glue.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_DISTRIBUTION_CUSTOMIZATION);
 
   var defaultBranch = Services.prefs.getDefaultBranch(null);
new file mode 100644
--- /dev/null
+++ b/browser/components/tests/unit/test_distribution_cachedexistence.js
@@ -0,0 +1,118 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests that DistributionCustomizer correctly caches the existence
+ * of the distribution.ini file and just rechecks it after a version
+ * update.
+ */
+
+const PREF_CACHED_FILE_EXISTENCE  = "distribution.iniFile.exists.value";
+const PREF_CACHED_FILE_APPVERSION = "distribution.iniFile.exists.appversion";
+const PREF_LOAD_FROM_PROFILE      = "distribution.testing.loadFromProfile";
+
+const gTestDir = do_get_cwd();
+
+Cu.import("resource://gre/modules/AppConstants.jsm");
+
+add_task(async function() {
+  // Start with a clean slate of the prefs that control this feature.
+  Services.prefs.clearUserPref(PREF_CACHED_FILE_APPVERSION);
+  Services.prefs.clearUserPref(PREF_CACHED_FILE_EXISTENCE);
+  setupTest();
+
+  let {DistributionCustomizer} = Cu.import("resource:///modules/distribution.js");
+  let distribution = new DistributionCustomizer();
+
+  copyDistributionToProfile();
+
+  // Check that checking for distribution.ini returns the right value and sets up
+  // the cached prefs.
+  let exists = distribution._hasDistributionIni;
+
+  Assert.ok(exists);
+  Assert.equal(Services.prefs.getBoolPref(PREF_CACHED_FILE_EXISTENCE, undefined),
+               true);
+  Assert.equal(Services.prefs.getStringPref(PREF_CACHED_FILE_APPVERSION, "unknown"),
+               AppConstants.MOZ_APP_VERSION);
+
+  // Check that calling _hasDistributionIni again will use the cached value. We do
+  // this by deleting the file and expecting it to still return true instead of false.
+  // Also, we need to delete _hasDistributionIni from the object because the getter
+  // was replaced with a stored value.
+  deleteDistribution();
+  delete distribution._hasDistributionIni;
+
+  exists = distribution._hasDistributionIni;
+  Assert.ok(exists);
+
+  // Now let's invalidate the PREF_CACHED_FILE_EXISTENCE pref to make sure the
+  // value gets recomputed correctly.
+  Services.prefs.clearUserPref(PREF_CACHED_FILE_EXISTENCE);
+  delete distribution._hasDistributionIni;
+  exists = distribution._hasDistributionIni;
+
+  // It now should return false, as well as storing false in the pref.
+  Assert.ok(!exists);
+  Assert.equal(Services.prefs.getBoolPref(PREF_CACHED_FILE_EXISTENCE, undefined),
+               false);
+
+  // Check now that it will use the new cached value instead of returning true in
+  // the presence of the file.
+  copyDistributionToProfile();
+  delete distribution._hasDistributionIni;
+  exists = distribution._hasDistributionIni;
+
+  Assert.ok(!exists);
+
+  // Now let's do the same, but invalidating the App Version, as if a version
+  // update occurred.
+  Services.prefs.setStringPref(PREF_CACHED_FILE_APPVERSION, "older version");
+  delete distribution._hasDistributionIni;
+  exists = distribution._hasDistributionIni;
+
+  Assert.ok(exists);
+  Assert.equal(Services.prefs.getBoolPref(PREF_CACHED_FILE_EXISTENCE, undefined),
+               true);
+  Assert.equal(Services.prefs.getStringPref(PREF_CACHED_FILE_APPVERSION, "unknown"),
+               AppConstants.MOZ_APP_VERSION);
+});
+
+
+/*
+ * Helper functions
+ */
+function copyDistributionToProfile() {
+  // Copy distribution.ini file to the profile dir.
+  let distroDir = gProfD.clone();
+  distroDir.leafName = "distribution";
+  let iniFile = distroDir.clone();
+  iniFile.append("distribution.ini");
+  if (iniFile.exists()) {
+    iniFile.remove(false);
+    print("distribution.ini already exists, did some test forget to cleanup?");
+  }
+
+  let testDistributionFile = gTestDir.clone();
+  testDistributionFile.append("distribution.ini");
+  testDistributionFile.copyTo(distroDir, "distribution.ini");
+  Assert.ok(testDistributionFile.exists());
+}
+
+function deleteDistribution() {
+  let distroDir = gProfD.clone();
+  distroDir.leafName = "distribution";
+  let iniFile = distroDir.clone();
+  iniFile.append("distribution.ini");
+  iniFile.remove(false);
+}
+
+function setupTest() {
+  // Set special pref to load distribution.ini from the profile folder.
+  Services.prefs.setBoolPref(PREF_LOAD_FROM_PROFILE, true);
+}
+
+do_register_cleanup(function() {
+  deleteDistribution();
+  Services.prefs.clearUserPref(PREF_LOAD_FROM_PROFILE);
+});
--- a/browser/components/tests/unit/xpcshell.ini
+++ b/browser/components/tests/unit/xpcshell.ini
@@ -1,10 +1,11 @@
 [DEFAULT]
 head = head.js
 firefox-appdir = browser
 skip-if = toolkit == 'android'
 support-files =
   distribution.ini
   data/engine-de-DE.xml
 
+[test_browserGlue_migration_loop_cleanup.js]
 [test_distribution.js]
-[test_browserGlue_migration_loop_cleanup.js]
+[test_distribution_cachedexistence.js]