Bug 230484 – Add initial support for File System Access API

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

[Sihui Liu]

...

[Sihui Liu]

Created attachment 438700 [details]
Patch

[Sihui Liu]

Created attachment 438725 [details]
Patch

[Sihui Liu]

Created attachment 438737 [details]
Patch

[Sihui Liu]

Created attachment 438743 [details]
Patch

[Sihui Liu]

Created attachment 438750 [details]
Patch

[Sihui Liu]

Created attachment 438760 [details]
Patch

[Sihui Liu]

Created attachment 438779 [details]
Patch

[Sihui Liu]

Created attachment 438783 [details]
Patch

[youenn fablet]

Comment on attachment 438779 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:67
> +        promise.resolve(result.releaseReturnValue());

promise.settle is not working?

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:85
> +        promise.resolve(result.releaseReturnValue());

promise.settle is not working?

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:45
> +void FileSystemHandle::isSameEntry(FileSystemHandle& handle, DOMPromiseDeferred<IDLBoolean>&& promise)

We could try to keep const here by having a const impl() getter.

> Source/WebCore/Modules/storage/StorageManager.cpp:84
> +    return connectionInfo.connection.persisted(WTFMove(connectionInfo.origin), [promise = WTFMove(promise)](bool persisted) mutable {

I would tend to rename persisted to getPersisted, retrieveIsPersisted or something else with a verb...

> Source/WebCore/Modules/storage/StorageManager.cpp:85
>          promise.resolve(persisted);

Are we sure persisted will not sometimes return an exception?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:41
> +    : m_identifier(FileSystemStorageHandleIdentifier::generate())

Should it be generateThreadSafe()?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:36
> +{

Every operation is done out of main thread here, we could add some ASSERT(!isMainThread())

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:83
> +    if (auto handles = m_handlesByConnection.find(connection.uniqueID()); handles != m_handlesByConnection.end()) {

I would tend to do the usual:
auto handles = m_handlesByConnection.find(connection.uniqueID());
if (handles == m_handlesByConnection.end())
   return;
...

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:87
> +            m_registry.unregisterHandle(handleIdentifier);

Call to remove-then-take is probably an issue.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:39
> +    explicit FileSystemStorageManager(String&& path, FileSystemStorageHandleRegistry&);

No need for explicit

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:49
> +    FileSystemStorageHandleRegistry& m_registry;

Are we sure this is always granted that FileSystemStorageHandleRegistry will outlive its FileSystemStorageManager?
We could make FileSystemStorageHandleRegistry  thread safe ref counted and take a Ref<> here if we are not sure of that.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:50
> +    WeakHashSet<IPC::Connection> m_connections;

This does not look safe.
It might be better to move to a HashSet<IPC::Connection::UniqueID>
I am not sure of the actual use of m_connections, maybe it can be removed altogether.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:145
> +    handle->getFileHandle(connection, name, createIfNecessary, WTFMove(completionHandler));

There we could eventually take a String&& name if getFileHandle could make use of it.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6
> +	objectVersion = 54;

Do we need that change?

> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:70
> +void FileSystemStorageHandleProxy::getFileHandle(String&& name, bool createIfNecessary, CompletionHandler<void(WebCore::ExceptionOr<Ref<WebCore::FileSystemFileHandle>>&&)>&& completionHandler)

We do not use String&& here. A const String& might be enough.
Ditto probably below

> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:49
>      connection().sendWithAsyncReply(Messages::NetworkStorageManager::Persist(origin), WTFMove(completionHandler));

It does not seem like we need && here. const ClientOrigin& should be sufficient. Ditto for persisted.

> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:52
> +void WebStorageConnection::fileSystemGetDirectory(WebCore::ClientOrigin&& origin, CompletionHandler<void(WebCore::ExceptionOr<Ref<WebCore::FileSystemDirectoryHandle>>&&)>&& completionHandler)

Ditto for ClientOrigin&&

> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:55
> +    connection.sendWithAsyncReply(Messages::NetworkStorageManager::FileSystemGetDirectory(origin), [weakConnection = makeWeakPtr(connection), completionHandler = WTFMove(completionHandler)](auto identifier, auto error) mutable {

I think messages.in supports Expected<> which might better here.
There are a few places above where it could be used as well.
Maybe it could support ExceptionOr as well.

[Sihui Liu]

Comment on attachment 438779 [details]
Patch

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

S

>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:67
>> +        promise.resolve(result.releaseReturnValue());
> 
> promise.settle is not working?

Yes, result here is ExceptionOr<Ref<WebCore::FileSystemDirectoryHandle>>. I tried settle() and a compile error said it cannot convert.

>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:85
>> +        promise.resolve(result.releaseReturnValue());
> 
> promise.settle is not working?

Ditto.

>> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:45
>> +void FileSystemHandle::isSameEntry(FileSystemHandle& handle, DOMPromiseDeferred<IDLBoolean>&& promise)
> 
> We could try to keep const here by having a const impl() getter.

Sure.

>> Source/WebCore/Modules/storage/StorageManager.cpp:84
>> +    return connectionInfo.connection.persisted(WTFMove(connectionInfo.origin), [promise = WTFMove(promise)](bool persisted) mutable {
> 
> I would tend to rename persisted to getPersisted, retrieveIsPersisted or something else with a verb...

Sure, will change to getPersisted

>> Source/WebCore/Modules/storage/StorageManager.cpp:85
>>          promise.resolve(persisted);
> 
> Are we sure persisted will not sometimes return an exception?

It does not based on current implementation (once it connects to the network process it should get an answer).

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:41
>> +    : m_identifier(FileSystemStorageHandleIdentifier::generate())
> 
> Should it be generateThreadSafe()?

Yes, will change.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:36
>> +{
> 
> Every operation is done out of main thread here, we could add some ASSERT(!isMainThread())

Sure

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:83
>> +    if (auto handles = m_handlesByConnection.find(connection.uniqueID()); handles != m_handlesByConnection.end()) {
> 
> I would tend to do the usual:
> auto handles = m_handlesByConnection.find(connection.uniqueID());
> if (handles == m_handlesByConnection.end())
>    return;
> ...

Sure.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:87
>> +            m_registry.unregisterHandle(handleIdentifier);
> 
> Call to remove-then-take is probably an issue.

oops, I meant to use take(), will remove remove()

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:39
>> +    explicit FileSystemStorageManager(String&& path, FileSystemStorageHandleRegistry&);
> 
> No need for explicit

Right

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:49
>> +    FileSystemStorageHandleRegistry& m_registry;
> 
> Are we sure this is always granted that FileSystemStorageHandleRegistry will outlive its FileSystemStorageManager?
> We could make FileSystemStorageHandleRegistry  thread safe ref counted and take a Ref<> here if we are not sure of that.

Yes, I guess we can set FileSystemStorageHandleRegistry on the background thread to ensure that.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.h:50
>> +    WeakHashSet<IPC::Connection> m_connections;
> 
> This does not look safe.
> It might be better to move to a HashSet<IPC::Connection::UniqueID>
> I am not sure of the actual use of m_connections, maybe it can be removed altogether.

Yes, it is unneeded.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:145
>> +    handle->getFileHandle(connection, name, createIfNecessary, WTFMove(completionHandler));
> 
> There we could eventually take a String&& name if getFileHandle could make use of it.

Sure

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6
>> +	objectVersion = 54;
> 
> Do we need that change?

Nope.

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:70
>> +void FileSystemStorageHandleProxy::getFileHandle(String&& name, bool createIfNecessary, CompletionHandler<void(WebCore::ExceptionOr<Ref<WebCore::FileSystemFileHandle>>&&)>&& completionHandler)
> 
> We do not use String&& here. A const String& might be enough.
> Ditto probably below

Sure.

>> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:49
>>      connection().sendWithAsyncReply(Messages::NetworkStorageManager::Persist(origin), WTFMove(completionHandler));
> 
> It does not seem like we need && here. const ClientOrigin& should be sufficient. Ditto for persisted.

Will change.

>> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:52
>> +void WebStorageConnection::fileSystemGetDirectory(WebCore::ClientOrigin&& origin, CompletionHandler<void(WebCore::ExceptionOr<Ref<WebCore::FileSystemDirectoryHandle>>&&)>&& completionHandler)
> 
> Ditto for ClientOrigin&&

Will change.

[Sihui Liu]

Created attachment 438870 [details]
Patch

[Sihui Liu]

Created attachment 438902 [details]
Patch

[Sihui Liu]

Created attachment 438918 [details]
Patch

[youenn fablet]

Comment on attachment 438918 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: storage/filesystemaccess/directory-handle-basics.html

Would be good to add some more information about what the patch does.
For instance pointers to spec, what is done and what is still missing...

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:49
> +void FileSystemDirectoryHandle::getFileHandle(String&& name, std::optional<FileSystemDirectoryHandle::GetFileOptions> options, DOMPromiseDeferred<IDLInterface<FileSystemFileHandle>>&& promise)

String&& does not seem needed since impl().getFileHandle seems to take a const String&.
Ditto in other places below.

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:79
> +void FileSystemDirectoryHandle::resolve(FileSystemHandle& handle, DOMPromiseDeferred<IDLSequence<IDLUSVString>>&& promise)

Can we keep const& here?

> Source/WebCore/Modules/filesystemaccess/FileSystemHandleImpl.h:36
> +class FileSystemHandleImpl : public RefCounted<FileSystemHandleImpl> {

What is the purpose of this class? Does it need to be refcounted?
Is it to handle worker  vs. window differences?

> Source/WebCore/Modules/storage/StorageManager.cpp:74
> +    return ConnectionInfo { *connection, { context->topOrigin().data(), origin->data() } };

No need for return here. Ditto below.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:61
> +void FileSystemStorageHandle::isSameEntry(FileSystemStorageHandleIdentifier identifier, CompletionHandler<void(bool)>&& completionHandler)

Do we need a completion handler or are we expecting to always return a result synchronously?
Ditto below.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.h:57
> +    WeakPtr<FileSystemStorageManager> m_manager;

m_manager will probably be used in a background thread, are we sure m_manager will be destroyed in the same background thread?
From my reading of the code, it seems correct, but I want to make sure.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:48
> +void FileSystemStorageManager::createHandle(IPC::Connection& connection, FileSystemStorageHandle::Type type, String&& path, String&& name, bool createIfNecessary, CompletionHandler<void(Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError>)>&& completionHandler)

Do we need a completion handler here or do we always expect to synchronously return the value.
Ditto for other methods.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:71
> +    m_handlesByConnection.ensure(connection.uniqueID(), [&] {

It seems we are only using connection for its uniqueID. I guess we could simply pass uniqueID on all the methods?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:101
> +    m_queue->dispatch([this, protectedThis = Ref { *this }, protectedConnection = Ref { connection }]() {

We probably to ensure proctedThis goes back to main run loop.
If we are sure we only need to keep connection identifiers, we could remove protectedConnection and capture its id instead.

> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:83
> +        completionHandler(WebCore::FileSystemFileHandle::create(String { name }, WTFMove(handle)));

I think it would be better to create FileSystemFileHandle objects in WebCore and pass the necessary parameters to create it in the completion handler.
One reason is that this code path might be used for creating both window and worker file handle objects.
We may also in the future want to have file handle objects be ActiveDOMObjects or ContextDestructionObserver for instance.

[Sihui Liu]

Comment on attachment 438918 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Test: storage/filesystemaccess/directory-handle-basics.html
> 
> Would be good to add some more information about what the patch does.
> For instance pointers to spec, what is done and what is still missing...

Sure, will add.

>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:49
>> +void FileSystemDirectoryHandle::getFileHandle(String&& name, std::optional<FileSystemDirectoryHandle::GetFileOptions> options, DOMPromiseDeferred<IDLInterface<FileSystemFileHandle>>&& promise)
> 
> String&& does not seem needed since impl().getFileHandle seems to take a const String&.
> Ditto in other places below.

Okay.

>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:79
>> +void FileSystemDirectoryHandle::resolve(FileSystemHandle& handle, DOMPromiseDeferred<IDLSequence<IDLUSVString>>&& promise)
> 
> Can we keep const& here?

Sure.

>> Source/WebCore/Modules/filesystemaccess/FileSystemHandleImpl.h:36
>> +class FileSystemHandleImpl : public RefCounted<FileSystemHandleImpl> {
> 
> What is the purpose of this class? Does it need to be refcounted?
> Is it to handle worker  vs. window differences?

Yes, for worker (and the possibility for WebKitLegacy)

>> Source/WebCore/Modules/storage/StorageManager.cpp:74
>> +    return ConnectionInfo { *connection, { context->topOrigin().data(), origin->data() } };
> 
> No need for return here. Ditto below.

Will remove.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:61
>> +void FileSystemStorageHandle::isSameEntry(FileSystemStorageHandleIdentifier identifier, CompletionHandler<void(bool)>&& completionHandler)
> 
> Do we need a completion handler or are we expecting to always return a result synchronously?
> Ditto below.

This will be returned sync-ly. Will remove the completion handler here.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.h:57
>> +    WeakPtr<FileSystemStorageManager> m_manager;
> 
> m_manager will probably be used in a background thread, are we sure m_manager will be destroyed in the same background thread?
> From my reading of the code, it seems correct, but I want to make sure.

Yes, this will be used on background queue only.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:48
>> +void FileSystemStorageManager::createHandle(IPC::Connection& connection, FileSystemStorageHandle::Type type, String&& path, String&& name, bool createIfNecessary, CompletionHandler<void(Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError>)>&& completionHandler)
> 
> Do we need a completion handler here or do we always expect to synchronously return the value.
> Ditto for other methods.

They should return sync-ly. Will change their return value.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageManager.cpp:71
>> +    m_handlesByConnection.ensure(connection.uniqueID(), [&] {
> 
> It seems we are only using connection for its uniqueID. I guess we could simply pass uniqueID on all the methods?

Yes.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:101
>> +    m_queue->dispatch([this, protectedThis = Ref { *this }, protectedConnection = Ref { connection }]() {
> 
> We probably to ensure proctedThis goes back to main run loop.
> If we are sure we only need to keep connection identifiers, we could remove protectedConnection and capture its id instead.

We don't need to dispatch it back to main runloop since NetworkStorageManager::close() should take the last reference?

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:83
>> +        completionHandler(WebCore::FileSystemFileHandle::create(String { name }, WTFMove(handle)));
> 
> I think it would be better to create FileSystemFileHandle objects in WebCore and pass the necessary parameters to create it in the completion handler.
> One reason is that this code path might be used for creating both window and worker file handle objects.
> We may also in the future want to have file handle objects be ActiveDOMObjects or ContextDestructionObserver for instance.

Sounds good.

[youenn fablet]

> We don't need to dispatch it back to main runloop since NetworkStorageManager::close() should take the last reference?

This is not guaranteed and in any case, destructing in the same thread it is constructing to is usually safer.

[Sihui Liu]

Created attachment 438957 [details]
Patch

[Sihui Liu]

Created attachment 438958 [details]
Patch

[Sihui Liu]

Created attachment 438960 [details]
Patch

[Sihui Liu]

rdar://83405890

[youenn fablet]

Comment on attachment 438960 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:51
> +    bool createIfNecessary = options ? options->create : false;

You can probably use bool createIfNecessary = options.value_or({ }).create;
Ditto below

> Source/WebCore/Modules/filesystemaccess/FileSystemHandleImpl.h:36
> +class FileSystemHandleImpl : public RefCounted<FileSystemHandleImpl> {

Not clear to me we need RefCounted, maybe a UniqueRef<FileSystemHandleImpl> might be needed.
I also think we could have a Connection-like class with the same interface except it would take a FileSystemHandleIdentifier that would be globally unique to the process.
Then FileSystemHandle would own a Ref/RefPtr<Connection> and an identifier.
The connection would have a SessionID and would be responsible to do the abstraction between worker and window.
In that case, you could share the same connection for all handles of a give worker/window.

Let's check this idea after landing this one.

> Source/WebCore/Modules/storage/StorageManager.cpp:57
> +ExceptionOr<StorageManager::ConnectionInfo> StorageManager::connectionInfo()

It could be a free function taking a Navigator*.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageError.h:50
> +    default:

I would remove default so that any new error value would trigger the need to update this code.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:120
> +        RELEASE_ASSERT_NOT_REACHED();

I would tend to replace default by SymbolicLink here.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:77
> +        m_fileSystemStorageHandleRegistry = nullptr;

Should we set m_fileSystemStorageHandleRegistry to nullptr after clearing the maps in case one of the map values has a reference to m_fileSystemStorageHandleRegistry?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:219
>      m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {

I see that we are not hopping the ref back to main thread here.
I would tend to audit the code and make all dispatch that takes a ref do the same (Either hop back to main thread to make sure it stays destroyed in main thread or not).

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:220
>          m_localOriginStorageManagers.clear();

Should we set m_fileSystemStorageHandleRegistry to nullptr?

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

Seems good enough like this but we might want to have a strategy in case of network process crash

> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:75
> +    m_connection->sendWithAsyncReply(Messages::NetworkStorageManager::GetFileHandle(m_identifier, name, createIfNecessary), [this, protectedThis = Ref { *this }, name, completionHandler = WTFMove(completionHandler)](auto result) mutable {

It seems we only need connection, maybe we should catch connection instead.
Here and below.

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

It would not be null in the case we would catch it, I guess if connection is null, it would mean connection is lost and we would probably end up in the if (!result) code path.
This is worth checking.

> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:83
> +        completionHandler(WTFMove(impl));

Could be a one liner

> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.h:60
> +    RefPtr<IPC::Connection> m_connection;

It seems it could be called from main thread and worker thread.
By keeping a ref here, I am unclear of how we do handle network process crash recovery, in particular in workers.
This is worth thinking after landing this patch, as part of the worker model.

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

I am not sure we can have a result and a !weakConnection. Worth checking what is the default result when connection gets closed with a pending reply.

> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:63
> +        return completionHandler(WebCore::FileSystemDirectoryHandle::create({ }, WTFMove(impl)));

Could be a one liner.

[Sihui Liu]

Created attachment 439124 [details]
Patch

[Sihui Liu]

Comment on attachment 438960 [details]
Patch

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

>> Source/WebCore/Modules/filesystemaccess/FileSystemDirectoryHandle.cpp:51
>> +    bool createIfNecessary = options ? options->create : false;
> 
> You can probably use bool createIfNecessary = options.value_or({ }).create;
> Ditto below

I need to use bool createIfNecessary = options.value_or(FileSystemDirectoryHandle::GetFileOptions { }).create to compile.. which seems more verbose than this.

>> Source/WebCore/Modules/filesystemaccess/FileSystemHandleImpl.h:36
>> +class FileSystemHandleImpl : public RefCounted<FileSystemHandleImpl> {
> 
> Not clear to me we need RefCounted, maybe a UniqueRef<FileSystemHandleImpl> might be needed.
> I also think we could have a Connection-like class with the same interface except it would take a FileSystemHandleIdentifier that would be globally unique to the process.
> Then FileSystemHandle would own a Ref/RefPtr<Connection> and an identifier.
> The connection would have a SessionID and would be responsible to do the abstraction between worker and window.
> In that case, you could share the same connection for all handles of a give worker/window.
> 
> Let's check this idea after landing this one.

Sure

>> Source/WebCore/Modules/storage/StorageManager.cpp:57
>> +ExceptionOr<StorageManager::ConnectionInfo> StorageManager::connectionInfo()
> 
> It could be a free function taking a Navigator*.

Okay.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageError.h:50
>> +    default:
> 
> I would remove default so that any new error value would trigger the need to update this code.

Sure.

>> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:120
>> +        RELEASE_ASSERT_NOT_REACHED();
> 
> I would tend to replace default by SymbolicLink here.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:77
>> +        m_fileSystemStorageHandleRegistry = nullptr;
> 
> Should we set m_fileSystemStorageHandleRegistry to nullptr after clearing the maps in case one of the map values has a reference to m_fileSystemStorageHandleRegistry?

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:219
>>      m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> I see that we are not hopping the ref back to main thread here.
> I would tend to audit the code and make all dispatch that takes a ref do the same (Either hop back to main thread to make sure it stays destroyed in main thread or not).

Sure, I will add the main thread callback.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:220
>>          m_localOriginStorageManagers.clear();
> 
> Should we set m_fileSystemStorageHandleRegistry to nullptr?

No, handles will be unregistered when they go away and m_fileSystemStorageHandleRegistry should stay as long as NetworkStorageManager is functional.

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:56
>> +        return completionHandler(WebCore::Exception { WebCore::UnknownError, "Connection is lost" });
> 
> Seems good enough like this but we might want to have a strategy in case of network process crash

Current strategy is if network process crashes, the handler will be numb (so user will need to create a new one). We can think about recovering I guess.

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:75
>> +    m_connection->sendWithAsyncReply(Messages::NetworkStorageManager::GetFileHandle(m_identifier, name, createIfNecessary), [this, protectedThis = Ref { *this }, name, completionHandler = WTFMove(completionHandler)](auto result) mutable {
> 
> It seems we only need connection, maybe we should catch connection instead.
> Here and below.

We can catch connection here.

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:80
>> +            return completionHandler(WebCore::Exception { WebCore::UnknownError, "Connection is lost"_s });
> 
> It would not be null in the case we would catch it, I guess if connection is null, it would mean connection is lost and we would probably end up in the if (!result) code path.
> This is worth checking.

Looks like if connection is lost, result will be set as { }, which is invalid identifier 0. We can check that.

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.cpp:83
>> +        completionHandler(WTFMove(impl));
> 
> Could be a one liner

Compile error for completionHandler(FileSystemStorageHandleProxy::create(result.value(), *m_connection)): no matching function for call to object of type

>> Source/WebKit/WebProcess/WebCoreSupport/FileSystemStorageHandleProxy.h:60
>> +    RefPtr<IPC::Connection> m_connection;
> 
> It seems it could be called from main thread and worker thread.
> By keeping a ref here, I am unclear of how we do handle network process crash recovery, in particular in workers.
> This is worth thinking after landing this patch, as part of the worker model.

I intend to not let FileSystemStorageHandleProxy be used in worker thread by maybe adding a new WorkerFileSystemHandleImpl

>> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:60
>> +            return completionHandler(WebCore::Exception { WebCore::UnknownError, "Connection is lost"_s });
> 
> I am not sure we can have a result and a !weakConnection. Worth checking what is the default result when connection gets closed with a pending reply.

It can happen. I think result is set to be  Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError> { } when connection is lost, which is invalid identifier.

>> Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:63
>> +        return completionHandler(WebCore::FileSystemDirectoryHandle::create({ }, WTFMove(impl)));
> 
> Could be a one liner.

Okay.

[youenn fablet]

> > I am not sure we can have a result and a !weakConnection. Worth checking what is the default result when connection gets closed with a pending reply.
> 
> It can happen. I think result is set to be 
> Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError> { } when
> connection is lost, which is invalid identifier.

You could change the default behavior by using AsyncReplyError, something like:

template<> struct AsyncReplyError< Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError>  > {
    static Expected<FileSystemStorageHandleIdentifier, FileSystemStorageError>  create() { return makeUnexpected(WebCore::FileSystemStorageError::XXX); };
};

Then, I would think you could just check whether the result is an error or not.

[EWS]

Committed r283029 (242089@main): <https://commits.webkit.org/242089@main>

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

[John Wilander]

This seems to be causing errors for non-Cocoa or non-clang builds:

In file included from /app/webkit/WebKitBuild/Release/DerivedSources/WebKit/NetworkStorageManagerMessages.h:29,
                 from /app/webkit/Source/WebKit/WebProcess/WebCoreSupport/WebStorageConnection.cpp:31,
                 from /app/webkit/WebKitBuild/Release/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-19.cpp:3:
/app/webkit/Source/WebKit/NetworkProcess/storage/FileSystemStorageError.h: In function ‘WebCore::ExceptionCode WebKit::convertToExceptionCode(WebKit::FileSystemStorageError)’:
/app/webkit/Source/WebKit/NetworkProcess/storage/FileSystemStorageError.h:55:1: warning: control reaches end of non-void function [-Wreturn-type]
   55 | }
      | ^

I believe there's a macro we use to add a default clause or ignore the error when there is no default clause.

[John Wilander]

We may have switched to just adding
     default:
        ASSERT_NOT_REACHED();