Bug 230805 – Make File System Access API available in Worker

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Radar WebKit Bug Importer]

<rdar://problem/83552511>

[Sihui Liu]

Created attachment 439299 [details]
Patch

[Sihui Liu]

Created attachment 439314 [details]
Patch

[Sihui Liu]

Created attachment 439316 [details]
Patch

[Sihui Liu]

Created attachment 439319 [details]
Patch

[Sihui Liu]

Created attachment 439321 [details]
Patch

[Sihui Liu]

Created attachment 439322 [details]
Patch

[Sihui Liu]

Created attachment 439324 [details]
Patch

[Sihui Liu]

Created attachment 439325 [details]
Patch

[youenn fablet]

Comment on attachment 439325 [details]
Patch

Patch is quite big, I would split the StorageManager part in its own patch (and start with this one).
Then the next one on FileSystemStorageConnection which I think could reuse the same model as StorageManager.
Overall StorageManager seems good to me. Not sure why we need an identifier for FileSystemStorageConnection.

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

> Source/WTF/wtf/ObjectIdentifier.h:113
> +    ObjectIdentifier isolatedCopy() const

Why do we need this one?

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

WTFMove

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:51
> +    m_scope->addFileSystemStorageConnection(m_identifier, *this);

There should be a need for one connection per worker, so we can use the ScriptContextExecutionIdentifier to identify the connection if needed.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:53
> +        m_mainThreadConnection = WTFMove(connection);

I think we should be able to get the connection of the owner of the worker here.
Either we do callOnMainThreadAndWait and we retrieve a process wide connection.
Or we do a postTaskToOwner like we do in WorkerThreadableLoader and we can retrieve the connection from the worker's Document.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:96
> +                downcast<WorkerGlobalScope>(scope).fileSystemStorageConnection(connectionIdentifier)->didIsSameEntry(callbackIdentifier, WTFMove(result));

If we have just one connection per worker, we should be able to do without connectionIdentifier.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:59
> +    RefPtr<WorkerGlobalScope> m_scope;

I would use a WeakPtr<WorkerGlobalScope> instead of a Ref.
This will reduce the issue of a ref cycle.
This should be safe given we will always use m_scope from worker thread.
And we can pass the weak ptr as part of callbacks as well.

