Bug 1389344 - Avoid checking for distribution.ini existence on every startup. r=mkaply draft
authorFelipe Gomes <felipc@gmail.com>
Tue, 19 Sep 2017 14:41:06 -0300
changeset 667063 4974d7c13317c7a5a48addb309af59634cc508ee
parent 666416 42151fcd6cfc216d147730d0f2c6a2acd52d22fd
child 732291 b264e9375e72c7c012dc52009534018c8e2e6037
push id80607
push userfelipc@gmail.com
push dateTue, 19 Sep 2017 17:41:35 +0000
reviewersmkaply
bugs1389344
milestone57.0a1
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]