1798459 - Streamline WritableFileStream implementation

Status: RESOLVED FIXED

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

Description

[Jan Varga [:janv]]

Some parts of the WritableFileStream implementation are based on an older SyncAccessHandle implementation. Actually, OPFS implementation has changed a bit overall. We need to bring WritableFileStream up to date.

This bug will also cover some WritableFileStream bugs fixes and cleanup.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Move StreamAlgorithms to the cpp file and rename it to WritableFileStreamUnderlyingSinkAlgorithms; r=#dom-storage

Depends on D160897

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Remove unused FileSystemWritableFileStream::mRequestHandler; r=#dom-storage

Depends on D160901

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Add FileSystemWritableFileStream::mManager to cycle collection; r=#dom-storage

Depends on D160903

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Remove dead code related to blobs; r=#dom-storage

Depends on D160904

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Clean up FileSystemWritableFileStream construction code; r=#dom-storage

Depends on D160905

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Separate out FileSystemWritableFileStream::Close and IsClosed which are not part of the WebIDL interface; r=#dom-storage

Depends on D160906

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Make FileSystemWritableFileStream::DoSeek a private method; r=#dom-storage

Depends on D160907

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Synchronize the order of WebIDL methods between the h and cpp file; r=#dom-storage

Depends on D160908

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Move FileSystemWritableFileStream::ClearActor to the correct place; r=#dom-storage

Depends on D160909

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Clean up FileSystemWritableFileStream member variables; r=#dom-storage

Having protected members in a final class doesn't make sense.

Depends on D160910

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Fix dom/fs/api to build in hybrid/non-unified mode; r=#dom-storage

Depends on D160911

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Share origin metadata between TestFileSystemDataManager.cpp and TestFileSystemDataManagerVersion001.cpp; r=#dom-storage

Depends on D160912

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Remove unnecessary include directives; r=#dom-storage

Depends on D160913

Comment 14

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Fix TestFileSystemDatabaseManagerVersion001 perma gtest failure; r=#dom-storage

Depends on D160914

Comment 15

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Use a dedicated close message for closing writable file streams; r=#dom-storage

Depends on D160915

Comment 16

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Run test_writableFileStream.js in the mochitest context and in the main thread xpcshell context; r=#dom-storage

Depends on D161023

Comment 17

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Add a shutdown check to FileSystemRequestHandler::GetWritable; r=#dom-storage

Depends on D161032

Comment 18

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Remove dead code related to now removed mTaskQueue; r=#dom-storage

Depends on D161023

Comment 19

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Support writes with length less than 2GB for now; r=#dom-storage

Depends on D161268

Comment 20

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Close parent side file handles to unlock child side. r=#dom-storage

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Call SetStream on actor only when FileSystemWritableFileStream was successfully created; r=#dom-storage

This patch also moves a similar call for SyncAccessHandle.

Depends on D161171

Comment 22

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Disable WritableFileStream on the main thread; r=#dom-storage

WritableFileStream can be enabled on the main thread only when bug 1798513 gets fixed.

Depends on D161274

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Reduce code duplication and data copying in FileSystemWritableFileStream.cpp; r=#dom-storage

Depends on D161354

Comment 24

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Switch WritableFileStream to exclusive locks; r=#dom-storage

Shared locks can't be used without copy-on-open and replace-on-close support.

Depends on D161274

Comment 25

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Clean up IPDL changes; r=#dom-storage

Depends on D161539

Comment 26

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Null check actor before sending the close message; r=#dom-storage

Depends on D161540

Comment 27

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Move autoUnlock.release() to correct place; r=#dom-storage

Depends on D161541

Comment 28

[Jan Varga [:janv]]

Attachment: Bug 1798459 - Move TruncFile to FileSystemWritableFileStream.cpp; r=#dom-storage

There are no other callers anymore.

Comment 29

