1786465 - FS: Transition to a robust object and actor lifecycle management

Status: RESOLVED FIXED

(Core :: DOM: File, task, P2)

Description

[Jan Varga [:janv]]

The current lifecycle management is quite limited and doesn't provide a good way to handle the cases when a window/worker is dying or entire app is shutting down. The goal of this bug is to replace FileSystemActorHolder with a dedicated object FileSystemManager held by StorageManager. The child actor will be created only once per StorageManager which is effectively once per nsIGlobalObject.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Introduce FileSystemManager intended to be referenced by FileSystemHandle; r=#dom-storage

This patch also splits the creation of the top level protocol and getting the
root handle into separate logical steps.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Simplify FileSystemManagerChild by using the VirtualSendImpl attribute in PFileSystemManager definition; r=#dom-storage

Depends on D155351

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Replace PBackgroundFileSystem protocol with a single async message; r=#dom-storage

PBackgroundFileSystem currently contains only one async message and we don't
plan to add more messages, so the one and only message can be defined directly
on PBackground.

Depends on D155366

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Rename POriginPrivateFileSystem to PFileSystemManager; r=#dom-storage

The child actor will be held by FileSystemManager on the child side and the
parent actor already holds FileSystemDataManager, so it makes sense to rename
the protocol to PFileSystemManager.

Depends on D155367

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Create only one instance of FileSystemManagerChild per nsIGlobalObject; r=#dom-storage

This is the main lifecycle change. FileSystemHandle now holds
FileSystemManager object instead of holding FileSystemActorHolder object.
There's only one instance of FileSystemManager per StorageManager (which is
created only once for nsIGlobalObject). FileSystemActorHolder objects were
created for every StorageManager::GetDirectory call.

Depends on D155374

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Expect not null FileSystemManager in FileSystemRequestHandler methods; r=#dom-storage

Depends on D155381

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Shutdown FileSystemManager when StorageManager is shutting down; r=#dom-storage

Depends on D155382

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Add a StorageManager back reference to FileSystemManager; r=#dom-storage

This will become useful when FileSystemManager gets an invalidated during
origin clearing. In that case, we need to throw away existing FileSystemManager
and create it again later.

Depends on D155383

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Move FileSystemDataManager to own source files; r=#dom-storage

Depends on D155384

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Convert FileSystemDataManager to be ref counted; r=#dom-storage

Depends on D155550

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Start using FileSystemDataManager in FileSystemManagerParent; r=#dom-storage

Depends on D155551

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Create only one FileSystemDataManager per origin; r=#dom-storage

Depends on D155552

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Move TaskQueue ownership from FileSystemManagerParent to FileSystemDataManager; r=#dom-storage

After this change, we will create only one task queue per origin.

Depends on D155553

Comment 14

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Removed unused __delete__ message from PFileSystemManager protocol; r=#dom-storage

The message has no use in top level protocols.

Depends on D155554

Comment 15

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Clean up CreateFileSystemManagerParent; r=#dom-storage

This patch mostly removes:

• some unused lambda captures
• redundant variables like pbackground
• named lambda functions which are used only once
• obsolete comments

Depends on D155555

Comment 16

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Introduce Registered class template; r=#dom-storage

See the documentation in FileSystemHelpers.h

Depends on D155556

Comment 17

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Implement registration counting of FileSystemDataManager; r=#dom-storage

FileSystemDataManager can be now closed when it's still alive.

Depends on D155785

Comment 18

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Add support for asynchronous closing of FileSystemDataManager; r=#dom-storage

Depends on D155786

Comment 19

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Add support for asynchronous creation of FileSystemDataManager; r=#dom-storage

Depends on D155787

Comment 20

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Add support for asynchronous opening of FileSystemDataManager; r=#dom-storage

Depends on D155788

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Handle pending open/close operations in GetOrCreateFileSystemDataManager; r=#dom-storage

Depends on D155789

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceb8fe66d2b2
Introduce FileSystemManager intended to be referenced by FileSystemHandle; r=dom-storage-reviewers,jesup

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Convert OriginPrivateFileSystemChild to a standard actor; r=#dom-storage

The send methods which need to be mocked in tests are declared in
OriginPrivateFileSystemChild the same way as POriginPrivateFileSystemChild send
methods, just without the Msg suffix and virtual. The relevant
POriginPrivateFileSystemChild send methods are hidden in
OriginPrivateFileSystemChild, so they can only be used internally for the
forwarding.

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ceb8fe66d2b2

Comment 25

[Jan Varga [:janv]]

https://treeherder.mozilla.org/jobs?repo=try&revision=e904a301ac85b60d4dd1fb8b667929666371331e

