Bug 232146 – Perform FileSystemSyncAccessHandle operations in web process

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

Then only web process needs to manage the file handle.

[Sihui Liu]

Created attachment 442645 [details]
Patch

[Sihui Liu]

Created attachment 442647 [details]
Patch

[Geoffrey Garen]

Seems like a nice approach.

[youenn fablet]

Comment on attachment 442647 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        truncate(), getSize() and flush() operations will actually be sync operations.

For read/write, we have to block but for other methods, we should try not to block.
I would tend to implement the meat of these operations in a single WorkerGlobalScope work queue and use promises to signal when we are back from the work queue.

> Source/WebKit/ChangeLog:9
> +        file handle, pass it to web process and close it.

We probably need to have a dedicated network process crash handling (in this patch or as a follow-up) since the WebProcess sync handle might continue working while the new network process will not know of this pre-existing sync handle.

[Sihui Liu]

Created attachment 442789 [details]
Patch

[youenn fablet]

Comment on attachment 442789 [details]
Patch

Looks almost ready, I'd like to understand whether close should also hop to the work queue.

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

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:75
> +            result = Exception { UnknownError };

It seems we could just return success here and create the Exception Object in worker as needed.
Ditto for the other methods.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:83
> +        workerThread->runLoop().postTask(WTFMove(task));

I do not think we need a specific task:
workerThread->runLoop().postTask([...] { ... {); should work just fine.
Ditto below.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159
> +    for (auto& promise : pendingPromises) {

Is that correct?
I would have expected that close would be queued on the work queue so that the following would actually work fine:
handle.flush(); // no await
handle.close(); // flush first, then close.

To make it work, that would mean calling closeFile within a WorkerGlobalScope::postFileSystemStorageTask task.

In any case, we should be able to write a test and validate this with other implementations.

[Radar WebKit Bug Importer]

<rdar://problem/84809428>

[Sihui Liu]

Created attachment 443025 [details]
Patch

[Sihui Liu]

Created attachment 443039 [details]
Patch

[Sihui Liu]

Created attachment 443040 [details]
Patch

[Sihui Liu]

Created attachment 443045 [details]
Patch

[Sihui Liu]

Created attachment 443050 [details]
Patch

[youenn fablet]

Comment on attachment 443050 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159
> +{

Might be worth adding ASSERT(!isClosingOrClosed()) since we are calling this from several places.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:163
> +        });

return here to remove the else.

> Source/WebCore/workers/WorkerGlobalScope.cpp:82
> +    static NeverDestroyed<Ref<WorkQueue>> queue(WorkQueue::create("Shared File System Storage Queue",  WorkQueue::QOS::Utility));

Not really sure what the best QOS value is. On one hand, we want to be fast but we do not want too many fast queues.
Maybe default is fine here.

> Source/WebCore/workers/WorkerGlobalScope.cpp:83
> +    return queue.get().copyRef();

Seems wasteful to return a Ref<>. We could return a WorQueue&, or define the static queue directly in WorkerGlobalScope::postFileSystemStorageTask

[Sihui Liu]

Comment on attachment 443050 [details]
Patch

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

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:159
>> +{
> 
> Might be worth adding ASSERT(!isClosingOrClosed()) since we are calling this from several places.

hmmm closeBackend() can happen after isClosingOrClosed is true; see FileSystemSyncAccessHandle::~FileSystemSyncAccessHandle().

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:163
>> +        });
> 
> return here to remove the else.

Sure.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:82
>> +    static NeverDestroyed<Ref<WorkQueue>> queue(WorkQueue::create("Shared File System Storage Queue",  WorkQueue::QOS::Utility));
> 
> Not really sure what the best QOS value is. On one hand, we want to be fast but we do not want too many fast queues.
> Maybe default is fine here.

Okay, will change to Default.

>> Source/WebCore/workers/WorkerGlobalScope.cpp:83
>> +    return queue.get().copyRef();
> 
> Seems wasteful to return a Ref<>. We could return a WorQueue&, or define the static queue directly in WorkerGlobalScope::postFileSystemStorageTask

Sure.

[Sihui Liu]

Created attachment 443778 [details]
Patch for landing

[EWS]

Committed r285566 (244074@main): <https://commits.webkit.org/244074@main>

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