...
Created attachment 439428 [details] Patch
Created attachment 439430 [details] Patch
<rdar://problem/83606465>
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 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?
Created attachment 439499 [details] Patch
Created attachment 439539 [details] Patch
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 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.
Created attachment 439656 [details] Patch for landing
Created attachment 439657 [details] Patch for landing
Created attachment 439658 [details] Patch for landing
Created attachment 439661 [details] Patch for landing
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].