Comment 26

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Convert FileSystemDataManager gtests to test stuff on PBackgronnd thread; r=#dom-storage

Depends on D156060

Comment 27

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Make sure that gDataManagers is touched on the PBackground thread only; r=#dom-storage

Depends on D156061

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16b9fd20cbf4
Convert OriginPrivateFileSystemChild to a standard actor; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/3b0da4b4bb83
Replace PBackgroundFileSystem protocol with a single async message; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/e4b3f3d8ba5f
Rename POriginPrivateFileSystem to PFileSystemManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/031dd102befc
Create only one instance of FileSystemManagerChild per nsIGlobalObject; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/ede256a0c761
Expect not null FileSystemManager in FileSystemRequestHandler methods; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/4eff22f565cc
Shutdown FileSystemManager when StorageManager is shutting down; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/f62bd47cabd3
Add a StorageManager back reference to FileSystemManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/399dce373c59
Move FileSystemDataManager to own source files; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/37b7616c8b42
Convert FileSystemDataManager to be ref counted; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/a2f6102fc8ea
Start using FileSystemDataManager in FileSystemManagerParent; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/cfa7bbd206c3
Create only one FileSystemDataManager per origin; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/256aef51d87b
Move TaskQueue ownership from FileSystemManagerParent to FileSystemDataManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/224c332142d0
Removed unused __delete__ message from PFileSystemManager protocol; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/c3840fd37212
Clean up CreateFileSystemManagerParent; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/04e60585a3f0
Introduce Registered class template; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/0256861a7ac5
Implement registration counting of FileSystemDataManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/6d1f25188b13
Add support for asynchronous closing of FileSystemDataManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/3b39bc9214e6
Add support for asynchronous creation of FileSystemDataManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/fbc09d7df86e
Add support for asynchronous opening of FileSystemDataManager; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/0cc0844f049a
Handle pending open/close operations in GetOrCreateFileSystemDataManager; r=dom-storage-reviewers,jesup

Comment 29

[Jan Varga [:janv]]

Attachment: Bug 1786465 - A follow-up fix to allow copy elision; r=#dom-storage

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14b0be678d8c
A follow-up fix to allow copy elision; r=dom-storage-reviewers,jesup

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/16b9fd20cbf4
https://hg.mozilla.org/mozilla-central/rev/3b0da4b4bb83
https://hg.mozilla.org/mozilla-central/rev/e4b3f3d8ba5f
https://hg.mozilla.org/mozilla-central/rev/031dd102befc
https://hg.mozilla.org/mozilla-central/rev/ede256a0c761
https://hg.mozilla.org/mozilla-central/rev/4eff22f565cc
https://hg.mozilla.org/mozilla-central/rev/f62bd47cabd3
https://hg.mozilla.org/mozilla-central/rev/399dce373c59
https://hg.mozilla.org/mozilla-central/rev/37b7616c8b42
https://hg.mozilla.org/mozilla-central/rev/a2f6102fc8ea
https://hg.mozilla.org/mozilla-central/rev/cfa7bbd206c3
https://hg.mozilla.org/mozilla-central/rev/256aef51d87b
https://hg.mozilla.org/mozilla-central/rev/224c332142d0
https://hg.mozilla.org/mozilla-central/rev/c3840fd37212
https://hg.mozilla.org/mozilla-central/rev/04e60585a3f0
https://hg.mozilla.org/mozilla-central/rev/0256861a7ac5
https://hg.mozilla.org/mozilla-central/rev/6d1f25188b13
https://hg.mozilla.org/mozilla-central/rev/3b39bc9214e6
https://hg.mozilla.org/mozilla-central/rev/fbc09d7df86e
https://hg.mozilla.org/mozilla-central/rev/0cc0844f049a
https://hg.mozilla.org/mozilla-central/rev/14b0be678d8c

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a28786219093
Convert FileSystemDataManager gtests to test stuff on PBackgronnd thread; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/f47b9b43da3c
Make sure that gDataManagers is touched on the PBackground thread only; r=dom-storage-reviewers,jesup

Comment 33

[Natalia Csoregi [:nataliaCs]]

Backed out along with Bug 1788182, Bug 1788178 for causing bustage on TestFileSystemDataManager.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/a2b735d7c83a08d0da187bd76e4d6744d5ce5a19
• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=f47b9b43da3c5ad9f4fe34a767c77c8c583232e7&selectedTaskRun=ZxL25bweQVCADtTycQZiyw.1
• failure log: https://treeherder.mozilla.org/logviewer?job_id=389076423&repo=autoland&lineNumber=9558

