Bug 234271

Summary: REGRESSION (r286601): storage/filesystemaccess/sync-access-handle-read-write-worker.html and file-system-access/sandboxed_FileSystemSyncAccessHandle-truncate.https.tentative.worker.html are consistently failing
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: WebCore Misc.Assignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ews-watchlist, gyuyoung.kim, jenner, ryuan.choi, sergio, sihui_liu, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233567
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch ews-feeder: commit-queue-

Description Ryan Haddad 2021-12-13 14:27:16 PST
storage/filesystemaccess/sync-access-handle-read-write-worker.html and imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-truncate.https.tentative.worker.html are consistently failing on bots after https://trac.webkit.org/changeset/286601/webkit

--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/storage/filesystemaccess/sync-access-handle-read-write-worker-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/storage/filesystemaccess/sync-access-handle-read-write-worker-actual.txt
@@ -4,26 +4,9 @@
 
 
 Starting worker: resources/sync-access-handle-read-write.js
-PASS [Worker] fileSize is 0
-[Worker] test read() and write():
-PASS [Worker] writeSize is writeBuffer.byteLength
-PASS [Worker] readSize is readBuffer.byteLength
-PASS [Worker] readText is "This is first sentence."
-PASS [Worker] writeSize is writeBuffer.byteLength
-PASS [Worker] readSize is readBuffer.byteLength
-PASS [Worker] readText is "This is second sentence."
-PASS [Worker] readSize is readBuffer.byteLength
-PASS [Worker] readText is "This is first sentence.This is second sentence."
-PASS [Worker] accessHandle.read(new ArrayBuffer(1), { "at" : Number.MAX_SAFE_INTEGER }) threw exception InvalidStateError: Failed to read at offset.
-PASS [Worker] accessHandle.write(new ArrayBuffer(1), { "at" : Number.MAX_SAFE_INTEGER }) threw exception InvalidStateError: Failed to write at offset.
-[Worker] test flush():
-PASS [Worker] fileSize is totalWriteSize
-[Worker] test truncate():
-PASS [Worker] fileSize is 4
-[Worker] test write() with pending operation:
-[Worker] accessHandle.truncate(0)
-PASS [Worker] accessHandle.read(readBuffer, { "at" : 0 }) threw exception InvalidStateError: Access handle has unfinished operation.
+FAIL [Worker] InvalidStateError: The object is in an invalid state.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
 

--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-truncate.https.tentative.worker-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-truncate.https.tentative.worker-actual.txt
@@ -1,5 +1,5 @@
 
-FAIL test createSyncAccessHandle.truncate with pending operation assert_unreached: Should have rejected: undefined Reached unreachable code
-PASS test SyncAccessHandle.truncate with different sizes
-PASS test SyncAccessHandle.truncate after SyncAccessHandle.write
+FAIL test createSyncAccessHandle.truncate with pending operation promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
+FAIL test SyncAccessHandle.truncate with different sizes promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
+FAIL test SyncAccessHandle.truncate after SyncAccessHandle.write promise_test: Unhandled rejection with value: object "InvalidStateError: The object is in an invalid state."
 

https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r286602%20(6850)/results.html
Comment 1 Radar WebKit Bug Importer 2021-12-13 14:27:47 PST
<rdar://problem/86434111>
Comment 3 Robert Jenner 2021-12-13 14:35:30 PST
*** Bug 234267 has been marked as a duplicate of this bug. ***
Comment 4 Sihui Liu 2021-12-14 01:03:14 PST
Created attachment 447115 [details]
Patch
Comment 5 youenn fablet 2021-12-14 09:49:08 PST
Comment on attachment 447115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447115&action=review

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:60
> +        promise.resolve(FileSystemFileHandle::create(context, String { name }, result.returnValue(), WTFMove(connection)));

I am not sure we can guarantee that the task given to workerRunLoop.postTask in WorkerFileSystemStorageConnection::getFileHandle/getDirectoryHandle will always be executed.
Say we called worker.stop() and a few milliseconds later we call workerRunLoop::postTask.

Maybe the best approach would be for FileSystemStorageConnection::getFileHandle/getDirectoryHandle to return in the completion handler an allocated object that would have the sync handle identifier and a weakref to the IPC connection.
This object would, when destroyed, call the sync handle using connection and identifier.
Then either FileSystemFileHandle would store this object until destroyed, or FileSystemFileHandle could steal the identifier and clear the connection so that the object destructor would do nothing in that case.
Comment 6 Sihui Liu 2021-12-14 16:27:52 PST
Created attachment 447174 [details]
Patch
Comment 7 youenn fablet 2021-12-15 07:11:50 PST
Comment on attachment 447174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447174&action=review

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:37
> +class FileSystemHandleCloseScope : public ThreadSafeRefCounted<FileSystemHandleCloseScope> {

Not sure it needs to be refcounted, maybe we can just have a unique_ptr>
Also, maybe we should expose this object in WebCore and callbacks with a needed WebProcess specialisation to do the clean-up.
That way, we would not need to modify the code to allow FileSystemFileHandle creation with a null context in WebCore.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:47
> +            m_connection->closeHandle(m_identifier);

I am not sure closeHandle is expected to be called from a non main thread.
You might fix this by explicitly call callOnMainThread, or make ThreadSafeRefCounted destruction main run loop.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:168
> +                    return;

We are doing twice the same "is scope closed" here and in the WebCore callback.
Comment 8 Sihui Liu 2021-12-15 23:53:06 PST
Created attachment 447331 [details]
Patch
Comment 9 Sihui Liu 2021-12-16 00:02:32 PST
Created attachment 447332 [details]
Patch
Comment 10 Sihui Liu 2021-12-16 00:34:57 PST
Created attachment 447335 [details]
Patch
Comment 11 Sihui Liu 2021-12-16 09:26:47 PST
Created attachment 447363 [details]
Patch
Comment 12 EWS 2021-12-16 14:02:04 PST
Committed r287156 (245333@main): <https://commits.webkit.org/245333@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447363 [details].