FileSystem: Invoke correct error callback for bad readEntries() call

The DirectoryReader readEntries() call fails if invoked a second time while there's an outstanding request. But the failure was being reported on the first call's callback, which is non-inuitive. Fix by reordering the duplicate call logic and not relying on the reader's OnError handler. Manual Test: entries-api/filesystemdirectoryreader-manual.html Bug: 1054981 Change-Id: Ieaaea993c48af5d9c1fabe4ec509afe3d22b3477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2069017Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Commit-Queue: Joshua Bell <jsbell@chromium.org> Cr-Commit-Position: refs/heads/master@{#747076}

FileSystem: Invoke correct error callback for bad readEntries() call
The DirectoryReader readEntries() call fails if invoked a second time while there's an outstanding request. But the failure was being reported on the first call's callback, which is non-inuitive. Fix by reordering the duplicate call logic and not relying on the reader's OnError handler. Manual Test: entries-api/filesystemdirectoryreader-manual.html Bug: 1054981 Change-Id: Ieaaea993c48af5d9c1fabe4ec509afe3d22b3477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2069017Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Commit-Queue: Joshua Bell <jsbell@chromium.org> Cr-Commit-Position: refs/heads/master@{#747076}
fd14a399 · Joshua Bell · Commit Bot · 2dba6fd3 · fd14a399 · fd14a399
Commit fd14a399 authored Mar 05, 2020 by Joshua Bell Committed by Commit Bot Mar 05, 2020
2 changed files
--- a/third_party/blink/renderer/modules/filesystem/directory_reader.cc
+++ b/third_party/blink/renderer/modules/filesystem/directory_reader.cc
@@ -51,6 +51,20 @@ DirectoryReader::DirectoryReader(DOMFileSystemBase* file_system,

 void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
                                  V8ErrorCallback* error_callback) {
+  if (entries_callback_) {
+    // Non-null entries_callback_ means multiple readEntries() calls are made
+    // concurrently. We don't allow doing it.
+    Filesystem()->ReportError(
+        WTF::Bind(
+            [](V8ErrorCallback* error_callback, base::File::Error error) {
+              error_callback->InvokeAndReportException(
+                  nullptr, file_error::CreateDOMException(error));
+            },
+            WrapPersistent(error_callback)),
+        base::File::FILE_ERROR_FAILED);
+    return;
+  }
+
  auto success_callback_wrapper = WTF::BindRepeating(
      [](DirectoryReader* persistent_reader, EntryHeapVector* entries) {
        persistent_reader->AddEntries(*entries);
@@ -71,15 +85,6 @@ void DirectoryReader::readEntries(V8EntriesCallback* entries_callback,
    return;
  }

-  if (entries_callback_) {
-    // Non-null entries_callback_ means multiple readEntries() calls are made
-    // concurrently. We don't allow doing it.
-    Filesystem()->ReportError(
-        WTF::Bind(&DirectoryReader::OnError, WrapPersistentIfNeeded(this)),
-        base::File::FILE_ERROR_FAILED);
-    return;
-  }
-
  if (!has_more_entries_ || !entries_.IsEmpty()) {
    EntryHeapVector* entries =
        MakeGarbageCollected<EntryHeapVector>(std::move(entries_));

--- a/third_party/blink/web_tests/fast/filesystem/read-directory-multiple-readEntry-calls.html
+++ b/third_party/blink/web_tests/fast/filesystem/read-directory-multiple-readEntry-calls.html
+<!doctype html>
+<title>readEntries() called multiple times</title>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="resources/fs-test-util.js"></script>
+<script>
+
+// Needed as a rejected promise is lazily analyzed.
+setup({ allow_uncaught_exception: true });
+
+// Wrap a typical FS method call with trailing success/error callback
+// methods into a promise. Pass object as first arg, method as second,
+// and optional args.
+// Example: file = await asPromise(dir, dir.getFile, path, {});
+function asPromise(receiver, func) {
+  const args = [...arguments].slice(2);
+  return new Promise((resolve, reject) => {
+    func.apply(receiver, [...args, resolve, reject]);
+  });
+}
+
+promise_test(async t => {
+  const fileSystem =
+    await asPromise(window, webkitRequestFileSystem, TEMPORARY, 100);
+  const dir = fileSystem.root;
+  await asPromise(null, removeAllInDirectory, dir);
+  t.add_cleanup(() => asPromise(null, removeAllInDirectory, dir));
+
+  const files = ['a', 'b', 'c', 'd', 'e', 'f'];
+  for (const file of files) {
+    await asPromise(dir, dir.getFile, file, {create: true});
+  }
+
+  const reader = dir.createReader();
+  const read = [];
+
+  while (true) {
+    // Call, but don't await as that could let the first one finish.
+    const p = asPromise(reader, reader.readEntries);
+    const q = asPromise(reader, reader.readEntries);
+
+    // First call should have succeeded.
+    const entries = await p;
+
+    // Second should have failed.
+    promise_rejects_dom(t, 'InvalidStateError', q,
+                        'Second call before first finishes should fail');
+
+    if (entries.length === 0)
+      break;
+    entries.forEach(entry => { read.push(entry.name); });
+  }
+
+  assert_array_equals(read.sort(), files.sort(),
+                      'Should read all files, despite failed calls.');
+
+}, 'Verify that readEntries() fails predictably when called multiple times');
+
+</script>