> Source/WebCore/Modules/storage/StorageConnection.h:38
> +class StorageConnection : public ThreadSafeRefCounted<StorageConnection> {

For extra safety, I would use DestructionThread::MainLoop

[Sihui Liu]

Comment on attachment 439325 [details]
Patch

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

I will go split the patch into smaller ones for easier review.

>> Source/WTF/wtf/ObjectIdentifier.h:113
>> +    ObjectIdentifier isolatedCopy() const
> 
> Why do we need this one?

I used CrossThreadCopy for the result in WorkerFileSystemStorageConnection, and ExceptionOr.h will call isolatedCopy on the result value.

An alternative is to add template<> struct CrossThreadCopierBase... like for ExceptionOr<void> in ExceptionOr.h I guess

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

Will change.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:51
>> +    m_scope->addFileSystemStorageConnection(m_identifier, *this);
> 
> There should be a need for one connection per worker, so we can use the ScriptContextExecutionIdentifier to identify the connection if needed.

I made it possible that there can be multiple connections per worker because WorkerFileSystemStorageConnection is affiliated with network process connection. 
E.g. If network process connection is lost, the WorkerFileSystemStorageConnection whose mainThreadConnection is using the NetworkProcessConnection will be inactive . New handles created later with StorageManager.getDirectory will has a new WorkerFileSystemStorageConnection with new mainThreadConnection.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:53
>> +        m_mainThreadConnection = WTFMove(connection);
> 
> I think we should be able to get the connection of the owner of the worker here.
> Either we do callOnMainThreadAndWait and we retrieve a process wide connection.
> Or we do a postTaskToOwner like we do in WorkerThreadableLoader and we can retrieve the connection from the worker's Document.

Do you mean we should not keep the m_mainThreadConnection in WorkerFileSystemStorageConnection?

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:96
>> +                downcast<WorkerGlobalScope>(scope).fileSystemStorageConnection(connectionIdentifier)->didIsSameEntry(callbackIdentifier, WTFMove(result));
> 
> If we have just one connection per worker, we should be able to do without connectionIdentifier.

Like explained above; current design is one connection per handle family (root handle is the one created from StorageManager.getDirectory())

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:59
>> +    RefPtr<WorkerGlobalScope> m_scope;
> 
> I would use a WeakPtr<WorkerGlobalScope> instead of a Ref.
> This will reduce the issue of a ref cycle.
> This should be safe given we will always use m_scope from worker thread.
> And we can pass the weak ptr as part of callbacks as well.

Sure.

[Sihui Liu]

Created attachment 439705 [details]
Patch

[Sihui Liu]

Created attachment 439737 [details]
Patch

[youenn fablet]

Comment on attachment 439737 [details]
Patch

Looks good to me, not sure we need a worker file system connection per file handle though.

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

> Source/WTF/wtf/ObjectIdentifier.h:113
> +    ObjectIdentifier isolatedCopy() const

If template<> struct CrossThreadCopierBase works, I think I would prefer that, it makes it clearer where

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:35
> +static uint64_t generateIdentifier()

I would go with ObjectIdentifier if possible.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:50
> +    m_scope->addFileSystemStorageConnection(m_identifier, *this);

I would have thought there would be just one WorkerFileSystemStorageConnection per WorkerGlobalScope.
Why do we need multiple ones?

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:51
> +    m_mainThreadConnection = WTFMove(connection);

This one could be added close to m_scope init.

> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:125
> +    auto workerFileSystemStorageConnection = WorkerFileSystemStorageConnection::create(*m_scope, Ref { *mainThreadFileSystemStorageConnection });

I would think we could reuse the same WorkerFileSystemStorageConnection for all file handles.
Is the issue about network process crash?

[Sihui Liu]

Comment on attachment 439737 [details]
Patch

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

>> Source/WTF/wtf/ObjectIdentifier.h:113
>> +    ObjectIdentifier isolatedCopy() const
> 
> If template<> struct CrossThreadCopierBase works, I think I would prefer that, it makes it clearer where

Will change.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:35
>> +static uint64_t generateIdentifier()
> 
> I would go with ObjectIdentifier if possible.

This is only used in WorkerGlobalScope so I just use int; will change to ObjectIdentifier.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:50
>> +    m_scope->addFileSystemStorageConnection(m_identifier, *this);
> 
> I would have thought there would be just one WorkerFileSystemStorageConnection per WorkerGlobalScope.
> Why do we need multiple ones?

So WorkerFileSytemStorageConnection is a basically a proxy of WebFileSystemStorageConnection.
For WebFileSystemStorageConnection, our current approach is one valid WebFileSystemStorageConnection per process; its IPC connection is lost (network process crashes), it becomes invalid (all requests from handle will fail).
When a new handle is created and there is no valid connection, we will create a new WebFileSystemStorageConnection for it.

For WorkerFileSytemStorageConnection, it's binded with one WebFileSystemStorageConnection. (We won't replace its mainthread connection when it becomes invalid.)
If we are going to have one valid WorkerFileSystemStorageConnection per WorkerGlobalScope, I can think of two ways. 
One is each time we update WebFileSystemStorageConnection, we create a new WorkerFileSystemStorageConnection.
Another is each time we create a new handle in worker, we check if there is new WebFileSystemStorageConnection on the main thread, if there is we will create a new WorkerFileSystemStorageConnection.
These updates involves cross-thread state checks, and we will need to keep reference to existing WorkerFileSystemStorageConnection in the task dispatched to the main thread, so the result can be dispatched back to it (since WorkerGlobalScope only knows valid WorkerFileSystemStorageConnection), which is a reference cycle, and we need to ensure the WorkerFileSystemStorageConnection is destroyed on the Worker thread.
It seems too complicated.

By tracking all existing WorkerFileSystemStorageConnections (valid and invalid) in WorkerGlobalScope, we avoid managing the connection state. Every time a root handle is created (not each handle will get a new WorkerFileSystemStorageConnection), we will create a new WorkerFileSystemStorageConnection with a valid WebFileSystemStorageConnection.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:51
>> +    m_mainThreadConnection = WTFMove(connection);
> 
> This one could be added close to m_scope init.

Sure.

