Bug 234271 – 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

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Radar WebKit Bug Importer]

<rdar://problem/86434111>

[Ryan Haddad]

https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Ffile-system-access%2Fsandboxed_FileSystemSyncAccessHandle-truncate.https.tentative.worker.html&test=storage%2Ffilesystemaccess%2Fsync-access-handle-read-write-worker.html

[Robert Jenner]

*** Bug 234267 has been marked as a duplicate of this bug. ***

[Sihui Liu]

Created attachment 447115 [details]
Patch

[youenn fablet]

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.

[Sihui Liu]

Created attachment 447174 [details]
Patch

[youenn fablet]

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.

[Sihui Liu]

Created attachment 447331 [details]
Patch

[Sihui Liu]

Created attachment 447332 [details]
Patch

[Sihui Liu]

Created attachment 447335 [details]
Patch

[Sihui Liu]

Created attachment 447363 [details]
Patch

[EWS]

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].