Title: [284652] trunk
Revision
284652
Author
sihui_...@apple.com
Date
2021-10-21 16:13:02 -0700 (Thu, 21 Oct 2021)

Log Message

FileSystemSyncAccessHandle should close platform file handle on close()
https://bugs.webkit.org/show_bug.cgi?id=232067
<rdar://problem/84517013>

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

* web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt:

Source/WebCore:

This patch also ensures no request is sent after close() is called.

Test: storage/filesystemaccess/sync-access-handle-close-worker.html

* Modules/filesystemaccess/FileSystemFileHandle.cpp:
(WebCore::FileSystemFileHandle::createSyncAccessHandle):
* Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:
(WebCore::FileSystemSyncAccessHandle::FileSystemSyncAccessHandle):
(WebCore::FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle):
(WebCore::FileSystemSyncAccessHandle::isClosingOrClosed const):
(WebCore::FileSystemSyncAccessHandle::truncate):
(WebCore::FileSystemSyncAccessHandle::getSize):
(WebCore::FileSystemSyncAccessHandle::flush):
(WebCore::FileSystemSyncAccessHandle::close):
(WebCore::FileSystemSyncAccessHandle::didClose):
(WebCore::FileSystemSyncAccessHandle::read):
(WebCore::FileSystemSyncAccessHandle::write):
* Modules/filesystemaccess/FileSystemSyncAccessHandle.h:

LayoutTests:

* storage/filesystemaccess/resources/sync-access-handle-close.js: Added.
(finishTest):
(async testFunctions):
(async test):
* storage/filesystemaccess/sync-access-handle-close-worker-expected.txt: Added.
* storage/filesystemaccess/sync-access-handle-close-worker.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (284651 => 284652)


--- trunk/LayoutTests/ChangeLog	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/LayoutTests/ChangeLog	2021-10-21 23:13:02 UTC (rev 284652)
@@ -1,3 +1,18 @@
+2021-10-21  Sihui Liu  <sihui_...@apple.com>
+
+        FileSystemSyncAccessHandle should close platform file handle on close()
+        https://bugs.webkit.org/show_bug.cgi?id=232067
+        <rdar://problem/84517013>
+
+        Reviewed by Youenn Fablet.
+
+        * storage/filesystemaccess/resources/sync-access-handle-close.js: Added.
+        (finishTest):
+        (async testFunctions):
+        (async test):
+        * storage/filesystemaccess/sync-access-handle-close-worker-expected.txt: Added.
+        * storage/filesystemaccess/sync-access-handle-close-worker.html: Added.
+
 2021-10-21  Ayumi Kojima  <ayumi_koj...@apple.com>
 
         [ iOS ] http/wpt/service-workers/skipFetchEvent.https.html is a flaky failure.

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (284651 => 284652)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2021-10-21 23:13:02 UTC (rev 284652)
@@ -1,3 +1,13 @@
+2021-10-21  Sihui Liu  <sihui_...@apple.com>
+
+        FileSystemSyncAccessHandle should close platform file handle on close()
+        https://bugs.webkit.org/show_bug.cgi?id=232067
+        <rdar://problem/84517013>
+
+        Reviewed by Youenn Fablet.
+
+        * web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt:
+
 2021-10-21  Rob Buis  <rb...@igalia.com>
 
         [css-contain] Support contain:style for counters

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt (284651 => 284652)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt	2021-10-21 23:13:02 UTC (rev 284652)
@@ -1,14 +1,14 @@
 
-FAIL SyncAccessHandle.close is idempotent promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
-FAIL SyncAccessHandle.close is idempotent when called immediately promise_test: Unhandled rejection with value: object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)."
-FAIL SyncAccessHandle.read fails after SyncAccessHandle.close settles promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
-FAIL SyncAccessHandle.read fails immediately after calling SyncAccessHandle.close promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
-FAIL SyncAccessHandle.write fails after SyncAccessHandle.close settles promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
-FAIL SyncAccessHandle.write fails immediately after calling SyncAccessHandle.close promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
-FAIL SyncAccessHandle.flush fails after SyncAccessHandle.close settles promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
-FAIL SyncAccessHandle.flush fails immediately after calling SyncAccessHandle.close promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
-FAIL SyncAccessHandle.getSize fails after SyncAccessHandle.close settles promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
-FAIL SyncAccessHandle.getSize fails immediately after calling SyncAccessHandle.close promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
-FAIL SyncAccessHandle.truncate fails after SyncAccessHandle.close settles promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
-FAIL SyncAccessHandle.truncate fails immediately after calling SyncAccessHandle.close promise_rejects_dom: function "function () { throw e }" threw object "UnknownError: The operation failed for an unknown transient reason (e.g. out of memory)." that is not a DOMException InvalidStateError: property "code" is equal to 0, expected 11
+PASS SyncAccessHandle.close is idempotent
+PASS SyncAccessHandle.close is idempotent when called immediately
+PASS SyncAccessHandle.read fails after SyncAccessHandle.close settles
+PASS SyncAccessHandle.read fails immediately after calling SyncAccessHandle.close
+PASS SyncAccessHandle.write fails after SyncAccessHandle.close settles
+PASS SyncAccessHandle.write fails immediately after calling SyncAccessHandle.close
+PASS SyncAccessHandle.flush fails after SyncAccessHandle.close settles
+PASS SyncAccessHandle.flush fails immediately after calling SyncAccessHandle.close
+PASS SyncAccessHandle.getSize fails after SyncAccessHandle.close settles
+PASS SyncAccessHandle.getSize fails immediately after calling SyncAccessHandle.close
+PASS SyncAccessHandle.truncate fails after SyncAccessHandle.close settles
+PASS SyncAccessHandle.truncate fails immediately after calling SyncAccessHandle.close
 

