Fix removeOldItems_() while delete in progress

The inProgress_ Set was meant to stop removeOldItems_() deleting entries
which are currently in progress. For this to work properly, we must
ensure that item are not removed from the inProgress_ Set whilst
removeOldItems_() is running.

The single Set is replaced with Map<string, Set> so that each
TrashConfig is independent. Entries are only ever added to the set,
which is deleted once removeOldItems_() is run for each TrashConfig.

Bug: 953310
Change-Id: I2f07e7f3c3cf8733e3c91fade20f37d9ffc95afb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2563390
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832689}

ui/file_manager/file_manager/background/js/trash.js

@@ -32,21 +32,22 @@ class TrashItem {
 class Trash {
   constructor() {
     /**
-     * Store TrashDirs to avoid repeated lookup.
+     * Store TrashDirs to avoid repeated lookup, keyed by TrashConfig.id.
      * @private {!Object<string, !TrashDirs>}
      * @const
      */
     this.trashDirs_ = {};
 
     /**
-     * Set of in-progress deletes. Items in this list are ignored by
-     * removeOldItems_(). Use getInProgressKey_() to create a globally unique
-     * key.
+     * Set of in-progress deletes, keyed by TrashConfig.id with Set containing
+     * *.trashinfo filename. Items in this list are ignored by
+     * removeOldItems_(). Each Set entry is removed once removeOldItems_() runs
+     * for the related TrashConfig.
      *
-     * @private {!Set<string>}
+     * @private {!Map<string, Set<string>>}
      * @const
      */
-    this.inProgress_ = new Set();
+    this.inProgress_ = new Map(TrashConfig.CONFIG.map(c => [c.id, new Set()]));
   }
 
   /**
@@ -139,7 +140,10 @@ class Trash {
     trashDirs = await TrashDirs.getTrashDirs(entry.filesystem, config);
 
     // Check and remove old items max once per session.
-    this.removeOldItems_(trashDirs, Date.now());
+    this.removeOldItems_(trashDirs, config, Date.now()).then(() => {
+      // In-progress set is not required after removeOldItems_() runs.
+      this.inProgress_.delete(config.id);
+    });
     this.trashDirs_[config.id] = trashDirs;
     return trashDirs;
   }
@@ -188,16 +192,6 @@ class Trash {
     });
   }
 
-  /**
-   * Gets a globally unique key to use in the in-progress set.
-   *
-   * @param {!DirectoryEntry} trashInfoDir /.Trash/info directory.
-   * @param {string} trashInfoName name of the *.trashinfo file.
-   */
-  getInProgressKey_(trashInfoDir, trashInfoName) {
-    return `${trashInfoDir.toURL()}/${trashInfoName}`;
-  }
-
   /**
    * Move a file or a directory to the trash.
    *
@@ -215,12 +209,13 @@ class Trash {
     // Write trashinfo first, then only move file if info write succeeds.
     // If any step fails, the file will be unchanged, and any partial trashinfo
     // file created will be cleaned up when we remove old items.
-    const inProgressKey = this.getInProgressKey_(trashDirs.info, trashInfoName);
-    this.inProgress_.add(inProgressKey);
+    const inProgress = this.inProgress_.get(config.id);
+    if (inProgress) {
+      inProgress.add(trashInfoName);
+    }
     const infoEntry = await this.writeTrashInfoFile_(
         trashDirs.info, trashInfoName, config.pathPrefix + entry.fullPath);
     const filesEntry = await this.moveTo_(entry, trashDirs.files, name);
-    this.inProgress_.delete(inProgressKey);
     return new TrashItem(entry.name, filesEntry, infoEntry, config.pathPrefix);
   }
 
@@ -269,9 +264,10 @@ class Trash {
   /**
    * Remove any items from trash older than 30d.
    * @param {!TrashDirs} trashDirs
+   * @param {!TrashConfig} config trash config for entry.
    * @param {number} now Current time in milliseconds from epoch.
    */
-  async removeOldItems_(trashDirs, now) {
+  async removeOldItems_(trashDirs, config, now) {
     const ls = (reader) => {
       return new Promise((resolve, reject) => {
         reader.readEntries(results => resolve(results), error => reject(error));
@@ -324,8 +320,8 @@ class Trash {
           }
 
           // Ignore any in-progress files.
-          if (this.inProgress_.has(
-                  this.getInProgressKey_(trashDirs.info, entry.name))) {
+          const inProgress = this.inProgress_.get(config.id);
+          if (inProgress && inProgress.has(entry.name)) {
             console.log(`Ignoring write in progress ${entry.toURL()}`);
             continue;
           }

ui/file_manager/file_manager/background/js/trash_unittest.js

@@ -326,7 +326,7 @@ async function testRestore(done) {
 /**
  * Test removeOldEntries_().
  *
- * @suppress {accessControls} Access removeOldItems_() and inProgress_().
+ * @suppress {accessControls} Access removeOldItems_() and inProgress_.
  */
 async function testRemoveOldItems_(done) {
   const trash = new Trash();
@@ -343,6 +343,10 @@ async function testRemoveOldItems_(done) {
   const file5 = MockFileEntry.create(fs, '/dir/file5', null, new Blob(['f5']));
   const file6 = MockFileEntry.create(fs, '/dir/file6', null, new Blob(['f6']));
 
+  // Get TrashConfig.
+  const config = trash.shouldMoveToTrash(volumeManager, fs.root);
+  assert(config);
+
   // Move files to trash.
   for (const f of [file1, file2, file3, file4, file5, file6]) {
     await trash.removeFileOrDirectory(volumeManager, f, deletePermanently);
@@ -357,8 +361,7 @@ async function testRemoveOldItems_(done) {
   // Files that are write-in-progress with no DeletionDate should be ignored.
   fs.entries['/.Trash/info/file1.trashinfo'].content =
       new Blob(['no-deletion-date']);
-  trash.inProgress_.add(
-      `${fs.entries['/.Trash/info'].toURL()}/file1.trashinfo`);
+  trash.inProgress_.set('downloads-/', new Set(['file1.trashinfo']));
   delete fs.entries['/.Trash/files/file1'];
   // Files without a matching file in .Trash/files should be deleted.
   delete fs.entries['/.Trash/files/file2'];
@@ -373,7 +376,7 @@ async function testRemoveOldItems_(done) {
 
   const trashDirs =
       new TrashDirs(fs.entries['/.Trash/files'], fs.entries['/.Trash/info']);
-  await trash.removeOldItems_(trashDirs, now);
+  await trash.removeOldItems_(trashDirs, config, now);
   assertTrue(!!fs.entries['/']);
   assertTrue(!!fs.entries['/.Trash']);
   assertTrue(!!fs.entries['/.Trash/files']);
@@ -387,18 +390,18 @@ async function testRemoveOldItems_(done) {
   // Items older than 30d should be deleted.
   const daysAgo29 = now + (29 * 24 * 60 * 60 * 1000);
   const daysAgo31 = now + (31 * 24 * 60 * 60 * 1000);
-  await trash.removeOldItems_(trashDirs, daysAgo29);
+  await trash.removeOldItems_(trashDirs, config, daysAgo29);
   assertEquals(8, Object.keys(fs.entries).length);
 
-  await trash.removeOldItems_(trashDirs, daysAgo31);
+  await trash.removeOldItems_(trashDirs, config, daysAgo31);
   assertTrue(!!fs.entries['/.Trash/info/file1.trashinfo']);
   assertFalse(!!fs.entries['/.Trash/info/file5.trashinfo']);
   assertFalse(!!fs.entries['/.Trash/files/file5']);
   assertEquals(6, Object.keys(fs.entries).length);
 
-  trash.inProgress_.delete(
-      `${fs.entries['/.Trash/info'].toURL()}/file1.trashinfo`);
-  await trash.removeOldItems_(trashDirs, daysAgo31);
+  // trashinfo with no matching file, and not in-progress should be deleted.
+  trash.inProgress_.get('downloads-/').delete('file1.trashinfo');
+  await trash.removeOldItems_(trashDirs, config, daysAgo31);
   assertFalse(!!fs.entries['/.Trash/info/file1.trashinfo']);
   assertEquals(5, Object.keys(fs.entries).length);