FileSystemSyncAccessHandle should be invalidated when network process crashes

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

The newly launched network process will not know about pre-existing FileSystemSyncAccessHandle.
Either FileSystemSyncAccessHandle needs to register to new network process, or FileSystemSyncAccessHandle should be invalidated.

[Radar WebKit Bug Importer]

<rdar://problem/85187706>

[Sihui Liu]

Created attachment 445236 [details]
Patch

[youenn fablet]

Comment on attachment 445236 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:75
> +    void invalidateAccessHandle(FileSystemSyncAccessHandleIdentifier) final;

I would tend to have a Worker specific method, like registerAccessHandle(FileSystemSyncAccessHandle), and WorkerFileSystemStorageConnection could store a WeakHashSet<FileSystemSyncAccessHandle>.
Then what would be needed is for some code to call WorkerFileSystemStorageConnection::connectionClosed() when the network process connection gets closed.
For instance by adding a DedicatedWorkerGlobalScope::forEach(Function<void(WorkerGlobalScope&)>&&).

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:93
> +    HashMap<WebCore::FileSystemSyncAccessHandleIdentifier, Function<void()>> m_accessHandleInvalidationHandlers;

If we go with registerInvalidationHandler/unregisterInvalidationHandler at connection level, a Vector seems enough here.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.messages.in:24
> +    InvalidateAccessHandle(WebCore::FileSystemSyncAccessHandleIdentifier identifier)

Why do we need that message, it seems the IPC connection being closed should be enough?

[Sihui Liu]

(In reply to youenn fablet from comment #3)
> Comment on attachment 445236 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445236&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:75
> > +    void invalidateAccessHandle(FileSystemSyncAccessHandleIdentifier) final;
> 
> I would tend to have a Worker specific method, like
> registerAccessHandle(FileSystemSyncAccessHandle), and
> WorkerFileSystemStorageConnection could store a
> WeakHashSet<FileSystemSyncAccessHandle>.
> Then what would be needed is for some code to call
> WorkerFileSystemStorageConnection::connectionClosed() when the network
> process connection gets closed.
> For instance by adding a
> DedicatedWorkerGlobalScope::forEach(Function<void(WorkerGlobalScope&)>&&).
> 
Do you mean to dispatch a task to each worker thread to invalidate all its sync access handles? That sounds reasonable if we want to invalidate all access handles. Here we may want to invalidate some handle for the data removal case, so I store handler for each handle (see below).

> > Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.h:93
> > +    HashMap<WebCore::FileSystemSyncAccessHandleIdentifier, Function<void()>> m_accessHandleInvalidationHandlers;
> 
> If we go with registerInvalidationHandler/unregisterInvalidationHandler at
> connection level, a Vector seems enough here.
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.messages.in:24
> > +    InvalidateAccessHandle(WebCore::FileSystemSyncAccessHandleIdentifier identifier)
> 
> Why do we need that message, it seems the IPC connection being closed should
> be enough?

As mentioned in the changelog, the invalidation handler will be used in two cases:
1. connection to backend (network process) is lost
2. access handle is invalidated on network process's request (when website data is deleted)
The message is needed for the second case (I will use it in  https://bugs.webkit.org/show_bug.cgi?id=233567)

[youenn fablet]

> > Why do we need that message, it seems the IPC connection being closed should
> > be enough?
> 
> As mentioned in the changelog, the invalidation handler will be used in two
> cases:
> 1. connection to backend (network process) is lost
> 2. access handle is invalidated on network process's request (when website
> data is deleted)
> The message is needed for the second case (I will use it in 
> https://bugs.webkit.org/show_bug.cgi?id=233567)

Understood, let's add the IPC message in the patch that needs it then, here it does not seem used at all.

Also, it seems we could manage a map <FileSystemSyncAccessHandleIdentifier, pair< ScriptExecutionContextIdentifier , FileSystemHandleIdentifier>> in WebFileSystemStorageConnection and use it for this purpose as well as future purposes.

[Sihui Liu]

Created attachment 445471 [details]
Patch

[Sihui Liu]

Created attachment 445482 [details]
Patch

[Sihui Liu]

Created attachment 445486 [details]
Patch

[Sihui Liu]

Created attachment 445499 [details]
Patch

[Sihui Liu]

Created attachment 445516 [details]
Patch

[Sihui Liu]

Created attachment 445529 [details]
Patch

[youenn fablet]

Comment on attachment 445529 [details]
Patch

WinCairo issue is legit, probably a missing include.

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

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp:100
> +void FileSystemFileHandle::registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier identifier, FileSystemSyncAccessHandle* handle)

FileSystemSyncAccessHandle& would be better.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:52
> +    m_source->registerSyncAccessHandle(m_identifier, this);

*this would be better.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:319
> +        handle->invalidate();

if (auto handle...)

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:73
> +    void registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier, ScriptExecutionContextIdentifier, FileSystemSyncAccessHandle*) final;