Added: trunk/LayoutTests/storage/filesystemaccess/resources/sync-access-handle-close.js (0 => 284652)


--- trunk/LayoutTests/storage/filesystemaccess/resources/sync-access-handle-close.js	                        (rev 0)
+++ trunk/LayoutTests/storage/filesystemaccess/resources/sync-access-handle-close.js	2021-10-21 23:13:02 UTC (rev 284652)
@@ -0,0 +1,48 @@
+if (this.importScripts) {
+    importScripts('../../../resources/js-test.js');
+}
+
+description("This test checks close() of FileSystemSyncAccessHandle");
+
+var accessHandle, promise;
+
+function finishTest(error)
+{
+    if (error) {
+        testFailed(error);
+    }
+    finishJSTest();
+}
+
+async function testFunctions()
+{
+    shouldThrow("await accessHandle.close()");
+    shouldThrow("await accessHandle.getSize()");
+    shouldThrow("await accessHandle.flush()");
+    shouldThrow("await accessHandle.read(new ArrayBuffer(1), { \"at\" : 0 })");
+    shouldThrow("await accessHandle.write(new ArrayBuffer(1), { \"at\" : 0 })");
+}
+
+async function test() {
+    try {
+        var rootHandle = await navigator.storage.getDirectory();
+        // Create a new file for this test.
+        await rootHandle.removeEntry("sync-access-handle-close.txt").then(() => { }, () => { });
+        var fileHandle = await rootHandle.getFileHandle("sync-access-handle-close.txt", { "create" : true  });
+        accessHandle = await fileHandle.createSyncAccessHandle();
+
+        var closePromise = accessHandle.close();
+        debug("test after invoking close():");
+        testFunctions();
+
+        debug("test after close() is done:");
+        await closePromise;
+        testFunctions();
+
+        finishTest();
+    } catch (error) {
+        finishTest(error);
+    }
+}
+
+test();

Added: trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker-expected.txt (0 => 284652)


--- trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker-expected.txt	2021-10-21 23:13:02 UTC (rev 284652)
@@ -0,0 +1,22 @@
+[Worker] This test checks close() of FileSystemSyncAccessHandle
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Starting worker: resources/sync-access-handle-close.js
+[Worker] test after invoking close():
+PASS [Worker] await accessHandle.close() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.getSize() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.flush() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.read(new ArrayBuffer(1), { "at" : 0 }) threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.write(new ArrayBuffer(1), { "at" : 0 }) threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+[Worker] test after close() is done:
+PASS [Worker] await accessHandle.close() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.getSize() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.flush() threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.read(new ArrayBuffer(1), { "at" : 0 }) threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS [Worker] await accessHandle.write(new ArrayBuffer(1), { "at" : 0 }) threw exception SyntaxError: Unexpected identifier 'accessHandle'.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker.html (0 => 284652)


--- trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker.html	                        (rev 0)
+++ trunk/LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker.html	2021-10-21 23:13:02 UTC (rev 284652)
@@ -0,0 +1,9 @@
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script>
+worker = startWorker('resources/sync-access-handle-close.js');</script>
+</body>
+</html>
\ No newline at end of file

Modified: trunk/Source/WebCore/ChangeLog (284651 => 284652)


