Bug 232146 - Perform FileSystemSyncAccessHandle operations in web process
Summary: Perform FileSystemSyncAccessHandle operations in web process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 231706
  Show dependency treegraph
 
Reported: 2021-10-22 08:39 PDT by Sihui Liu
Modified: 2021-11-10 00:20 PST (History)
4 users (show)

See Also:


Attachments
Patch (50.80 KB, patch)
2021-10-27 16:12 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (51.55 KB, patch)
2021-10-27 16:22 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (49.44 KB, patch)
2021-10-29 00:20 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (50.91 KB, patch)
2021-11-01 15:43 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.93 KB, patch)
2021-11-01 17:11 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.93 KB, patch)
2021-11-01 17:23 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.52 KB, patch)
2021-11-01 17:56 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.51 KB, patch)
2021-11-01 18:44 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (51.49 KB, patch)
2021-11-09 23:21 PST, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-10-22 08:39:31 PDT
Then only web process needs to manage the file handle.
Comment 1 Sihui Liu 2021-10-27 16:12:37 PDT
Created attachment 442645 [details]
Patch
Comment 2 Sihui Liu 2021-10-27 16:22:23 PDT
Created attachment 442647 [details]
Patch
Comment 3 Geoffrey Garen 2021-10-27 16:30:59 PDT
Seems like a nice approach.
Comment 4 youenn fablet 2021-10-28 00:23:25 PDT
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.
Comment 5 Sihui Liu 2021-10-29 00:20:48 PDT
Created attachment 442789 [details]
Patch
Comment 6 youenn fablet 2021-10-29 01:21:24 PDT
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.
Comment 7 Radar WebKit Bug Importer 2021-10-29 08:40:18 PDT
<rdar://problem/84809428>
Comment 8 Sihui Liu 2021-11-01 15:43:07 PDT
Created attachment 443025 [details]
Patch
Comment 9 Sihui Liu 2021-11-01 17:11:13 PDT
Created attachment 443039 [details]
Patch
Comment 10 Sihui Liu 2021-11-01 17:23:59 PDT
Created attachment 443040 [details]
Patch
Comment 11 Sihui Liu 2021-11-01 17:56:59 PDT
Created attachment 443045 [details]
Patch
Comment 12 Sihui Liu 2021-11-01 18:44:16 PDT
Created attachment 443050 [details]
Patch
Comment 13 youenn fablet 2021-11-09 10:12:04 PST
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
Comment 14 Sihui Liu 2021-11-09 23:21:38 PST
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.
Comment 15 Sihui Liu 2021-11-09 23:21:50 PST
Created attachment 443778 [details]
Patch for landing
Comment 16 EWS 2021-11-10 00:20:38 PST
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].