FilesApp trash race fix

I just noticed another flake in
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/42616.

1/ The in-progress map is changed to store a globally unique identifier
so that multiple deletes across volumes cannot collide.

2/ The in-progress map was being cleared after the *.trashinfo files was
written, but before the file is moved. This is now changed so that we
only clear the map once the file is moved.

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

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

@@ -77,10 +77,14 @@ class Trash {
     this.trashDirs_ = {};
 
     /**
+     * Set of in-progress deletes. Items in this list are ignored by
+     * removeOldItems_(). Use getInProgressKey_() to create a globally unique
+     * key.
+     *
      * @private {!Set<string>}
      * @const
      */
-    this.infoWritesInProgress_ = new Set();
+    this.inProgress_ = new Set();
   }
 
   /**
@@ -193,19 +197,16 @@ class Trash {
    * removeOldItems_() if it is running concurrently.
    *
    * @param {!DirectoryEntry} trashInfoDir /.Trash/info directory.
-   * @param {string} name name for <name>.trashinfo file.
-   * @param {string} path path to use in .trashinfo file.
+   * @param {string} trashInfoName name of the *.trashinfo file.
+   * @param {string} path path to use in *.trashinfo file.
    * @return {!Promise<!FileEntry>}
    * @private
    */
-  async writeTrashInfoFile_(trashInfoDir, name, path) {
-    const filename = `${name}.trashinfo`;
-    this.infoWritesInProgress_.add(filename);
+  async writeTrashInfoFile_(trashInfoDir, trashInfoName, path) {
     return new Promise((resolve, reject) => {
-      trashInfoDir.getFile(filename, {create: true}, infoFile => {
+      trashInfoDir.getFile(trashInfoName, {create: true}, infoFile => {
         infoFile.createWriter(writer => {
           writer.onwriteend = () => {
-            this.infoWritesInProgress_.delete(filename);
             resolve(infoFile);
           };
           writer.onerror = reject;
@@ -248,6 +249,16 @@ 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.
    *
@@ -260,13 +271,17 @@ class Trash {
     const trashDirs = await this.getTrashDirs_(entry, config);
     const name =
         await fileOperationUtil.deduplicatePath(trashDirs.files, entry.name);
+    const trashInfoName = `${name}.trashinfo`;
 
     // 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 infoEntry = await this.writeTrashInfoFile_(
-        trashDirs.info, name, config.pathPrefix + entry.fullPath);
+        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);
   }
 
@@ -369,8 +384,9 @@ class Trash {
             continue;
           }
 
-          // Ignore any write-in-progress files.
-          if (this.infoWritesInProgress_.has(entry.name)) {
+          // Ignore any in-progress files.
+          if (this.inProgress_.has(
+                  this.getInProgressKey_(trashDirs.info, entry.name))) {
             console.log(`Ignoring write in progress ${entry.toURL()}`);
             continue;
           }

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

@@ -326,8 +326,7 @@ async function testRestore(done) {
 /**
  * Test removeOldEntries_().
  *
- * @suppress {accessControls} Access removeOldItems_() and
- * infoWritesInProgress_.
+ * @suppress {accessControls} Access removeOldItems_() and inProgress_().
  */
 async function testRemoveOldItems_(done) {
   const trash = new Trash();
@@ -358,7 +357,8 @@ 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.infoWritesInProgress_.add('file1.trashinfo');
+  trash.inProgress_.add(
+      `${fs.entries['/.Trash/info'].toURL()}/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'];
@@ -396,7 +396,8 @@ async function testRemoveOldItems_(done) {
   assertFalse(!!fs.entries['/.Trash/files/file5']);
   assertEquals(6, Object.keys(fs.entries).length);
 
-  trash.infoWritesInProgress_.delete('file1.trashinfo');
+  trash.inProgress_.delete(
+      `${fs.entries['/.Trash/info'].toURL()}/file1.trashinfo`);
   await trash.removeOldItems_(trashDirs, daysAgo31);
   assertFalse(!!fs.entries['/.Trash/info/file1.trashinfo']);
   assertEquals(5, Object.keys(fs.entries).length);