Bug 234520 – Ensure file handles used in FileSystemAccess API are closed

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

[Sihui Liu]

...

[Sihui Liu]

Created attachment 447617 [details]
Patch

[Sihui Liu]

Created attachment 447619 [details]
Patch

[Darin Adler]

Comment on attachment 447619 [details]
Patch

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

> Source/WTF/wtf/CrossThreadCopier.h:182
> +    static Type copy(Type&& source)
> +    {
> +        return std::make_pair(CrossThreadCopier<F>::copy(WTFMove(source.first)), CrossThreadCopier<S>::copy(WTFMove(source.second)));
> +    }

Do we really want to call a cross-thread move operation "copy"? Maybe there’s no way to avoid it, but it seems wrong.

> Source/WebCore/dom/ExceptionOr.h:222
> +    static Type copy(Type&& source)

Same thought. This is a move, not a a copy, I think.

> Source/WebCore/platform/FileHandle.h:37
> +    WTF_MAKE_NONCOPYABLE(FileHandle);

I am not sure it’s better to use this macro than deleting the copy and assignment operators. We invented this macro back when there was no simple way to write this in a class. I kind of wish we didn’t use a macro for this, since it’s now a pretty simple idiom.

> Source/WebCore/platform/FileHandle.h:39
> +    static FileHandle adopt(FileSystem::PlatformFileHandle&);

This should take an rvalue reference, not an lvalue reference. That way we can pass it a file handle returned by a function, and don’t require using a local variable for it.

> Source/WebCore/platform/FileHandle.h:56
> +    FileHandle isolatedCopy() &&;

This operation is a move, not a copy, I think. Not sure it should be named isolatedCopy.

> Source/WebCore/platform/FileHandle.h:59
> +    FileHandle(FileSystem::PlatformFileHandle);

This function should take an rvalue reference, and I think should do the same thing as adopt. In fact, given this can take an rvalue reference, I think we can do away with the named adopt function entirely. You’ll be able to see the adoption because the caller will use WTFMove or std::exchange.

> Source/WebKit/Platform/IPC/SharedFileHandle.h:39
> +    static std::optional<SharedFileHandle> create(FileSystem::PlatformFileHandle&);

This should take an rvalue reference, not an lvalue reference. That way we can pass it a file handle returned by a function, and don’t require using a local variable for it.

> Source/WebKit/Platform/IPC/SharedFileHandle.h:48
> +    explicit SharedFileHandle(FileSystem::PlatformFileHandle& handle)

This should take an rvalue reference, not an lvalue reference.

> Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp:34
> +std::optional<SharedFileHandle> SharedFileHandle::create(FileSystem::PlatformFileHandle& handle)

This should take an rvalue reference, not an lvalue reference.

> Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:152
> +        auto resultValue = WTFMove(result.value());

Why do we have to move this into a local variable? Can’t we just use a reference to it in place rather than moving it?

[Darin Adler]

Comment on attachment 447619 [details]
Patch

This seems like an important bug fix. Can we write a test?

[youenn fablet]

Looks good to me as well.
I do not really like using a PlatformFileHandle&.
I would be tempted to keep using PlatformFileHandle instead even though it leaves some valid PlatformFileHandle that are adopted by FileHandle.
This seems inline with other C++ movable constructs like optional.
Or use && as Darin mentionned.
 
