Bug 230861 - Replace FileSystemHandleImpl with FileSystemStorageConnection
Summary: Replace FileSystemHandleImpl with FileSystemStorageConnection
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-09-27 15:21 PDT by Sihui Liu
Modified: 2021-10-13 18:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (69.85 KB, patch)
2021-09-27 20:35 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (69.87 KB, patch)
2021-09-27 21:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (69.42 KB, patch)
2021-09-28 11:30 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (70.14 KB, patch)
2021-09-28 16:02 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (96.88 KB, patch)
2021-09-29 14:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (96.88 KB, patch)
2021-09-29 14:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (96.72 KB, patch)
2021-09-29 14:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (96.73 KB, patch)
2021-09-29 14:39 PDT, 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-09-27 15:21:07 PDT
...
Comment 1 Sihui Liu 2021-09-27 20:35:13 PDT
Created attachment 439428 [details]
Patch
Comment 2 Sihui Liu 2021-09-27 21:15:46 PDT
Created attachment 439430 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-09-27 21:49:39 PDT
<rdar://problem/83606465>
Comment 4 youenn fablet 2021-09-28 09:00:53 PDT
Comment on attachment 439430 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:40
> +    , m_connection(connection)

WTFMove

> Source/WebCore/Modules/storage/StorageManager.cpp:113
> +        promise.resolve(FileSystemDirectoryHandle::create({ }, result.releaseReturnValue(), Ref { *connection }));

WTFMove

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:302
> +        m_fileSystemStorageConnection = WebFileSystemStorageConnection::create(m_connection.get());

I am not sure this is the right place to have good network process crash recovery behaviour.
WebFileSystemStorageConnection might be kept alive by its File handles after NetworkProcessConnection is gone.
One possibility is to have one global WebFileSystemStorageConnection which would make sure to react upon network process connection crash (for instance by getting a connection everytime it is needed or by caching an IPC connection and resetting it to nullptr on crash).
Comment 5 Sihui Liu 2021-09-28 09:32:00 PDT
Comment on attachment 439430 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:302
>> +        m_fileSystemStorageConnection = WebFileSystemStorageConnection::create(m_connection.get());
> 
> I am not sure this is the right place to have good network process crash recovery behaviour.
> WebFileSystemStorageConnection might be kept alive by its File handles after NetworkProcessConnection is gone.
> One possibility is to have one global WebFileSystemStorageConnection which would make sure to react upon network process connection crash (for instance by getting a connection everytime it is needed or by caching an IPC connection and resetting it to nullptr on crash).

WebFileSystemStorageConnection does hold reference to IPC connection and set it null on connection close now? Or do you mean something different?
Comment 6 Sihui Liu 2021-09-28 11:30:06 PDT
Created attachment 439499 [details]
Patch
Comment 7 Sihui Liu 2021-09-28 16:02:22 PDT
Created attachment 439539 [details]
Patch
Comment 8 youenn fablet 2021-09-29 09:16:49 PDT
Comment on attachment 439539 [details]
Patch

Looks good to me. If possible, it would be good to see if we can simplify the ID management to not do any conversion, either here or as a follow-up.

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

> Source/WebCore/Modules/storage/StorageConnection.h:44
> +    using GetDirectoryCallback = CompletionHandler<void(ExceptionOr<FileSystemHandleIdentifier>&&, RefPtr<FileSystemStorageConnection>&&)>;

I guess it could be an ExceptionOr<std::pair<FileSystemHandleIdentifier, Ref<FileSystemStorageConnection>>>.
Or maybe the callsite could retrieve the connection somehow to just return the id.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:55
> +        return completionHandler(WebCore::Exception { WebCore::UnknownError, "Connection is lost" });

For now, I would do:
if (!m_connection)
    m_connection = WebProcess::singleton()....

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:64
> +    });

Can you pass the completionHandler directly?

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:81
> +        completionHandler(makeObjectIdentifier<WebCore::FileSystemHandleIdentifierType>(storageIdentifier.toUInt64()));

Do we need two different types, FileSystemHandleIdentifier and FileSystemStorageHandleIdentifier?
It would be convenient to just use one if we can.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:49
> +    WebFileSystemStorageConnection(IPC::Connection&);

explicit

> Source/WebKit/WebProcess/WebProcess.cpp:1215
> +        m_fileSystemStorageConnection = nullptr;

We might not need to set it to nullptr if we get a new IPC::Connection directly in m_fileSystemStorageConnection
Comment 9 Sihui Liu 2021-09-29 10:57:19 PDT
Comment on attachment 439539 [details]
Patch

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

>> Source/WebCore/Modules/storage/StorageConnection.h:44
>> +    using GetDirectoryCallback = CompletionHandler<void(ExceptionOr<FileSystemHandleIdentifier>&&, RefPtr<FileSystemStorageConnection>&&)>;
> 
> I guess it could be an ExceptionOr<std::pair<FileSystemHandleIdentifier, Ref<FileSystemStorageConnection>>>.
> Or maybe the callsite could retrieve the connection somehow to just return the id.

I will change it to pair.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:55
>> +        return completionHandler(WebCore::Exception { WebCore::UnknownError, "Connection is lost" });
> 
> For now, I would do:
> if (!m_connection)
>     m_connection = WebProcess::singleton()....

Like I said before, the identifier of handle is only meaningful to one network process. If we launch a new network process, the new network process won't recognize the identifiers and the operations will still fail.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:64
>> +    });
> 
> Can you pass the completionHandler directly?

Sure.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:81
>> +        completionHandler(makeObjectIdentifier<WebCore::FileSystemHandleIdentifierType>(storageIdentifier.toUInt64()));
> 
> Do we need two different types, FileSystemHandleIdentifier and FileSystemStorageHandleIdentifier?
> It would be convenient to just use one if we can.

I guess we can just use FileSystemHandleIdentifier. I will do a follow-up on this: https://bugs.webkit.org/show_bug.cgi?id=230965

>> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:49
>> +    WebFileSystemStorageConnection(IPC::Connection&);
> 
> explicit

Sure

>> Source/WebKit/WebProcess/WebProcess.cpp:1215
>> +        m_fileSystemStorageConnection = nullptr;
> 
> We might not need to set it to nullptr if we get a new IPC::Connection directly in m_fileSystemStorageConnection

Same reason as above: we don't want to launch a new network process to handle operations on existing connections.
Comment 10 Sihui Liu 2021-09-29 14:15:04 PDT
Created attachment 439656 [details]
Patch for landing
Comment 11 Sihui Liu 2021-09-29 14:15:25 PDT
Created attachment 439657 [details]
Patch for landing
Comment 12 Sihui Liu 2021-09-29 14:16:40 PDT
Created attachment 439658 [details]
Patch for landing
Comment 13 Sihui Liu 2021-09-29 14:39:11 PDT
Created attachment 439661 [details]
Patch for landing
Comment 14 EWS 2021-09-29 15:45:30 PDT
Committed r283271 (242300@main): <https://commits.webkit.org/242300@main>

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