Bug 231466 - Implement FileSystemSyncAccessHandle read() and write()
Summary: Implement FileSystemSyncAccessHandle read() and write()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 231706
  Show dependency treegraph
 
Reported: 2021-10-08 17:29 PDT by Sihui Liu
Modified: 2021-10-13 18:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (46.27 KB, patch)
2021-10-08 17:33 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2021-10-10 15:15 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.59 KB, patch)
2021-10-10 15:33 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (49.94 KB, patch)
2021-10-10 20:29 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.27 KB, patch)
2021-10-10 20:50 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.27 KB, patch)
2021-10-10 21:10 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.27 KB, patch)
2021-10-10 21:29 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (50.27 KB, patch)
2021-10-10 21:42 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (50.27 KB, patch)
2021-10-10 22:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (50.25 KB, patch)
2021-10-10 22:27 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (50.25 KB, patch)
2021-10-10 22:42 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (78.76 KB, patch)
2021-10-11 16:34 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (78.16 KB, patch)
2021-10-11 19:54 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (77.98 KB, patch)
2021-10-11 20:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (78.61 KB, patch)
2021-10-11 20:56 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (78.61 KB, patch)
2021-10-11 22:04 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (78.79 KB, patch)
2021-10-11 23:36 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (79.01 KB, patch)
2021-10-12 10:30 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (91.10 KB, patch)
2021-10-12 14:02 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (91.16 KB, patch)
2021-10-12 15:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-10-08 17:29:27 PDT
...
Comment 1 Radar WebKit Bug Importer 2021-10-08 17:29:50 PDT
<rdar://problem/84050394>
Comment 2 Sihui Liu 2021-10-08 17:33:47 PDT Comment hidden (obsolete)
Comment 3 Sihui Liu 2021-10-10 15:15:19 PDT Comment hidden (obsolete)
Comment 4 Sihui Liu 2021-10-10 15:33:36 PDT Comment hidden (obsolete)
Comment 5 Sihui Liu 2021-10-10 20:29:05 PDT Comment hidden (obsolete)
Comment 6 Sihui Liu 2021-10-10 20:50:15 PDT Comment hidden (obsolete)
Comment 7 Sihui Liu 2021-10-10 21:10:42 PDT Comment hidden (obsolete)
Comment 8 Sihui Liu 2021-10-10 21:29:27 PDT Comment hidden (obsolete)
Comment 9 Sihui Liu 2021-10-10 21:42:35 PDT Comment hidden (obsolete)
Comment 10 Sihui Liu 2021-10-10 22:15:22 PDT Comment hidden (obsolete)
Comment 11 Sihui Liu 2021-10-10 22:27:31 PDT Comment hidden (obsolete)
Comment 12 Sihui Liu 2021-10-10 22:42:38 PDT
Created attachment 440759 [details]
Patch
Comment 13 Sihui Liu 2021-10-10 23:43:43 PDT
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?
Comment 14 youenn fablet 2021-10-11 02:02:51 PDT
(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.
Comment 15 youenn fablet 2021-10-11 02:12:57 PDT
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.
Comment 16 Sihui Liu 2021-10-11 10:06:40 PDT
(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.
Comment 17 Sihui Liu 2021-10-11 16:34:02 PDT
Created attachment 440854 [details]
Patch
Comment 18 Sihui Liu 2021-10-11 19:54:28 PDT
Created attachment 440873 [details]
Patch
Comment 19 Sihui Liu 2021-10-11 20:16:46 PDT
Created attachment 440876 [details]
Patch
Comment 20 Sihui Liu 2021-10-11 20:56:11 PDT
Created attachment 440880 [details]
Patch
Comment 21 Sihui Liu 2021-10-11 22:04:34 PDT
Created attachment 440889 [details]
Patch
Comment 22 Sihui Liu 2021-10-11 23:17:51 PDT
(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.
Comment 23 Sihui Liu 2021-10-11 23:36:14 PDT
Created attachment 440898 [details]
Patch
Comment 24 youenn fablet 2021-10-12 01:18:45 PDT
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.
Comment 25 Sihui Liu 2021-10-12 10:28:19 PDT
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.
Comment 26 Sihui Liu 2021-10-12 10:30:07 PDT
Created attachment 440954 [details]
Patch
Comment 27 youenn fablet 2021-10-12 11:09:46 PDT
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.
Comment 28 Sihui Liu 2021-10-12 11:36:06 PDT
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.
Comment 29 youenn fablet 2021-10-12 11:46:31 PDT
(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.
Comment 30 Sihui Liu 2021-10-12 14:02:55 PDT
Created attachment 440976 [details]
Patch for landing
Comment 31 Sihui Liu 2021-10-12 15:16:34 PDT
Created attachment 440997 [details]
Patch for landing
Comment 32 EWS 2021-10-12 16:36:03 PDT
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].