Bug 230861 – Replace FileSystemHandleImpl with FileSystemStorageConnection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 439428 [details]
Patch

[Sihui Liu]

Created attachment 439430 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83606465>

[youenn fablet]

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

[Sihui Liu]

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?

[Sihui Liu]

Created attachment 439499 [details]
Patch

[Sihui Liu]

Created attachment 439539 [details]
Patch

[youenn fablet]

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

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 439656 [details]
Patch for landing

[Sihui Liu]

Created attachment 439657 [details]
Patch for landing

[Sihui Liu]

Created attachment 439658 [details]
Patch for landing

[Sihui Liu]

Created attachment 439661 [details]
Patch for landing

[EWS]

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