[task 2022-08-31T23:02:02.399Z] 23:02:02     INFO -  In file included from Unified_cpp_parent_datamodel0.cpp:2:0:
[task 2022-08-31T23:02:02.399Z] 23:02:02     INFO -  /builds/worker/checkouts/gecko/dom/fs/test/gtest/parent/datamodel/TestFileSystemDataManager.cpp: In static member function 'static void mozilla::dom::fs::test::TestFileSystemDataManager::SetUpTestCase()':
[task 2022-08-31T23:02:02.399Z] 23:02:02    ERROR -  /builds/worker/checkouts/gecko/dom/fs/test/gtest/parent/datamodel/TestFileSystemDataManager.cpp:85:34: error: 'mozilla::dom::fs::test::TestFileSystemDataManager::SetUpTestCase()::<lambda()>' has a field 'mozilla::dom::fs::test::TestFileSystemDataManager::SetUpTestCase()::<lambda()>::<resolver capture>' whose type uses the anonymous namespace [-Werror=subobject-linkage]
[task 2022-08-31T23:02:02.400Z] 23:02:02     INFO -                          [&resolver]() { return resolver->Done(); });
[task 2022-08-31T23:02:02.400Z] 23:02:02     INFO -                                    ^
[task 2022-08-31T23:02:02.400Z] 23:02:02     INFO -  cc1plus: all warnings being treated as errors
[task 2022-08-31T23:02:02.400Z] 23:02:02    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:669: Unified_cpp_parent_datamodel0.o] Error 1
[task 2022-08-31T23:02:02.400Z] 23:02:02     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/fs/test/gtest/parent/datamodel'
[task 2022-08-31T23:02:02.401Z] 23:02:02    ERROR -  gmake[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: dom/fs/test/gtest/parent/datamodel/target-objects] Error 2
[task 2022-08-31T23:02:02.401Z] 23:02:02     INFO -  gmake[3]: *** Waiting for unfinished jobs....

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55d79c50ca2f
Convert FileSystemDataManager gtests to test stuff on PBackgronnd thread; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/5767c2e6ca26
Make sure that gDataManagers is touched on the PBackground thread only; r=dom-storage-reviewers,jesup

Comment 35

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/55d79c50ca2f
https://hg.mozilla.org/mozilla-central/rev/5767c2e6ca26

Comment 36

[Jan Varga [:janv]]

Attachment: Bug 1786465 - Fix code to silence static-analysis warnings; r=#dom-storage

Comment 37

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14998637bb01
Simplify FileSystemManagerChild by using the VirtualSendImpl attribute in PFileSystemManager definition; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/91baa63fd25c
Fix code to silence static-analysis warnings; r=dom-storage-reviewers,jesup

Comment 39

[Sandor Molnar[:smolnar]]

Backed out for causing multiple lint failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/292d43f4f4d259a83975b0a6ea2ffee2f473b5e2

Push with failures

Failure log ESlint

Failure log py black

Failure log mozlint wpt

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b52f1a06e743
Simplify FileSystemManagerChild by using the VirtualSendImpl attribute in PFileSystemManager definition; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/1f65e7ff82af
Fix code to silence static-analysis warnings; r=dom-storage-reviewers,jesup

Comment 41

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing wpt failures . CLOSED TREE
Backout link
Push with failures
Link to failure log 1
Link to failure log 2
Failure line 1 : TEST-UNEXPECTED-FAIL | /fs/FileSystemDirectoryHandle-removeEntry.https.any.worker.html | removeEntry() while the file has an open writable fails - promise_test: Unhandled rejection with value: object "TypeError: navigator.storage.getDirectory is not a function"
Failure line 2 : TEST-UNEXPECTED-FAIL | /fs/FileSystemDirectoryHandle-removeEntry.https.any.html | removeEntry() while the file has an open writable fails - promise_test: Unhandled rejection with value: object "TypeError: navigator.storage.getDirectory is not a function"

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36c6c340fe64
Simplify FileSystemManagerChild by using the VirtualSendImpl attribute in PFileSystemManager definition; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/f1c5606ca6e0
Fix code to silence static-analysis warnings; r=dom-storage-reviewers,jesup

Comment 43

[Norisz Fay [:noriszfay]]

Backed out for causing FileSystem related wpt failures

Backout link

Push with failures

Failure log

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce78177f190e
Simplify FileSystemManagerChild by using the VirtualSendImpl attribute in PFileSystemManager definition; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/0b1048c61964
Fix code to silence static-analysis warnings; r=dom-storage-reviewers,jesup

Comment 45

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/ce78177f190e
https://hg.mozilla.org/mozilla-central/rev/0b1048c61964

Comment 46

[Jan Varga [:janv]]

Multiple patches were landing at once, it seems there were no problems with patches for this bug.
Anyway, the patch stack landed successfully in the end.