[Jan Varga [:janv]]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f768630d046c
Move StreamAlgorithms to the cpp file and rename it to WritableFileStreamUnderlyingSinkAlgorithms; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/da1781f32050
Remove unused FileSystemWritableFileStream::mRequestHandler; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/21d087d3ca5d
Add FileSystemWritableFileStream::mManager to cycle collection; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/f92f19dc1a9f
Remove dead code related to blobs; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/db0c3d15084b
Clean up FileSystemWritableFileStream construction code; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/5bfb07498c30
Separate out FileSystemWritableFileStream::Close and IsClosed which are not part of the WebIDL interface; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8c6510e97a9c
Make FileSystemWritableFileStream::DoSeek a private method; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/e3740924348f
Synchronize the order of WebIDL methods between the h and cpp file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/7e8b4f40c31d
Move FileSystemWritableFileStream::ClearActor to the correct place; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/3ed207332879
Clean up FileSystemWritableFileStream member variables; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/cc31ee7652a8
Share origin metadata between TestFileSystemDataManager.cpp and TestFileSystemDataManagerVersion001.cpp; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/9f4f237f33bb
Remove unnecessary include directives; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/326b8b13aca3
Fix TestFileSystemDatabaseManagerVersion001 perma gtest failure; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/985f88adbc8e
Use a dedicated close message for closing writable file streams; r=dom-storage-reviewers,jesup,jari
https://hg.mozilla.org/integration/autoland/rev/4f07a77a4abd
Remove dead code related to now removed mTaskQueue; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/5e9ca1b1c95c
Support writes with length less than 2GB for now; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/0f197d402d63
Close parent side file handles to unlock child side. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/1a86d4746c65
Run test_writableFileStream.js in the mochitest context and in the main thread  xpcshell context; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/7d2ffb13d1cc
Add a shutdown check to FileSystemRequestHandler::GetWritable; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/243631cf4913
Call SetStream on actor only when FileSystemWritableFileStream was successfully created; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/d9b3c17a5549
Switch WritableFileStream to exclusive locks; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/3b1eaafa31d2
Clean up IPDL changes; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/b68c63983ae7
Null check actor before sending the close message; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/b371599177b6
Move autoUnlock.release() to correct place; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/179d6204b3bb
Reduce code duplication and data copying in FileSystemWritableFileStream.cpp; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/83b1e1b3c168
Move TruncFile to FileSystemWritableFileStream.cpp; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/89c3a814819c
Disable WritableFileStream on the main thread; r=dom-storage-reviewers,jesup

Comment 31

[Atila Butkovits]

Backed out for causing failures at fs/FileSystemBaseHandle-IndexedDB.

Backout link: https://hg.mozilla.org/integration/autoland/rev/1576f7f93a3cb5eecdcae6118dad5f97a4904a05

Push where failures started: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=XNZhKi7gTkag7NZ67rv8pA.0&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=windows%2C10%2Cx86%2C2004%2Cwebrender%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-windows10-32-2004-qr%2Fopt-web-platform-tests%2Cwpt2&revision=d920e7aa41126a6c378ac92aebf282ebd071659a

Failure log: https://treeherder.mozilla.org/logviewer?job_id=395997253&repo=autoland&lineNumber=8047

Comment 32

[Jari Jalkanen]

