FilesApp trash: removeOldFiles should ignore write-in-progress info

*.trashinfo files require 2 async operation to first create the file,
and then write to it.  This can cause issues if removeOldFiles_() is
running concurrently.

Hopefully this fixes test flakes.

Bug: 953310
Change-Id: Ib6d192122a97f956ba79ec11a8f5e099f4d418c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2515499
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@{#823376}

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

@@ -75,6 +75,12 @@ class Trash {
      * @const
      */
     this.trashDirs_ = {};
+
+    /**
+     * @private {!Set<string>}
+     * @const
+     */
+    this.infoWritesInProgress_ = new Set();
   }
 
   /**
@@ -182,10 +188,9 @@ class Trash {
 
   /**
    * Write /.Trash/info/<name>.trashinfo file.
-   * Creates empty /.Trash/info/<name>.trashinfo.tmp file, writes to file,
-   * then moves to /.Trash/info/<name>.trashinfo. By using mv as the final
-   * operation we guarantee that another process such as removing old items
-   * will not read an incomplete *.trashinfo file.
+   * Since creating and writing the file requires multiple async operations, we
+   * keep a record of which writes are in progress to be ignored by
+   * removeOldItems_() if it is running concurrently.
    *
    * @param {!DirectoryEntry} trashInfoDir /.Trash/info directory.
    * @param {string} name name for <name>.trashinfo file.
@@ -194,12 +199,15 @@ class Trash {
    * @private
    */
   async writeTrashInfoFile_(trashInfoDir, name, path) {
-    const tmpName = `${name}.trashinfo.tmp`;
-    const finalName = `${name}.trashinfo`;
-    const tmpFile = await new Promise((resolve, reject) => {
-      trashInfoDir.getFile(tmpName, {create: true}, infoFile => {
+    const filename = `${name}.trashinfo`;
+    this.infoWritesInProgress_.add(filename);
+    return new Promise((resolve, reject) => {
+      trashInfoDir.getFile(filename, {create: true}, infoFile => {
         infoFile.createWriter(writer => {
-          writer.onwriteend = resolve.bind(null, infoFile);
+          writer.onwriteend = () => {
+            this.infoWritesInProgress_.delete(filename);
+            resolve(infoFile);
+          };
           writer.onerror = reject;
           const info = `[Trash Info]\nPath=${path}\nDeletionDate=${
               new Date().toISOString()}`;
@@ -207,7 +215,6 @@ class Trash {
         }, reject);
       }, reject);
     });
-    return this.moveTo_(tmpFile, trashInfoDir, finalName);
   }
 
   /**
@@ -350,20 +357,36 @@ class Trash {
           break;
         }
         for (const entry of entries) {
+          // Delete any directories.
           if (!entry.isFile) {
             rm(entry, console.error, 'Unexpected trash info directory');
             continue;
           }
 
+          // Delete any files not *.trashinfo.
           if (!entry.name.endsWith('.trashinfo')) {
             rm(entry, console.error, 'Unexpected trash info file');
             continue;
           }
 
+          // Ignore any write-in-progress files.
+          if (this.infoWritesInProgress_.has(entry.name)) {
+            console.log(`Ignoring write in progress ${entry.toURL()}`);
+            continue;
+          }
+
           const name = entry.name.substring(0, entry.name.length - 10);
           const filesEntry = filesEntries[name];
           delete filesEntries[name];
 
+          // Delete any .trashinfo file with no matching file entry (unless it
+          // was write-in-progress).
+          if (!filesEntry) {
+            rm(entry, console.error, 'No matching files entry');
+            continue;
+          }
+
+          // Delete any entries with no DeletionDate.
           const file = await new Promise(
               (resolve, reject) => entry.file(resolve, reject));
           const text = await file.text();
@@ -374,6 +397,7 @@ class Trash {
             continue;
           }
 
+          // Delete any entries with invalid DeletionDate.
           const d = Date.parse(found[1]);
           if (!d) {
             rm(entry, console.error, 'Could not parse DeletionDate in ' + text);
@@ -381,6 +405,7 @@ class Trash {
             continue;
           }
 
+          // Delete entries older than 30d.
           const ago30d = now - Trash.AUTO_DELETE_INTERVAL_MS;
           const ago30dStr = new Date(ago30d).toISOString();
           if (d < ago30d) {

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

@@ -326,7 +326,8 @@ async function testRestore(done) {
 /**
  * Test removeOldEntries_().
  *
- * @suppress {accessControls} Access removeOldItems_().
+ * @suppress {accessControls} Access removeOldItems_() and
+ * infoWritesInProgress_.
  */
 async function testRemoveOldItems_(done) {
   const trash = new Trash();
@@ -341,28 +342,34 @@ async function testRemoveOldItems_(done) {
   const file3 = MockFileEntry.create(fs, '/dir/file3', null, new Blob(['f3']));
   const file4 = MockFileEntry.create(fs, '/dir/file4', null, new Blob(['f4']));
   const file5 = MockFileEntry.create(fs, '/dir/file5', null, new Blob(['f5']));
+  const file6 = MockFileEntry.create(fs, '/dir/file6', null, new Blob(['f6']));
 
   // Move files to trash.
-  for (const f of [file1, file2, file3, file4, file5]) {
+  for (const f of [file1, file2, file3, file4, file5, file6]) {
     await trash.removeFileOrDirectory(volumeManager, f, deletePermanently);
   }
-  assertEquals(15, Object.keys(fs.entries).length);
+  assertEquals(17, Object.keys(fs.entries).length);
   const now = Date.now();
 
   // Directories inside info should be deleted.
   MockDirectoryEntry.create(fs, '/.Trash/info/baddir.trashinfo');
   // Files that do not end with .trashinfo should be deleted.
   MockFileEntry.create(fs, '/.Trash/info/f', null, new Blob(['f']));
-  // Files without a matching file in .Trash/files are left.
+  // 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');
   delete fs.entries['/.Trash/files/file1'];
+  // Files without a matching file in .Trash/files should be deleted.
+  delete fs.entries['/.Trash/files/file2'];
   // Files with no DeletionDate should be deleted.
-  fs.entries['/.Trash/info/file2.trashinfo'].content =
+  fs.entries['/.Trash/info/file3.trashinfo'].content =
       new Blob(['no-deletion-date']);
   // Files with DeletionDate which cannot be parsed should be deleted.
-  fs.entries['/.Trash/info/file3.trashinfo'].content =
+  fs.entries['/.Trash/info/file4.trashinfo'].content =
       new Blob(['DeletionDate=abc']);
   // Files with no matching trashinfo should be deleted.
-  delete fs.entries['/.Trash/info/file4.trashinfo'];
+  delete fs.entries['/.Trash/info/file5.trashinfo'];
 
   const trashDirs =
       new TrashDirs(fs.entries['/.Trash/files'], fs.entries['/.Trash/info']);
@@ -370,19 +377,28 @@ async function testRemoveOldItems_(done) {
   assertTrue(!!fs.entries['/']);
   assertTrue(!!fs.entries['/.Trash']);
   assertTrue(!!fs.entries['/.Trash/files']);
-  assertTrue(!!fs.entries['/.Trash/files/file5']);
+  assertTrue(!!fs.entries['/.Trash/files/file6']);
   assertTrue(!!fs.entries['/.Trash/info']);
   assertTrue(!!fs.entries['/.Trash/info/file1.trashinfo']);
-  assertTrue(!!fs.entries['/.Trash/info/file5.trashinfo']);
+  assertTrue(!!fs.entries['/.Trash/info/file6.trashinfo']);
   assertTrue(!!fs.entries['/dir']);
   assertEquals(8, Object.keys(fs.entries).length);
 
   // Items older than 30d should be deleted.
-  await trash.removeOldItems_(trashDirs, now + (29 * 24 * 60 * 60 * 1000));
+  const daysAgo29 = now + (29 * 24 * 60 * 60 * 1000);
+  const daysAgo31 = now + (31 * 24 * 60 * 60 * 1000);
+  await trash.removeOldItems_(trashDirs, daysAgo29);
   assertEquals(8, Object.keys(fs.entries).length);
-  await trash.removeOldItems_(trashDirs, now + (31 * 24 * 60 * 60 * 1000));
+
+  await trash.removeOldItems_(trashDirs, 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.infoWritesInProgress_.delete('file1.trashinfo');
+  await trash.removeOldItems_(trashDirs, daysAgo31);
+  assertFalse(!!fs.entries['/.Trash/info/file1.trashinfo']);
   assertEquals(5, Object.keys(fs.entries).length);
 
   done();