Bug 232363 - Release FileSystemStorageHandle when it is not in use
Summary: Release FileSystemStorageHandle when it is not in use
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-26 20:10 PDT by Sihui Liu
Modified: 2021-10-29 10:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (32.13 KB, patch)
2021-10-26 20:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (32.13 KB, patch)
2021-10-26 22:58 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (32.12 KB, patch)
2021-10-27 09:23 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (33.76 KB, patch)
2021-10-28 15:46 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (33.71 KB, patch)
2021-10-29 10: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-26 20:10:10 PDT
...
Comment 1 Sihui Liu 2021-10-26 20:16:38 PDT
Created attachment 442564 [details]
Patch
Comment 2 Sihui Liu 2021-10-26 22:58:36 PDT
Created attachment 442570 [details]
Patch
Comment 3 Sihui Liu 2021-10-27 09:23:55 PDT
Created attachment 442596 [details]
Patch
Comment 4 youenn fablet 2021-10-28 09:58:19 PDT
Comment on attachment 442596 [details]
Patch

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

> Source/WebCore/Modules/filesystemaccess/FileSystemFileHandle.cpp:69
> +            FileSystem::closeFile(file);

If we need to do it there, we might have an issue as this task might be scheduled but never executed.
Maybe we should have a result that, if destroyed and its file is valid, would call closeFile whatever.

> Source/WebCore/Modules/filesystemaccess/WorkerFileSystemStorageConnection.cpp:81
> +        return;

If we do not have a scope, shouldn't we still call closeHandle?

> Source/WebCore/Modules/storage/StorageManager.cpp:125
> +            connection->closeHandle(identifier);

Ditto here, should we call closeHandle if the task is not executed?
Comment 5 Sihui Liu 2021-10-28 15:46:21 PDT
Created attachment 442757 [details]
Patch for landing
Comment 6 EWS 2021-10-28 16:46:29 PDT
Committed r285007 (243653@main): <https://commits.webkit.org/243653@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442757 [details].
Comment 7 Radar WebKit Bug Importer 2021-10-28 16:47:16 PDT
<rdar://problem/84784824>
Comment 8 ayumi_kojima 2021-10-29 08:47:36 PDT
Reverted r285007 for reason:

Reverting r285007 because it caused storage/filesystemaccess/sync-access-handle-basics-worker.html to constantly fail.

Committed r285030 (243674@main): <https://commits.webkit.org/243674@main>
Comment 9 Sihui Liu 2021-10-29 10:16:36 PDT
Created attachment 442834 [details]
Patch for landing
Comment 10 Sihui Liu 2021-10-29 10:23:48 PDT
Comment on attachment 442834 [details]
Patch for landing

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

Re-landing patch after making a small change to the test file.

> LayoutTests/storage/filesystemaccess/resources/sync-access-handle-basics.js:23
> +        }, () => {
> +            createFileHandle(rootHandle);

This old patch exits early here. The removeEntry() means to clean up existing test file if it exists. If the file does not exist, removeEntry may throw error, and it's fine to continue test.
Comment 11 EWS 2021-10-29 10:53:26 PDT
Committed r285041 (243685@main): <https://commits.webkit.org/243685@main>

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