View in context: https://bugs.webkit.org/attachment.cgi?id=447619&action=review

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:91
> +    WorkerGlobalScope::postFileSystemStorageTask([weakThis = WeakPtr { *this }, file = m_file.get(), size, workerThread = Ref { scope->thread() }]() mutable {

m_file.handle() reads better to me here.

> Source/WebCore/platform/FileHandle.cpp:-62
> -    close();

Are we sure this is not needed by some other code path?

> Source/WebCore/platform/FileHandle.cpp:165
> +const FileSystem::PlatformFileHandle& FileHandle::get() const

get is a bit mysterious, can we rename it to handle, or platformFileHandle?

> Source/WebCore/platform/FileHandle.h:-48
> -

I would keep this empty line.

> Source/WebCore/platform/FileHandle.h:55
> +    const FileSystem::PlatformFileHandle& get() const;

I would think returning by value to be good enough, since fileHandle is probably either an int or a pointer.

> Source/WebCore/platform/FileHandle.h:59
> +    FileHandle(FileSystem::PlatformFileHandle);

explicit

[Sihui Liu]

(In reply to Darin Adler from comment #3)
> Comment on attachment 447619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447619&action=review
> 
> > Source/WTF/wtf/CrossThreadCopier.h:182
> > +    static Type copy(Type&& source)
> > +    {
> > +        return std::make_pair(CrossThreadCopier<F>::copy(WTFMove(source.first)), CrossThreadCopier<S>::copy(WTFMove(source.second)));
> > +    }
> 
> Do we really want to call a cross-thread move operation "copy"? Maybe
> there’s no way to avoid it, but it seems wrong.

Yes this might be confusing; I made the change so that crossThreadCopy() can be applied on FileHandle in WorkerFileSystemConnection. I will revert this change.

> 
> > Source/WebCore/dom/ExceptionOr.h:222
> > +    static Type copy(Type&& source)
> 
> Same thought. This is a move, not a a copy, I think.

Will revert it.

> 
> > Source/WebCore/platform/FileHandle.h:37
> > +    WTF_MAKE_NONCOPYABLE(FileHandle);
> 
> I am not sure it’s better to use this macro than deleting the copy and
> assignment operators. We invented this macro back when there was no simple
> way to write this in a class. I kind of wish we didn’t use a macro for this,
> since it’s now a pretty simple idiom.

Sure, will change to use copy and assignment operators.

> 
> > Source/WebCore/platform/FileHandle.h:39
> > +    static FileHandle adopt(FileSystem::PlatformFileHandle&);
> 
> This should take an rvalue reference, not an lvalue reference. That way we
> can pass it a file handle returned by a function, and don’t require using a
> local variable for it.

Sounds good.

> 
> > Source/WebCore/platform/FileHandle.h:56
> > +    FileHandle isolatedCopy() &&;
> 
> This operation is a move, not a copy, I think. Not sure it should be named
> isolatedCopy.

Will remove this.

> 
> > Source/WebCore/platform/FileHandle.h:59
> > +    FileHandle(FileSystem::PlatformFileHandle);
> 
> This function should take an rvalue reference, and I think should do the
> same thing as adopt. In fact, given this can take an rvalue reference, I
> think we can do away with the named adopt function entirely. You’ll be able
> to see the adoption because the caller will use WTFMove or std::exchange.

Sounds good.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:39
> > +    static std::optional<SharedFileHandle> create(FileSystem::PlatformFileHandle&);
> 
> This should take an rvalue reference, not an lvalue reference. That way we
> can pass it a file handle returned by a function, and don’t require using a
> local variable for it.

Sure.

> 
> > Source/WebKit/Platform/IPC/SharedFileHandle.h:48
> > +    explicit SharedFileHandle(FileSystem::PlatformFileHandle& handle)
> 
> This should take an rvalue reference, not an lvalue reference.

Will change.

> 
> > Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp:34
> > +std::optional<SharedFileHandle> SharedFileHandle::create(FileSystem::PlatformFileHandle& handle)
> 
> This should take an rvalue reference, not an lvalue reference.

Will change.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFileSystemStorageConnection.cpp:152
> > +        auto resultValue = WTFMove(result.value());
> 
> Why do we have to move this into a local variable? Can’t we just use a
> reference to it in place rather than moving it?

We can; will make the change.

(In reply to Darin Adler from comment #4)
> Comment on attachment 447619 [details]
> Patch
> 
> This seems like an important bug fix. Can we write a test?

There's an existing test for this, where we open many files via the API, and if files are not closed properly, the process will run out of file descriptors and test would fail. We used to close the file handles manually after use so the test can pass; this patch makes sure the file will be closed and we don't need to manage the close.

[Sihui Liu]

(In reply to youenn fablet from comment #5)
> Looks good to me as well.
> I do not really like using a PlatformFileHandle&.
> I would be tempted to keep using PlatformFileHandle instead even though it
> leaves some valid PlatformFileHandle that are adopted by FileHandle.
> This seems inline with other C++ movable constructs like optional.
> Or use && as Darin mentionned.

Will update as Darin suggested.

>  
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447619&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:91
> > +    WorkerGlobalScope::postFileSystemStorageTask([weakThis = WeakPtr { *this }, file = m_file.get(), size, workerThread = Ref { scope->thread() }]() mutable {
> 
> m_file.handle() reads better to me here.

Okay, will change.

> 
> > Source/WebCore/platform/FileHandle.cpp:-62
> > -    close();
> 
> Are we sure this is not needed by some other code path?

ah nice catch; I think we need to keep this.

> 
> > Source/WebCore/platform/FileHandle.cpp:165
> > +const FileSystem::PlatformFileHandle& FileHandle::get() const
> 
> get is a bit mysterious, can we rename it to handle, or platformFileHandle?

Sure.

> 
> > Source/WebCore/platform/FileHandle.h:-48
> > -
> 
> I would keep this empty line.

Sure.

> 
> > Source/WebCore/platform/FileHandle.h:55
> > +    const FileSystem::PlatformFileHandle& get() const;
> 
> I would think returning by value to be good enough, since fileHandle is
> probably either an int or a pointer.

Okay.

> 
> > Source/WebCore/platform/FileHandle.h:59
> > +    FileHandle(FileSystem::PlatformFileHandle);
> 
> explicit

Sure.

[Sihui Liu]

Created attachment 447838 [details]
Patch

[Sihui Liu]

Created attachment 447860 [details]
Patch

[EWS]

Committed r287405 (245540@main): <https://commits.webkit.org/245540@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86857459>