Make all FileSystemSyncAccessHandle methods sync #7972

szewai · 2022-12-21T20:24:07Z

`acd30cd`

Make all FileSystemSyncAccessHandle methods sync
https://bugs.webkit.org/show_bug.cgi?id=247071
rdar://problem/101620396

Reviewed by Youenn Fablet.

To match latest spec: https://fs.spec.whatwg.org/#api-filesystemsyncaccesshandle. Chrome has already shipped it.

* LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-flush.https.tentative.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-getSize.https.tentative.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-truncate.https.tentative.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fs/idlharness.https.any.worker-expected.txt:
* LayoutTests/storage/filesystemaccess/resources/sync-access-handle-basics.js:
* LayoutTests/storage/filesystemaccess/resources/sync-access-handle-close.js:
(testFunction):
(testFunctions):
(async testMultipleHandles):
(async test):
(testSyncFunction): Deleted.
(async testAsyncFunction): Deleted.
(async testFunctions): Deleted.
* LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js:
(async test):
* LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker-expected.txt:
* LayoutTests/storage/filesystemaccess/sync-access-handle-read-write-worker-expected.txt:
* Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp:
(WebCore::FileSystemFileHandle::createSyncAccessHandle):
(WebCore::FileSystemFileHandle::closeSyncAccessHandle):
* Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.h:
* Source/WebCore/Modules/filesystemaccess/FileSystemStorageConnection.h:
* Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:
(WebCore::FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle):
(WebCore::FileSystemSyncAccessHandle::truncate):
(WebCore::FileSystemSyncAccessHandle::getSize):
(WebCore::FileSystemSyncAccessHandle::flush):
(WebCore::FileSystemSyncAccessHandle::close):
(WebCore::FileSystemSyncAccessHandle::closeInternal):
(WebCore::FileSystemSyncAccessHandle::read):
(WebCore::FileSystemSyncAccessHandle::write):
(WebCore::FileSystemSyncAccessHandle::stop):
(WebCore::FileSystemSyncAccessHandle::invalidate):
(WebCore::FileSystemSyncAccessHandle::isClosingOrClosed const): Deleted.
(WebCore::FileSystemSyncAccessHandle::closeFile): Deleted.
(WebCore::FileSystemSyncAccessHandle::didCloseFile): Deleted.
(WebCore::FileSystemSyncAccessHandle::closeBackend): Deleted.
(WebCore::FileSystemSyncAccessHandle::didCloseBackend): Deleted.
(WebCore::FileSystemSyncAccessHandle::completePromise): Deleted.
* Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.h:
* Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.idl:
* Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:
(WebCore::WorkerFileSystemStorageConnection::closeSyncAccessHandle):
* Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:
* Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:
(WebKit::NetworkStorageManager::closeSyncAccessHandle):
* Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:
* Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in:
* Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:
(WebKit::WebFileSystemStorageConnection::closeSyncAccessHandle):
* Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:

Canonical link: https://commits.webkit.org/258473@main

f82b26e

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-04T02:04:36Z

EWS run on previous version of this PR (hash 64ab51f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

youennf

LGTM.
Please check whether close should block until the job is done in network process.
Some other nits below.

youennf · 2023-01-04T08:19:40Z

...c/web-platform-tests/fs/FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt

+FAIL SyncAccessHandle.write fails after SyncAccessHandle.close promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"
+FAIL SyncAccessHandle.flush fails after SyncAccessHandle.close promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"
+FAIL SyncAccessHandle.getSize fails after SyncAccessHandle.close promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"
+FAIL SyncAccessHandle.truncate fails after SyncAccessHandle.handle.close promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"


Should we file a bug to fix these?

Yes; will file.

youennf · 2023-01-04T08:25:18Z

Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp

-        });
-    });
+    bool succeeded = FileSystem::truncateFile(m_file.handle(), size);
+    return succeeded ? ExceptionOr<void> { } : Exception { UnknownError };


https://fs.spec.whatwg.org/#api-filesystemsyncaccesshandle-truncate is using InvalidStateError, maybe we should use this one with a message.
This may apply to other methods.

Sounds good; will update.

youennf · 2023-01-04T08:28:33Z

Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp

-    if (isClosingOrClosed())
-        return promise.reject(Exception { InvalidStateError, "AccessHandle is closing or closed"_s });
+    if (m_isClosed)
+        return Exception { InvalidStateError, "AccessHandle is closed"_s };

    auto* scope = downcast<WorkerGlobalScope>(scriptExecutionContext());
    if (!scope)