FileSystemSyncAccessHandle& is better.
I would tend to make this registerSyncAccessHandle not virtual but specific to WorkerFileSystemStorageConnection.
To call it, handle would need to do something like downcast<WorkerGlobalScope>(context).fileSystemStorageConnection().registerSyncAccessHandle(...).
We could then take a reference instead of a pointer.

FileSystemStorageConnection could have a registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier, ScriptExecutionContextIdentifier) that would do nothing in WorkerFileSystemStorageConnection.
Or we could somehow introduce a MainThreadFileSystemStorageConnection.

> Source/WebCore/workers/WorkerGlobalScope.h:96
> +    WEBCORE_EXPORT WorkerFileSystemStorageConnection* fileSystemStorageConnection();

Can we return a reference here?

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:215
> +            if (FileSystemStorageConnection* connection = downcast<WebCore::WorkerGlobalScope>(context).fileSystemStorageConnection())

auto*. Would be good to be able to use a ref.

[Sihui Liu]

(In reply to youenn fablet from comment #12)
> Comment on attachment 445529 [details]
> Patch
> 
> WinCairo issue is legit, probably a missing include.

Thanks for the review. Will make sure bots are green before landing.

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445529&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp:100
> > +void FileSystemFileHandle::registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier identifier, FileSystemSyncAccessHandle* handle)
> 
> FileSystemSyncAccessHandle& would be better.

Will change.

> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:52
> > +    m_source->registerSyncAccessHandle(m_identifier, this);
> 
> *this would be better.

Will change.

> 
> > Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:319
> > +        handle->invalidate();
> 
> if (auto handle...)

Sure.

> 
> > Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.h:73
> > +    void registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier, ScriptExecutionContextIdentifier, FileSystemSyncAccessHandle*) final;
> 
> FileSystemSyncAccessHandle& is better.
> I would tend to make this registerSyncAccessHandle not virtual but specific
> to WorkerFileSystemStorageConnection.
> To call it, handle would need to do something like
> downcast<WorkerGlobalScope>(context).fileSystemStorageConnection().
> registerSyncAccessHandle(...).
> We could then take a reference instead of a pointer.
> 
> FileSystemStorageConnection could have a
> registerSyncAccessHandle(FileSystemSyncAccessHandleIdentifier,
> ScriptExecutionContextIdentifier) that would do nothing in
> WorkerFileSystemStorageConnection.
> Or we could somehow introduce a MainThreadFileSystemStorageConnection.

Sure, will make registerSyncAccessHandle non-virtual.

> 
> > Source/WebCore/workers/WorkerGlobalScope.h:96
> > +    WEBCORE_EXPORT WorkerFileSystemStorageConnection* fileSystemStorageConnection();
> 
> Can we return a reference here?
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:215
> > +            if (FileSystemStorageConnection* connection = downcast<WebCore::WorkerGlobalScope>(context).fileSystemStorageConnection())
> 
> auto*. Would be good to be able to use a ref.

Sure.

[Sihui Liu]

Created attachment 445611 [details]
Patch

[Sihui Liu]

Created attachment 445618 [details]
Patch

[Sihui Liu]

Created attachment 445640 [details]
Patch

[EWS]

Committed r286414 (244760@main): <https://commits.webkit.org/244760@main>

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