>> Source/WebCore/Modules/storage/WorkerStorageConnection.cpp:125
>> +    auto workerFileSystemStorageConnection = WorkerFileSystemStorageConnection::create(*m_scope, Ref { *mainThreadFileSystemStorageConnection });
> 
> I would think we could reuse the same WorkerFileSystemStorageConnection for all file handles.
> Is the issue about network process crash?

Yes.

[Sihui Liu]

Created attachment 439752 [details]
Patch

[Sihui Liu]

Created attachment 439754 [details]
Patch

[youenn fablet]

I am not sure we need a one connection-per-handle organisation to neuter objects on network process crash so I think I would stick with just one connection in the worker for now. We can discuss how to handle network process crash later.

One possibility would be to have a WebProcess handle ObjectIdentifier <-> NetworkProcess handle ObjectIdentifier map in WebFileSystemStorageConnection.
The map is cleared on network process crash. If the map has no entry, the object is neutered.

But are we sure it is impossible to keep making these objects work after network process crash?
From what I see, these objects are real files so they will not disappear on network process crash.
In theory, if there was a network process crash, we could detect that a file handle needs to be 'repaired' and we could give back the necessary information in terms of name and so on from WebProcess to NetworkProcess.

[Sihui Liu]

(In reply to youenn fablet from comment #18)
> I am not sure we need a one connection-per-handle organisation to neuter
> objects on network process crash so I think I would stick with just one
> connection in the worker for now. We can discuss how to handle network
> process crash later.
> 
> One possibility would be to have a WebProcess handle ObjectIdentifier <->
> NetworkProcess handle ObjectIdentifier map in WebFileSystemStorageConnection.
> The map is cleared on network process crash. If the map has no entry, the
> object is neutered.
> 
> But are we sure it is impossible to keep making these objects work after
> network process crash?
> From what I see, these objects are real files so they will not disappear on
> network process crash.
> In theory, if there was a network process crash, we could detect that a file
> handle needs to be 'repaired' and we could give back the necessary
> information in terms of name and so on from WebProcess to NetworkProcess.

Okay, I will try making one connection per WorkerGlobalScope.

[Sihui Liu]

Created attachment 439766 [details]
Patch

[youenn fablet]

Comment on attachment 439766 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:67
> +    HashMap<CallbackIdentifier, FileSystemStorageConnection::ResolveCallback> m_resolveCallbacks;

Not sure whether this is mandatory to have a CallbackIdentifier, since we then need to use generateThreadSafe.
A counter owned by WorkerFileSystemStorageConnection might be good enough, like done in WorkerStorageConnection.

[Sihui Liu]

(In reply to youenn fablet from comment #21)
> Comment on attachment 439766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439766&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:67
> > +    HashMap<CallbackIdentifier, FileSystemStorageConnection::ResolveCallback> m_resolveCallbacks;
> 
> Not sure whether this is mandatory to have a CallbackIdentifier, since we
> then need to use generateThreadSafe.
> A counter owned by WorkerFileSystemStorageConnection might be good enough,
> like done in WorkerStorageConnection.

Because WorkerGlobalScope only holds reference to the valid connection now, and all result will be directed to it, the globally unique callback identifier makes sure we don't invoke callback from wrong connection.

[Sihui Liu]

Created attachment 439787 [details]
Patch for landing

[EWS]

Committed r283347 (242360@main): <https://commits.webkit.org/242360@main>

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

[youenn fablet]

(In reply to Sihui Liu from comment #22)
> (In reply to youenn fablet from comment #21)
> > Comment on attachment 439766 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=439766&action=review
> > 
> > > Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:67
> > > +    HashMap<CallbackIdentifier, FileSystemStorageConnection::ResolveCallback> m_resolveCallbacks;
> > 
> > Not sure whether this is mandatory to have a CallbackIdentifier, since we
> > then need to use generateThreadSafe.
> > A counter owned by WorkerFileSystemStorageConnection might be good enough,
> > like done in WorkerStorageConnection.
> 
> Because WorkerGlobalScope only holds reference to the valid connection now,
> and all result will be directed to it, the globally unique callback
> identifier makes sure we don't invoke callback from wrong connection.

We could keep the same WorkerFileSystemStorageConnection for the lifetime of the WorkerGlobalScope.
In case of new main thread connection, we could set the new connection on the WorkerFileSystemStorageConnection and clear WorkerFileSystemStorageConnection callbacks (returning InvalidStateError or so) when that happens.