Pre-existing issue but we do not use scope, do we need the downcast and the if check (here and below)?

Yes, we probably should remove it now that they are unused; will do.

youennf · 2023-01-04T08:30:08Z

Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp

-    for (auto& callback : callbacks)
-        callback(ExceptionOr<void> { *m_closeResult });
+    if (shouldNotifyBackend == ShouldNotifyBackend::Yes)
+        m_source->closeSyncAccessHandle(m_identifier);
 }

 ExceptionOr<unsigned long long> FileSystemSyncAccessHandle::read(BufferSource&& buffer, FileSystemSyncAccessHandle::FilesystemReadWriteOptions options)
 {
    ASSERT(!isMainThread());


We can either add these asserts to the other methods or remove these asserts since all methods are doing IO now in the same thread.

Sure; will remove.

youennf · 2023-01-04T08:31:45Z

Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp


-        mainThreadConnection->closeSyncAccessHandle(identifier, accessHandleIdentifier, WTFMove(mainThreadCallback));
+    callOnMainThread([workerThread = Ref { m_scope->thread() }, mainThreadConnection = m_mainThreadConnection, identifier, accessHandleIdentifier]() mutable {


Do we need workerThread?

youennf · 2023-01-04T08:33:47Z

Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp

-void FileSystemSyncAccessHandle::didCloseFile()
-{
-    closeBackend(CloseMode::Async);
+    closeInternal(ShouldNotifyBackend::Yes);


In case of ShouldNotifyBackend::Yes, should we block the worker thread until getting the notification from the backend?
If we do not block, could there be race conditions happening within the web app (say the web app has two workers doing IO?)

Could you elaborate on the race condition case? A file can only have one FileSystemSyncAccessHandle at a time.

I am thinking two web workers, the first one calls close and posts a message to the second one which immediately creates a sync handle on the same file entry.
A race condition between the close and the creation could theoretically happen although I am not sure this can happens today.
Say if the two web workers are in different processes and the postMessage does not go through the networking process.

I see, so the theoretical case is

worker1 closes access handle, sends message "close access handle" to network process and sends message "start" to worker2

worker2 receives the "start" message, and sends message "create access handle" to network process

network process receives "create access handle" message from worker 1 before "close access handle" from worker, and will fail the "create access handle" message

Is it possible that two web processes can talk to each other without going through network process? (The SyncAccessHandle is only available in DedicatedWorker now)

In the past postMessage was through UIProcess, now it is going through NetworkProcess, though it might be handled in a different thread than file sync.
WebLock is another possibility and is UIProcess based IIRC.

It feels like a rare case that network process will handle "create" message before the "close" message, as the "close access handle" message will be sent directly from web process 1 to network process, and the "create access handle" message from web process 2 will be sent after:

UI process receives the "release lock" message from web process 1

web process 2 receives "acquire lock" message from UI process

We could make the change if it turns out to be an issue I guess; blocking worker thread on close seems not too good for performance.

Agreed this is rare, but this is the kind of issues that are very hard to diagnose.
Could you check what Chrome is doing here?

The spec is loose in the definition of close, it seems your interpretation might be correct since the only explicit step is to set the state. It might be worth having a more precise definition of close.

It seems Chrome waits until close message is replied; I will make the change to be compatible (https://bugs.webkit.org/show_bug.cgi?id=250194).

I filed whatwg/fs#83 to keep track of this on spec side.

webkit-early-warning-system · 2023-01-04T21:40:41Z

EWS run on current version of this PR (hash f82b26e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=247071 rdar://problem/101620396 Reviewed by Youenn Fablet. To match latest spec: https://fs.spec.whatwg.org/#api-filesystemsyncaccesshandle. Chrome has already shipped it. * LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-flush.https.tentative.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-getSize.https.tentative.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fs/FileSystemSyncAccessHandle-truncate.https.tentative.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fs/idlharness.https.any.worker-expected.txt: * LayoutTests/storage/filesystemaccess/resources/sync-access-handle-basics.js: * LayoutTests/storage/filesystemaccess/resources/sync-access-handle-close.js: (testFunction): (testFunctions): (async testMultipleHandles): (async test): (testSyncFunction): Deleted. (async testAsyncFunction): Deleted. (async testFunctions): Deleted. * LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js: (async test): * LayoutTests/storage/filesystemaccess/sync-access-handle-close-worker-expected.txt: * LayoutTests/storage/filesystemaccess/sync-access-handle-read-write-worker-expected.txt: * Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp: (WebCore::FileSystemFileHandle::createSyncAccessHandle): (WebCore::FileSystemFileHandle::closeSyncAccessHandle): * Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.h: * Source/WebCore/Modules/filesystemaccess/FileSystemStorageConnection.h: * Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp: (WebCore::FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle): (WebCore::FileSystemSyncAccessHandle::truncate): (WebCore::FileSystemSyncAccessHandle::getSize): (WebCore::FileSystemSyncAccessHandle::flush): (WebCore::FileSystemSyncAccessHandle::close): (WebCore::FileSystemSyncAccessHandle::closeInternal): (WebCore::FileSystemSyncAccessHandle::read): (WebCore::FileSystemSyncAccessHandle::write): (WebCore::FileSystemSyncAccessHandle::stop): (WebCore::FileSystemSyncAccessHandle::invalidate): (WebCore::FileSystemSyncAccessHandle::isClosingOrClosed const): Deleted. (WebCore::FileSystemSyncAccessHandle::closeFile): Deleted. (WebCore::FileSystemSyncAccessHandle::didCloseFile): Deleted. (WebCore::FileSystemSyncAccessHandle::closeBackend): Deleted. (WebCore::FileSystemSyncAccessHandle::didCloseBackend): Deleted. (WebCore::FileSystemSyncAccessHandle::completePromise): Deleted. * Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.h: * Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.idl: * Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp: (WebCore::WorkerFileSystemStorageConnection::closeSyncAccessHandle): * Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h: * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp: (WebKit::NetworkStorageManager::closeSyncAccessHandle): * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h: * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.messages.in: * Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp: (WebKit::WebFileSystemStorageConnection::closeSyncAccessHandle): * Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h: Canonical link: https://commits.webkit.org/258473@main

webkit-commit-queue · 2023-01-05T05:10:35Z

Committed 258473@main (acd30cd): https://commits.webkit.org/258473@main

Reviewed commits have been landed. Closing PR #7972 and removing active labels.

szewai requested a review from cdumez as a code owner December 21, 2022 20:24

szewai self-assigned this Dec 21, 2022

szewai added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Dec 21, 2022

szewai force-pushed the eng/Make-all-FileSystemSyncAccessHandle-methods-sync branch from c1b9d31 to 64ab51f Compare January 4, 2023 02:04

szewai requested a review from youennf January 4, 2023 07:06

youennf approved these changes Jan 4, 2023

View reviewed changes

szewai force-pushed the eng/Make-all-FileSystemSyncAccessHandle-methods-sync branch from 64ab51f to f82b26e Compare January 4, 2023 21:38

szewai added the merge-queue Applied to send a pull request to merge-queue label Jan 5, 2023

webkit-early-warning-system force-pushed the eng/Make-all-FileSystemSyncAccessHandle-methods-sync branch from f82b26e to acd30cd Compare January 5, 2023 05:10

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 5, 2023

webkit-early-warning-system merged commit acd30cd into WebKit:main Jan 5, 2023

szewai deleted the eng/Make-all-FileSystemSyncAccessHandle-methods-sync branch January 10, 2023 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all FileSystemSyncAccessHandle methods sync #7972

Make all FileSystemSyncAccessHandle methods sync #7972

szewai commented Dec 21, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 4, 2023 •

edited

youennf left a comment

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 4, 2023

szewai Jan 4, 2023

youennf Jan 5, 2023

szewai Jan 6, 2023

youennf Jan 6, 2023

webkit-early-warning-system commented Jan 4, 2023 •

edited

webkit-commit-queue commented Jan 5, 2023


		mainThreadConnection->closeSyncAccessHandle(identifier, accessHandleIdentifier, WTFMove(mainThreadCallback));
		callOnMainThread([workerThread = Ref { m_scope->thread() }, mainThreadConnection = m_mainThreadConnection, identifier, accessHandleIdentifier]() mutable {

Make all FileSystemSyncAccessHandle methods sync #7972

Make all FileSystemSyncAccessHandle methods sync #7972

Conversation

szewai commented Dec 21, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 4, 2023 • edited

youennf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 4, 2023 • edited

webkit-commit-queue commented Jan 5, 2023

szewai commented Dec 21, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 4, 2023 •

edited

webkit-early-warning-system commented Jan 4, 2023 •

edited