Bug 231466 – Implement FileSystemSyncAccessHandle read() and write()

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

[Sihui Liu]

...

[Radar WebKit Bug Importer]

<rdar://problem/84050394>

[Sihui Liu]

Created attachment 440690 [details]
Patch

[Sihui Liu]

Created attachment 440737 [details]
Patch

[Sihui Liu]

Created attachment 440738 [details]
Patch

[Sihui Liu]

Created attachment 440750 [details]
Patch

[Sihui Liu]

Created attachment 440752 [details]
Patch

[Sihui Liu]

Created attachment 440753 [details]
Patch

[Sihui Liu]

Created attachment 440754 [details]
Patch

[Sihui Liu]

Created attachment 440756 [details]
Patch

[Sihui Liu]

Created attachment 440757 [details]
Patch

[Sihui Liu]

Created attachment 440758 [details]
Patch

[Sihui Liu]

Created attachment 440759 [details]
Patch

[Sihui Liu]

The newly added test should pass if bots have internal SDK; I am not sure whether I should update -expected.txt file or mark test as [Pass Failure] in TestExpectations. @Youenn, any advice?

[youenn fablet]

(In reply to Sihui Liu from comment #13)
> The newly added test should pass if bots have internal SDK; I am not sure
> whether I should update -expected.txt file or mark test as [Pass Failure] in
> TestExpectations. @Youenn, any advice?

Can we forward declare the functions we are using from internal SDK if we do not have it?
As an example, you can look at NSWSPI.h: it is including a private header if we have internal SDK and is declaring the functions webkit is using otherwise.

[youenn fablet]

Comment on attachment 440759 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Worth beefing up the change log to explain what the model is (transfer file handle, and do operations in WebProcess?).

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:211
> +        return FileSystemStorageError::Unknown;

Do we need getSize, flush and truncate if they will be implemented directly in WebProcess using the shared handle?

> Source/WebKit/Platform/IPC/SharedFileHandle.h:38
> +class SharedFileHandle {

This might be a class other platforms might want to support.
It might be worth moving most of the implementation in its own source file.
That will also reduce the number of includes here.

> Source/WebKit/Platform/IPC/SharedFileHandle.h:49
> +    SharedFileHandle() = default;

Do we need this one?

> Source/WebKit/Platform/IPC/SharedFileHandle.h:56
> +        fileport_makeport(m_handle, &fileport);

Should we check fileport_makeport return value?

> Source/WebKit/Platform/IPC/SharedFileHandle.h:68
> +        return SharedFileHandle::create(fileport_makefd(machPort.port()));

Can fileport_makefd return an invalid value?
If so, should we return std::nullopt?

> Source/WebKit/Platform/IPC/SharedFileHandle.h:75
> +        : m_handle(handle)

explicit

> LayoutTests/ChangeLog:9
> +        * storage/filesystemaccess/resources/sync-access-handle-read-write.js: Added.

Can we add an API test that is doing the following:
- Have two web processes A & B.
- Get a sync file handle in one process A, open it, write some data onto it.
- Crash process A so that close is not called from process A
- Validate that B can get access to the sync file somehow.

[Sihui Liu]

(In reply to youenn fablet from comment #14)
> (In reply to Sihui Liu from comment #13)
> > The newly added test should pass if bots have internal SDK; I am not sure
> > whether I should update -expected.txt file or mark test as [Pass Failure] in
> > TestExpectations. @Youenn, any advice?
> 
> Can we forward declare the functions we are using from internal SDK if we do
> not have it?
> As an example, you can look at NSWSPI.h: it is including a private header if
> we have internal SDK and is declaring the functions webkit is using
> otherwise.

Sure, will do.

[Sihui Liu]

Created attachment 440854 [details]
Patch

[Sihui Liu]

Created attachment 440873 [details]
Patch

[Sihui Liu]

Created attachment 440876 [details]
Patch

[Sihui Liu]

Created attachment 440880 [details]
Patch

[Sihui Liu]

Created attachment 440889 [details]
Patch

[Sihui Liu]

(In reply to youenn fablet from comment #15)
> Comment on attachment 440759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440759&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Reviewed by NOBODY (OOPS!).
> 
> Worth beefing up the change log to explain what the model is (transfer file
> handle, and do operations in WebProcess?).

Sure.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:211
> > +        return FileSystemStorageError::Unknown;
> 
> Do we need getSize, flush and truncate if they will be implemented directly
> in WebProcess using the shared handle?

They will not be implemented in web process. They are async according to the current proposal. (In our current implementation, the sync read and write will be blocked if there is unfinished async operation.)

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:38
> > +class SharedFileHandle {
> 
> This might be a class other platforms might want to support.
> It might be worth moving most of the implementation in its own source file.
> That will also reduce the number of includes here.
Sure.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:49
> > +    SharedFileHandle() = default;
> 
> Do we need this one?
It seems to be needed by decoder.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:56
> > +        fileport_makeport(m_handle, &fileport);
> 
> Should we check fileport_makeport return value?

Will add a check.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:68
> > +        return SharedFileHandle::create(fileport_makefd(machPort.port()));
> 
> Can fileport_makefd return an invalid value?
> If so, should we return std::nullopt?

I think if we fail at fileport_makefd, we may just return an invalid handle (because the ipc message is decoded correctly). Will add a check.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:75
> > +        : m_handle(handle)
> 
> explicit

Will add.

> 
> > LayoutTests/ChangeLog:9
> > +        * storage/filesystemaccess/resources/sync-access-handle-read-write.js: Added.
> 
> Can we add an API test that is doing the following:
> - Have two web processes A & B.
> - Get a sync file handle in one process A, open it, write some data onto it.
> - Crash process A so that close is not called from process A
> - Validate that B can get access to the sync file somehow.

Will add.

[Sihui Liu]

Created attachment 440898 [details]
Patch

[youenn fablet]

Comment on attachment 440898 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:1071
> +#endif

Not needed anymore?

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:110
> +{

Should we ASSERT(!isMainThread());

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:118
> +        return Exception { NotAllowedError };

Why do we use an optional but mark it as not allowed if not available?
Can we change buffer to just a BufferSource&?
If not, a test would be good.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:122
> +        return Exception { InvalidStateError, "Failed to read at offset"_s };

Do we have tests for this case?

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:129
> +    });

If we change buffer to be a BufferSource, maybe can we add a mutableData() getter instead.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:131
> +    auto size = source.length();

size not really needed.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:139
> +ExceptionOr<unsigned long long> FileSystemSyncAccessHandle::write(std::optional<BufferSource::VariantType> buffer, FileSystemSyncAccessHandle::FilesystemReadWriteOptions options)

I would guess a const BufferSource& would be better here if possible.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:141
> +    if (m_file == FileSystem::invalidPlatformFileHandle || m_isClosed)

Should we ASSERT(!isMainThread());

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:145
> +        return Exception { InvalidStateError, "Access handle has unfinished operation"_s };

Do we have a test for this case?

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:148
> +        return Exception { NotAllowedError };

Ditto.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:152
> +        return Exception { InvalidStateError, "Failed to write at offset"_s };

Do we have a test for this case?

> Source/WebCore/PAL/pal/spi/cocoa/FilePortSPI.h:28
> +#if HAVE(FILEPORT)

I would hope #if USE(APPLE_INTERNAL_SDK) is sufficient?

> Source/WebCore/PAL/pal/spi/cocoa/FilePortSPI.h:30
> +extern "C" {

Is that needed, I would hope fileport.h to do it for us?

> Source/WebKit/Platform/IPC/SharedFileHandle.cpp:33
> +#if HAVE(FILEPORT)

I am not sure we need this one here, otherwise EWS bots will not be able to run tests.
If needed, I would change it to !PLATFORM(COCOA)

> Source/WebKit/Platform/IPC/SharedFileHandle.h:39
> +    SharedFileHandle() = default;

I would add an empty line between constructor and getter.

[Sihui Liu]

Comment on attachment 440898 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:1071
>> +#endif
> 
> Not needed anymore?

Do you mean replace HAVE(FILEPORT) with USE(APPLE_INTERNAL_SDK)?

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:110
>> +{
> 
> Should we ASSERT(!isMainThread());

Sure.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:118
>> +        return Exception { NotAllowedError };
> 
> Why do we use an optional but mark it as not allowed if not available?
> Can we change buffer to just a BufferSource&?
> If not, a test would be good.

Will change to just BufferSource.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:122
>> +        return Exception { InvalidStateError, "Failed to read at offset"_s };
> 
> Do we have tests for this case?

Will add.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:129
>> +    });
> 
> If we change buffer to be a BufferSource, maybe can we add a mutableData() getter instead.

Sure.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:131
>> +    auto size = source.length();
> 
> size not really needed.

It's used in FileSystem::readFromFile. Or do you mean to replace it with something?

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:139
>> +ExceptionOr<unsigned long long> FileSystemSyncAccessHandle::write(std::optional<BufferSource::VariantType> buffer, FileSystemSyncAccessHandle::FilesystemReadWriteOptions options)
> 
> I would guess a const BufferSource& would be better here if possible.

Sure.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:141
>> +    if (m_file == FileSystem::invalidPlatformFileHandle || m_isClosed)
> 
> Should we ASSERT(!isMainThread());

Sure.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:145
>> +        return Exception { InvalidStateError, "Access handle has unfinished operation"_s };
> 
> Do we have a test for this case?

Yes, last test in LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:148
>> +        return Exception { NotAllowedError };
> 
> Ditto.

Will remove.

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:152
>> +        return Exception { InvalidStateError, "Failed to write at offset"_s };
> 
> Do we have a test for this case?

Will add.

>> Source/WebCore/PAL/pal/spi/cocoa/FilePortSPI.h:28
>> +#if HAVE(FILEPORT)
> 
> I would hope #if USE(APPLE_INTERNAL_SDK) is sufficient?

Will change.

>> Source/WebCore/PAL/pal/spi/cocoa/FilePortSPI.h:30
>> +extern "C" {
> 
> Is that needed, I would hope fileport.h to do it for us?

This is not needed.

>> Source/WebKit/Platform/IPC/SharedFileHandle.cpp:33
>> +#if HAVE(FILEPORT)
> 
> I am not sure we need this one here, otherwise EWS bots will not be able to run tests.
> If needed, I would change it to !PLATFORM(COCOA)

Will change.

>> Source/WebKit/Platform/IPC/SharedFileHandle.h:39
>> +    SharedFileHandle() = default;
> 
> I would add an empty line between constructor and getter.

Sure.

[Sihui Liu]

Created attachment 440954 [details]
Patch

[youenn fablet]

Comment on attachment 440954 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:58
> +    m_source->truncate(m_identifier, size, [weakThis = makeWeakPtr(*this), promise = WTFMove(promise)](auto result) mutable {

Since we have a file descriptor, we could well do truncate in WebProcess as well?
This would be faster and easier to do.
Ditto for getSize and flush?

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:226
> +                crossThreadResult = crossThreadCopy(result.exception());

Can we do result = crossThreadCopy(result) in the lambda?

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1548
> +}

Does not seem needed in this patch, if we need it let's split it in its own patch.

> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js:18
> +function arrayBufferToString(buffer)

You can use TextDecoder instead.

> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js:23
> +function stringToArrayBuffer(string)

You can use TextEncoder instead.

[Sihui Liu]

Comment on attachment 440954 [details]
Patch

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

>> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:58
>> +    m_source->truncate(m_identifier, size, [weakThis = makeWeakPtr(*this), promise = WTFMove(promise)](auto result) mutable {
> 
> Since we have a file descriptor, we could well do truncate in WebProcess as well?
> This would be faster and easier to do.
> Ditto for getSize and flush?

Yes, I think we will be able to flush, truncate and get size with file descriptor. These operations seems to be async (returning a Promise) according to the proposal, so I tend to keep how they are now in case there's spec change.

>> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:226
>> +                crossThreadResult = crossThreadCopy(result.exception());
> 
> Can we do result = crossThreadCopy(result) in the lambda?

I am not sure how to make a cross-thread copy of PlatformFileHandle of other platforms: e.g. GFileIOStream*. (Though they will not actually be used here, we need to provide a copy function if we do crossThreadCopy(result)?)

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1548
>> +}
> 
> Does not seem needed in this patch, if we need it let's split it in its own patch.

This is used in the API test.

>> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js:18
>> +function arrayBufferToString(buffer)
> 
> You can use TextDecoder instead.

Sure, will change.

>> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-read-write.js:23
>> +function stringToArrayBuffer(string)
> 
> You can use TextEncoder instead.

Sure, will change.

[youenn fablet]

(In reply to Sihui Liu from comment #28)
> Comment on attachment 440954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440954&action=review
> 
> >> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:58
> >> +    m_source->truncate(m_identifier, size, [weakThis = makeWeakPtr(*this), promise = WTFMove(promise)](auto result) mutable {
> > 
> > Since we have a file descriptor, we could well do truncate in WebProcess as well?
> > This would be faster and easier to do.
> > Ditto for getSize and flush?
> 
> Yes, I think we will be able to flush, truncate and get size with file
> descriptor. These operations seems to be async (returning a Promise)
> according to the proposal, so I tend to keep how they are now in case
> there's spec change.

It seems better to just do all file operations of a given descriptor in the same process.
We can execute these operations in a separate WorkQueue if we fear of blocking the worker thread.

[Sihui Liu]

Created attachment 440976 [details]
Patch for landing

[Sihui Liu]

Created attachment 440997 [details]
Patch for landing

[EWS]

Committed r284059 (242888@main): <https://commits.webkit.org/242888@main>

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