Bug 1462483: Part 2 - Cleanup directory iteration code. r?aswan draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 17 May 2018 15:14:48 -0700
changeset 796605 ae528dc849fa77830150f5eede9c6617f206188e
parent 796604 0e2f65cf66723a6c063a2be606b8ee7b6bfe88a1
child 796630 66e4f78682fe72d2190c078a9b3d17ce7160bede
push id110307
push usermaglione.k@gmail.com
push dateThu, 17 May 2018 22:18:20 +0000
reviewersaswan
bugs1462483
milestone62.0a1
Bug 1462483: Part 2 - Cleanup directory iteration code. r?aswan MozReview-Commit-ID: 3VzaVPsD8VT
toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
toolkit/mozapps/extensions/internal/XPIInstall.jsm
toolkit/mozapps/extensions/internal/XPIProvider.jsm
--- a/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
+++ b/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
@@ -397,54 +397,38 @@ var AddonTestUtils = {
         let {value} = Object.getOwnPropertyDescriptor(FileTestUtils,
                                                       "_globalTemporaryDirectory");
         if (value) {
           ignoreEntries.add(value.leafName);
         }
       }
 
       // Check that the temporary directory is empty
-      var dirEntries = this.tempDir.directoryEntries
-                           .QueryInterface(Ci.nsIDirectoryEnumerator);
       var entries = [];