--- trunk/Source/WebCore/ChangeLog	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/Source/WebCore/ChangeLog	2021-10-21 23:13:02 UTC (rev 284652)
@@ -1,3 +1,30 @@
+2021-10-21  Sihui Liu  <sihui_...@apple.com>
+
+        FileSystemSyncAccessHandle should close platform file handle on close()
+        https://bugs.webkit.org/show_bug.cgi?id=232067
+        <rdar://problem/84517013>
+
+        Reviewed by Youenn Fablet.
+
+        This patch also ensures no request is sent after close() is called.
+
+        Test: storage/filesystemaccess/sync-access-handle-close-worker.html
+
+        * Modules/filesystemaccess/FileSystemFileHandle.cpp:
+        (WebCore::FileSystemFileHandle::createSyncAccessHandle):
+        * Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:
+        (WebCore::FileSystemSyncAccessHandle::FileSystemSyncAccessHandle):
+        (WebCore::FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle):
+        (WebCore::FileSystemSyncAccessHandle::isClosingOrClosed const):
+        (WebCore::FileSystemSyncAccessHandle::truncate):
+        (WebCore::FileSystemSyncAccessHandle::getSize):
+        (WebCore::FileSystemSyncAccessHandle::flush):
+        (WebCore::FileSystemSyncAccessHandle::close):
+        (WebCore::FileSystemSyncAccessHandle::didClose):
+        (WebCore::FileSystemSyncAccessHandle::read):
+        (WebCore::FileSystemSyncAccessHandle::write):
+        * Modules/filesystemaccess/FileSystemSyncAccessHandle.h:
+
 2021-10-21  Dean Jackson  <d...@apple.com>
 
         [WebXR] WebXR on Cocoa doesn't work with multisampled contexts

Modified: trunk/Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp (284651 => 284652)


--- trunk/Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp	2021-10-21 23:13:02 UTC (rev 284652)
@@ -58,6 +58,9 @@
             return promise.reject(result.releaseException());
 
         auto resultValue = result.releaseReturnValue();
+        if (resultValue.second == FileSystem::invalidPlatformFileHandle)
+            return promise.reject(Exception { UnknownError, "Invalid platform file handle"_s });
+
         promise.settle(FileSystemSyncAccessHandle::create(protectedThis.get(), resultValue.first, resultValue.second));
     });
 }

Modified: trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp (284651 => 284652)


--- trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp	2021-10-21 23:13:02 UTC (rev 284652)
@@ -27,7 +27,6 @@
 #include "FileSystemSyncAccessHandle.h"
 
 #include "BufferSource.h"
-#include "ExceptionOr.h"
 #include "FileSystemFileHandle.h"
 #include "JSDOMPromiseDeferred.h"
 #include <wtf/CompletionHandler.h>
@@ -44,16 +43,28 @@
     , m_identifier(identifier)
     , m_file(file)
 {
+    ASSERT(m_file != FileSystem::invalidPlatformFileHandle);
 }
 
 FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle()
 {
-    if (!m_isClosed)
-        m_source->close(m_identifier, [](auto) { });
+    if (m_closeResult)
+        return;
+
+    ASSERT(m_closePromises.isEmpty());
+    m_source->close(m_identifier, [](auto) { });
 }
 
