FileSystemSyncAccessHandle should close platform file handle on close()

[Sihui Liu]

...

[Sihui Liu]

Created attachment 441994 [details]
Patch

[Sihui Liu]

Created attachment 442028 [details]
Patch

[youenn fablet]

Comment on attachment 442028 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:53
>          m_source->close(m_identifier, [](auto) { });

We should probably also make sure we close also if ScriptExecutionContext is stopped. Normally there should be no issue but it does not hurt to close sooner rather than later.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:100
> +    if (m_state == State::Closing)

Why do we need the Closing state?
It seems only useful for the promise rejection message.
I am not sure this is worth the effort.

> Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:130
> +        return Exception { InvalidStateError, "AccessHandle is being closed or closed"_s };

I would write it as "AccessHandle is closing or closed" or "AccessHandle is not active".

> LayoutTests/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt:4
> +FAIL SyncAccessHandle.read fails after SyncAccessHandle.close settles promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"

Why are we failing those tests? Are we missing something to be able to run properly sync_access_handle_test.
It would seem worthwhile to make sure we run these tests properly.

[Sihui Liu]

(In reply to youenn fablet from comment #3)
> Comment on attachment 442028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442028&action=review
> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:53
> >          m_source->close(m_identifier, [](auto) { });
> 
> We should probably also make sure we close also if ScriptExecutionContext is
> stopped. Normally there should be no issue but it does not hurt to close
> sooner rather than later.

Yeah I will follow up in a separate bugs, filed https://bugs.webkit.org/show_bug.cgi?id=231250 before. 

> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:100
> > +    if (m_state == State::Closing)
> 
> Why do we need the Closing state?
> It seems only useful for the promise rejection message.
> I am not sure this is worth the effort.

Will remove.

> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemSyncAccessHandle.cpp:130
> > +        return Exception { InvalidStateError, "AccessHandle is being closed or closed"_s };
> 
> I would write it as "AccessHandle is closing or closed" or "AccessHandle is
> not active".

Sure.

> 
> > LayoutTests/imported/w3c/web-platform-tests/file-system-access/sandboxed_FileSystemSyncAccessHandle-close.https.tentative.worker-expected.txt:4
> > +FAIL SyncAccessHandle.read fails after SyncAccessHandle.close settles promise_test: Unhandled rejection with value: object "InvalidStateError: AccessHandle is closed"
> 
> Why are we failing those tests? Are we missing something to be able to run
> properly sync_access_handle_test.
> It would seem worthwhile to make sure we run these tests properly.

I don't think we are missing things for this test. I will adjust the implementation to match the test expectation.

[Radar WebKit Bug Importer]

<rdar://problem/84517013>

[Sihui Liu]

Created attachment 442055 [details]
Patch

[EWS]

Committed r284652 (243372@main): <https://commits.webkit.org/243372@main>

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