-      while (dirEntries.hasMoreElements()) {
-        let {leafName} = dirEntries.nextFile;
+      for (let {leafName} of this.iterDirectory(this.tempDir)) {
         if (!ignoreEntries.has(leafName)) {
           entries.push(leafName);
         }
       }
       if (entries.length)
         throw new Error(`Found unexpected files in temporary directory: ${entries.join(", ")}`);
 
-      dirEntries.close();
-
       try {
         appDirForAddons.remove(true);
       } catch (ex) {
         testScope.info(`Got exception removing addon app dir: ${ex}`);
       }
 
       // ensure no leftover files in the system addon upgrade location
       let featuresDir = this.profileDir.clone();
       featuresDir.append("features");
       // upgrade directories will be in UUID folders under features/
-      let systemAddonDirs = [];
-      if (featuresDir.exists()) {
-        let featuresDirEntries = featuresDir.directoryEntries
-                                            .QueryInterface(Ci.nsIDirectoryEnumerator);
-        while (featuresDirEntries.hasMoreElements()) {
-          let entry = featuresDirEntries.getNext();
-          entry.QueryInterface(Ci.nsIFile);
-          systemAddonDirs.push(entry);
-        }
-
-        systemAddonDirs.map(dir => {
-          dir.append("stage");
-          pathShouldntExist(dir);
-        });
+      for (let dir of this.iterDirectory(featuresDir)) {
+        dir.append("stage");
+        pathShouldntExist(dir);
       }
 
       // ensure no leftover files in the user addon location
       let testDir = this.profileDir.clone();
       testDir.append("extensions");
       testDir.append("trash");
       pathShouldntExist(testDir);
 
@@ -470,16 +454,43 @@ var AddonTestUtils = {
         this.tempDir.remove(true);
       } catch (e) {
         Cu.reportError(e);
       }
     });
   },
 
   /**
+   * Iterates over the entries in a given directory.
+   *
+   * Fails silently if the given directory does not exist.
+   *
+   * @param {nsIFile} dir
+   *        Directory to iterate.
+   */
+  * iterDirectory(dir) {
+    let dirEnum;
+    try {
+      dirEnum = dir.directoryEntries;
+      let file;
+      while ((file = dirEnum.nextFile)) {
+        yield file;
+      }
+    } catch (e) {
+      if (dir.exists()) {
+        Cu.reportError(e);
+      }
+    } finally {
+      if (dirEnum) {
+        dirEnum.close();
+      }
+    }
+  },
+
+  /**
    * Creates a new HttpServer for testing, and begins listening on the
    * specified port. Automatically shuts down the server when the test
    * unit ends.
    *
    * @param {object} [options = {}]
    *        The options object.
    * @param {integer} [options.port = -1]
    *        The port to listen on. If omitted, listen on a random
@@ -1205,21 +1216,18 @@ var AddonTestUtils = {
    * @param {nsIFile} ext A file pointing to either the packed extension or its unpacked directory.
    * @param {number} time The time to which we set the lastModifiedTime of the extension
    *
    * @deprecated Please use promiseSetExtensionModifiedTime instead
    */
   setExtensionModifiedTime(ext, time) {
     ext.lastModifiedTime = time;
     if (ext.isDirectory()) {
-      let entries = ext.directoryEntries
-                       .QueryInterface(Ci.nsIDirectoryEnumerator);
-      while (entries.hasMoreElements())
-        this.setExtensionModifiedTime(entries.nextFile, time);
-      entries.close();
+      for (let file of this.iterDirectory(ext))
+        this.setExtensionModifiedTime(file, time);
     }
   },
 
   async promiseSetExtensionModifiedTime(path, time) {
     await OS.File.setDates(path, time, time);
 
     let iterator = new OS.File.DirectoryIterator(path);
     try {
--- a/toolkit/mozapps/extensions/internal/XPIInstall.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ -83,31 +83,32 @@ const PREF_DISTRO_ADDONS_PERMS        = 
 const PREF_INSTALL_REQUIRESECUREORIGIN = "extensions.install.requireSecureOrigin";
 const PREF_PENDING_OPERATIONS         = "extensions.pendingOperations";
 const PREF_SYSTEM_ADDON_UPDATE_URL    = "extensions.systemAddon.update.url";
 const PREF_XPI_ENABLED                = "xpinstall.enabled";
 const PREF_XPI_DIRECT_WHITELISTED     = "xpinstall.whitelist.directRequest";
 const PREF_XPI_FILE_WHITELISTED       = "xpinstall.whitelist.fileRequest";
 const PREF_XPI_WHITELIST_REQUIRED     = "xpinstall.whitelist.required";
 
-/* globals BOOTSTRAP_REASONS, KEY_APP_SYSTEM_ADDONS, KEY_APP_SYSTEM_DEFAULTS, PREF_BRANCH_INSTALLED_ADDON, PREF_SYSTEM_ADDON_SET, TEMPORARY_ADDON_SUFFIX, SIGNED_TYPES, TOOLKIT_ID, XPI_PERMISSION, XPIStates, getExternalType, isTheme, isWebExtension */
+/* globals BOOTSTRAP_REASONS, KEY_APP_SYSTEM_ADDONS, KEY_APP_SYSTEM_DEFAULTS, PREF_BRANCH_INSTALLED_ADDON, PREF_SYSTEM_ADDON_SET, TEMPORARY_ADDON_SUFFIX, SIGNED_TYPES, TOOLKIT_ID, XPI_PERMISSION, XPIStates, getExternalType, isTheme, isWebExtension, iterDirectory */
 const XPI_INTERNAL_SYMBOLS = [
   "BOOTSTRAP_REASONS",
   "KEY_APP_SYSTEM_ADDONS",
   "KEY_APP_SYSTEM_DEFAULTS",
   "PREF_BRANCH_INSTALLED_ADDON",
   "PREF_SYSTEM_ADDON_SET",
   "SIGNED_TYPES",
   "TEMPORARY_ADDON_SUFFIX",
   "TOOLKIT_ID",
   "XPI_PERMISSION",
   "XPIStates",
   "getExternalType",
   "isTheme",
   "isWebExtension",
+  "iterDirectory",
 ];
 
 for (let name of XPI_INTERNAL_SYMBOLS) {
   XPCOMUtils.defineLazyGetter(this, name, () => XPIInternal[name]);
 }
 
 /**
  * Returns a nsIFile instance for the given path, relative to the given
@@ -1100,17 +1101,17 @@ function recursiveRemove(aFile) {
       throw e;
     }
   }
 
   // Use a snapshot of the directory contents to avoid possible issues with
   // iterating over a directory while removing files from it (the YAFFS2
   // embedded filesystem has this issue, see bug 772238), and to remove
   // normal files before their resource forks on OSX (see bug 733436).
-  let entries = getDirectoryEntries(aFile, true);
+  let entries = Array.from(iterDirectory(aFile));
   entries.forEach(recursiveRemove);
 
   try {
     aFile.remove(true);
   } catch (e) {
     logger.error("Failed to remove empty directory " + aFile.path, e);
     throw e;
   }
@@ -1276,54 +1277,16 @@ SafeInstallOperation.prototype = {
       }
     }
 
     while (this._createdDirs.length > 0)
       recursiveRemove(this._createdDirs.pop());
   }
 };
 
-/**
- * Gets a snapshot of directory entries.
- *
- * @param {nsIFile} aDir
- *        Directory to look at
- * @param {boolean} aSortEntries
- *        True to sort entries by filename
- * @returns {nsIFile[]}
- *        An files in the directory, or an empty array if aDir is not a
- *        readable directory.
- */
-function getDirectoryEntries(aDir, aSortEntries) {
-  let dirEnum;
-  try {
-    dirEnum = aDir.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
-    let entries = [];
-    while (dirEnum.hasMoreElements())
-      entries.push(dirEnum.nextFile);
-
-    if (aSortEntries) {
-      entries.sort(function(a, b) {
-        return a.path > b.path ? -1 : 1;
-      });
-    }
-
-    return entries;
-  } catch (e) {
-    if (aDir.exists()) {
-      logger.warn("Can't iterate directory " + aDir.path, e);
-    }
-    return [];
-  } finally {
-    if (dirEnum) {
-      dirEnum.close();
-    }
-  }
-}
-
 function getHashStringForCrypto(aCrypto) {
   // return the two-digit hexadecimal code for a byte
   let toHexString = charCode => ("0" + charCode.toString(16)).slice(-2);
 
   // convert the binary hash data to a hex string.
   let binary = aCrypto.finish(false);
   let hash = Array.from(binary, c => toHexString(c.charCodeAt(0)));
   return hash.join("").toLowerCase();
@@ -2846,23 +2809,19 @@ class DirectoryInstallLocation {
     for (let name of aLeafNames) {
       let file = getFile(name, dir);
       recursiveRemove(file);
     }
 
     if (this._stagingDirLock > 0)
       return;
 
-    let dirEntries = dir.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
-    try {
-      if (dirEntries.nextFile)
-        return;
-    } finally {
-      dirEntries.close();
-    }
+    // eslint-disable-next-line no-unused-vars
+    for (let file of iterDirectory(dir))
+      return;
 
     try {
       setFilePermissions(dir, FileUtils.PERMS_DIRECTORY);
       dir.remove(false);
     } catch (e) {
       logger.warn("Failed to remove staging dir", e);
       // Failing to remove the staging directory is ignorable
     }
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -411,45 +411,35 @@ function getURIForResourceInFile(aFile, 
  */
 function buildJarURI(aJarfile, aPath) {
   let uri = Services.io.newFileURI(aJarfile);
   uri = "jar:" + uri.spec + "!/" + aPath;
   return Services.io.newURI(uri);
 }
 
 /**
- * Gets a snapshot of directory entries.
+ * Iterates over the entries in a given directory.
+ *
+ * Fails silently if the given directory does not exist.
  *
- * @param {nsIURI} aDir
- *        Directory to look at
- * @param {boolean} aSortEntries
- *        True to sort entries by filename
- * @returns {Array<nsIFile>}
- *        An array of nsIFile, or an empty array if aDir is not a readable directory
+ * @param {nsIFile} aDir
+ *        Directory to iterate.
  */
-function getDirectoryEntries(aDir, aSortEntries) {
+function* iterDirectory(aDir) {
   let dirEnum;
   try {
-    dirEnum = aDir.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
-    let entries = [];
-    while (dirEnum.hasMoreElements())
-      entries.push(dirEnum.nextFile);
-
-    if (aSortEntries) {
-      entries.sort(function(a, b) {
-        return a.path > b.path ? -1 : 1;
-      });
+    dirEnum = aDir.directoryEntries;
+    let file;
+    while ((file = dirEnum.nextFile)) {
+      yield file;
     }
-
-    return entries;
   } catch (e) {
     if (aDir.exists()) {
-      logger.warn("Can't iterate directory " + aDir.path, e);
+      logger.warn(`Can't iterate directory ${aDir.path}`, e);
     }
-    return [];
   } finally {
     if (dirEnum) {
       dirEnum.close();
     }
   }
 }
 
 /**
@@ -990,18 +980,17 @@ class DirectoryLocation extends XPIState
     if (!this.dir) {
       return addons;
     }
     this.initialized = true;
 
     // Use a snapshot of the directory contents to avoid possible issues with
     // iterating over a directory while removing files from it (the YAFFS2
     // embedded filesystem has this issue, see bug 772238).
-    let entries = getDirectoryEntries(this.dir);
-    for (let entry of entries) {
+    for (let entry of Array.from(iterDirectory(this.dir))) {
       let id = entry.leafName;
       if (id == DIR_STAGE || id == DIR_TRASH)
         continue;
 
       let isFile = id.toLowerCase().endsWith(".xpi");
       if (isFile) {
         id = id.substring(0, id.length - 4);
       }
@@ -2497,68 +2486,61 @@ var XPIProvider = {
    *        See checkForChanges
    * @returns {boolean}
    *        True if any new add-ons were installed
    */
   installDistributionAddons(aManifests, aAppChanged) {
     let distroDir;
     try {
       distroDir = FileUtils.getDir(KEY_APP_DISTRIBUTION, [DIR_EXTENSIONS]);
-      if (!distroDir.isDirectory())
-        return false;
     } catch (e) {
       return false;
     }
 
     let changed = false;
     let profileLocation = XPIStates.getLocation(KEY_APP_PROFILE);
 
-    let entries = distroDir.directoryEntries
-                           .QueryInterface(Ci.nsIDirectoryEnumerator);
-    let entry;
-    while ((entry = entries.nextFile)) {
-      let id = entry.leafName;
+    for (let file of iterDirectory(distroDir)) {
+      let id = file.leafName;
       if (id.endsWith(".xpi")) {
         id = id.slice(0, -4);
       } else {
-        logger.debug("Ignoring distribution add-on that isn't an XPI: " + entry.path);
+        logger.debug(`Ignoring distribution add-on that isn't an XPI: ${file.path}`);
         continue;
       }
 
       if (!gIDTest.test(id)) {
         logger.debug("Ignoring distribution add-on whose name is not a valid add-on ID: " +
-            entry.path);
+                     file.path);
         continue;
       }
 
       /* If this is not an upgrade and we've already handled this extension
        * just continue */
       if (!aAppChanged && Services.prefs.prefHasUserValue(PREF_BRANCH_INSTALLED_ADDON + id)) {
         continue;
       }
 
       try {
-        let addon = awaitPromise(XPIInstall.installDistributionAddon(id, entry, profileLocation));
+        let addon = awaitPromise(XPIInstall.installDistributionAddon(id, file, profileLocation));
 
         if (addon) {
           // aManifests may contain a copy of a newly installed add-on's manifest
           // and we'll have overwritten that so instead cache our install manifest
           // which will later be put into the database in processFileChanges
           if (!(KEY_APP_PROFILE in aManifests))
             aManifests[KEY_APP_PROFILE] = {};
           aManifests[KEY_APP_PROFILE][id] = addon;
           changed = true;
         }
       } catch (e) {
-        logger.error(`Failed to install distribution add-on ${entry.path}`, e);
+        logger.error(`Failed to install distribution add-on ${file.path}`, e);
       }
     }
 
-    entries.close();
-
     return changed;
   },
 
   getNewDistroAddons() {
     let addons = this.newDistroAddons;
     this.newDistroAddons = null;
     return addons;
   },
@@ -3012,16 +2994,17 @@ var XPIInternal = {
   XPIStates,
   XPI_PERMISSION,
   awaitPromise,
   canRunInSafeMode,
   getExternalType,
   getURIForResourceInFile,
   isTheme,
   isWebExtension,
+  iterDirectory,
 };
 
 var addonTypes = [
   new AddonManagerPrivate.AddonType("extension", URI_EXTENSION_STRINGS,
                                     "type.extension.name",
                                     AddonManager.VIEW_TYPE_LIST, 4000,
                                     AddonManager.TYPE_SUPPORTS_UNDO_RESTARTLESS_UNINSTALL),
   new AddonManagerPrivate.AddonType("theme", URI_EXTENSION_STRINGS,