Attachment: Bug 1798459 - Restore seek position after truncate on Windows. r=#dom-storage

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86ecad8aadd4
Move StreamAlgorithms to the cpp file and rename it to WritableFileStreamUnderlyingSinkAlgorithms; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/cd8399747baa
Remove unused FileSystemWritableFileStream::mRequestHandler; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/f76d3f9a70ff
Add FileSystemWritableFileStream::mManager to cycle collection; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/a2b17e11f763
Remove dead code related to blobs; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/43334f1adcfc
Clean up FileSystemWritableFileStream construction code; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8909f236b89a
Separate out FileSystemWritableFileStream::Close and IsClosed which are not part of the WebIDL interface; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/09d161716d5b
Make FileSystemWritableFileStream::DoSeek a private method; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/096b076e4164
Synchronize the order of WebIDL methods between the h and cpp file; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8a062cb0dbb6
Move FileSystemWritableFileStream::ClearActor to the correct place; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/1ea3643ba7bc
Clean up FileSystemWritableFileStream member variables; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8f1aa390cdf6
Share origin metadata between TestFileSystemDataManager.cpp and TestFileSystemDataManagerVersion001.cpp; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/da811fc519cf
Remove unnecessary include directives; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/09b9a623d1ca
Fix TestFileSystemDatabaseManagerVersion001 perma gtest failure; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/a030bc5bdd55
Use a dedicated close message for closing writable file streams; r=dom-storage-reviewers,jesup,jari
https://hg.mozilla.org/integration/autoland/rev/6fe657e916d7
Remove dead code related to now removed mTaskQueue; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/ad5ac806f30f
Support writes with length less than 2GB for now; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/c4622058ea12
Close parent side file handles to unlock child side. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/6e11fb0b4a90
Run test_writableFileStream.js in the mochitest context and in the main thread  xpcshell context; r=dom-storage-reviewers,jari
https://hg.mozilla.org/integration/autoland/rev/8e1a8864f3bf
Add a shutdown check to FileSystemRequestHandler::GetWritable; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/1f130aa6b2c8
Call SetStream on actor only when FileSystemWritableFileStream was successfully created; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/82e9af33502d
Switch WritableFileStream to exclusive locks; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/541a5807abed
Clean up IPDL changes; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/345e9946ae68
Null check actor before sending the close message; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/2fc83f8aebc5
Move autoUnlock.release() to correct place; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/05fe0dbea21a
Reduce code duplication and data copying in FileSystemWritableFileStream.cpp; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/a1f6ec76a0c8
Move TruncFile to FileSystemWritableFileStream.cpp; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/cc1d0692cbd0
Restore seek position after truncate on Windows. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/4313a4577ade
Disable WritableFileStream on the main thread; r=dom-storage-reviewers,jesup

Comment 34

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/86ecad8aadd4
https://hg.mozilla.org/mozilla-central/rev/cd8399747baa
https://hg.mozilla.org/mozilla-central/rev/f76d3f9a70ff
https://hg.mozilla.org/mozilla-central/rev/a2b17e11f763
https://hg.mozilla.org/mozilla-central/rev/43334f1adcfc
https://hg.mozilla.org/mozilla-central/rev/8909f236b89a
https://hg.mozilla.org/mozilla-central/rev/09d161716d5b
https://hg.mozilla.org/mozilla-central/rev/096b076e4164
https://hg.mozilla.org/mozilla-central/rev/8a062cb0dbb6
https://hg.mozilla.org/mozilla-central/rev/1ea3643ba7bc
https://hg.mozilla.org/mozilla-central/rev/8f1aa390cdf6
https://hg.mozilla.org/mozilla-central/rev/da811fc519cf
https://hg.mozilla.org/mozilla-central/rev/09b9a623d1ca
https://hg.mozilla.org/mozilla-central/rev/a030bc5bdd55
https://hg.mozilla.org/mozilla-central/rev/6fe657e916d7
https://hg.mozilla.org/mozilla-central/rev/ad5ac806f30f
https://hg.mozilla.org/mozilla-central/rev/c4622058ea12
https://hg.mozilla.org/mozilla-central/rev/6e11fb0b4a90
https://hg.mozilla.org/mozilla-central/rev/8e1a8864f3bf
https://hg.mozilla.org/mozilla-central/rev/1f130aa6b2c8
https://hg.mozilla.org/mozilla-central/rev/82e9af33502d
https://hg.mozilla.org/mozilla-central/rev/541a5807abed
https://hg.mozilla.org/mozilla-central/rev/345e9946ae68
https://hg.mozilla.org/mozilla-central/rev/2fc83f8aebc5
https://hg.mozilla.org/mozilla-central/rev/05fe0dbea21a
https://hg.mozilla.org/mozilla-central/rev/a1f6ec76a0c8
https://hg.mozilla.org/mozilla-central/rev/cc1d0692cbd0
https://hg.mozilla.org/mozilla-central/rev/4313a4577ade

Comment 35

[Jari Jalkanen]

Attachment: Bug 1798459 - Re-enable writable stream and its tests on main thread. r=#dom-storage

Depends on D162318

Comment 36

[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 37

[Phabricator Automation]

Comment on attachment 9303937 [details]
Bug 1798459 - Re-enable writable stream and its tests on main thread. r=#dom-storage

Revision D162319 was moved to bug 1802279. Setting attachment 9303937 [details] to obsolete.

Comment 38

[Jan Varga [:janv]]

Landed successfully in the end.