+bool FileSystemSyncAccessHandle::isClosingOrClosed() const
+{
+    return m_closeResult || !m_closePromises.isEmpty();
+}
+
 void FileSystemSyncAccessHandle::truncate(unsigned long long size, DOMPromiseDeferred<void>&& promise)
 {
+    if (isClosingOrClosed())
+        return promise.reject(Exception { InvalidStateError, "AccessHandle is closing or closed"_s });
+
     m_pendingOperationCount++;
     m_source->truncate(m_identifier, size, [weakThis = WeakPtr { *this }, promise = WTFMove(promise)](auto result) mutable {
         if (weakThis)
@@ -65,6 +76,9 @@
 
 void FileSystemSyncAccessHandle::getSize(DOMPromiseDeferred<IDLUnsignedLongLong>&& promise)
 {
+    if (isClosingOrClosed())
+        return promise.reject(Exception { InvalidStateError, "AccessHandle is closing or closed"_s });
+
     m_pendingOperationCount++;
     m_source->getSize(m_identifier, [weakThis = WeakPtr { *this }, promise = WTFMove(promise)](auto result) mutable {
         if (weakThis)
@@ -76,6 +90,9 @@
 
 void FileSystemSyncAccessHandle::flush(DOMPromiseDeferred<void>&& promise)
 {
+    if (isClosingOrClosed())
+        return promise.reject(Exception { InvalidStateError, "AccessHandle is closing or closed"_s });
+
     m_pendingOperationCount++;
     m_source->flush(m_identifier, [weakThis = WeakPtr { *this }, promise = WTFMove(promise)](auto result) mutable {
         if (weakThis)
@@ -87,23 +104,31 @@
 
 void FileSystemSyncAccessHandle::close(DOMPromiseDeferred<void>&& promise)
 {
-    if (m_isClosed)
-        return promise.reject(Exception { InvalidStateError });
+    if (m_closeResult)
+        return promise.settle(ExceptionOr<void> { *m_closeResult });
+    
+    auto isClosing = !m_closePromises.isEmpty();
+    m_closePromises.append(WTFMove(promise));
+    if (isClosing)
+        return;
 
+    FileSystem::closeFile(m_file);
+    m_file = FileSystem::invalidPlatformFileHandle;
+
     m_pendingOperationCount++;
-    m_source->close(m_identifier, [weakThis = WeakPtr { *this }, promise = WTFMove(promise)](auto result) mutable {
-        if (weakThis) {
-            weakThis->m_pendingOperationCount--;
-            weakThis->didClose();
-        }
-
-        promise.settle(WTFMove(result));
+    m_source->close(m_identifier, [this, protectedThis = Ref { *this }](auto result) mutable {
+        m_pendingOperationCount--;
+        didClose(WTFMove(result));
     });
 }
 
-void FileSystemSyncAccessHandle::didClose()
+void FileSystemSyncAccessHandle::didClose(ExceptionOr<void>&& result)
 {
-    m_isClosed = true;
+    m_closeResult = WTFMove(result);
+
+    auto promises = std::exchange(m_closePromises, { });
+    for (auto promise : promises)
+        promise.settle(ExceptionOr<void> { *m_closeResult });
 }
 
 ExceptionOr<unsigned long long> FileSystemSyncAccessHandle::read(BufferSource&& buffer, FileSystemSyncAccessHandle::FilesystemReadWriteOptions options)
@@ -110,8 +135,8 @@
 {
     ASSERT(!isMainThread());
 
-    if (m_file == FileSystem::invalidPlatformFileHandle || m_isClosed)
-        return Exception { InvalidStateError };
+    if (isClosingOrClosed())
+        return Exception { InvalidStateError, "AccessHandle is closing or closed"_s };
 
     if (m_pendingOperationCount)
         return Exception { InvalidStateError, "Access handle has unfinished operation"_s };
@@ -131,8 +156,8 @@
 {
     ASSERT(!isMainThread());
 
-    if (m_file == FileSystem::invalidPlatformFileHandle || m_isClosed)
-        return Exception { InvalidStateError };
+    if (isClosingOrClosed())
+        return Exception { InvalidStateError, "AccessHandle is closing or closed"_s };
 
     if (m_pendingOperationCount)
         return Exception { InvalidStateError, "Access handle has unfinished operation"_s };

Modified: trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.h (284651 => 284652)


--- trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.h	2021-10-21 22:45:19 UTC (rev 284651)
+++ trunk/Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.h	2021-10-21 23:13:02 UTC (rev 284652)
@@ -26,6 +26,7 @@
 #pragma once
 
 #include "BufferSource.h"
+#include "ExceptionOr.h"
 #include "FileSystemSyncAccessHandleIdentifier.h"
 #include "IDLTypes.h"
 #include <wtf/FileSystem.h>
@@ -35,7 +36,6 @@
 
 class FileSystemFileHandle;
 template<typename> class DOMPromiseDeferred;
-template<typename> class ExceptionOr;
 
 class FileSystemSyncAccessHandle : public RefCounted<FileSystemSyncAccessHandle>, public CanMakeWeakPtr<FileSystemSyncAccessHandle> {
 public:
@@ -50,18 +50,20 @@
     void getSize(DOMPromiseDeferred<IDLUnsignedLongLong>&&);
     void flush(DOMPromiseDeferred<void>&&);
     void close(DOMPromiseDeferred<void>&&);
-    void didClose();
+    void didClose(ExceptionOr<void>&&);
     ExceptionOr<unsigned long long> read(BufferSource&&, FilesystemReadWriteOptions);
     ExceptionOr<unsigned long long> write(BufferSource&&, FilesystemReadWriteOptions);
 
 private:
     FileSystemSyncAccessHandle(FileSystemFileHandle&, FileSystemSyncAccessHandleIdentifier, FileSystem::PlatformFileHandle);
+    bool isClosingOrClosed() const;
 
     Ref<FileSystemFileHandle> m_source;
     FileSystemSyncAccessHandleIdentifier m_identifier;
-    bool m_isClosed { false };
     uint64_t m_pendingOperationCount { 0 };
     FileSystem::PlatformFileHandle m_file;
+    std::optional<ExceptionOr<void>> m_closeResult;
+    Vector<DOMPromiseDeferred<void>> m_closePromises;
 };
 
 } // namespace WebCore
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to