1809066 - Check the file size after closing RemoteQuotaObjectParent

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P2)

Description

[Jan Varga [:janv]]

Check that content properly used MaybeUpdateSize before each write, so the file size on disk matches mCanonicalQuotaObject::mSize. If the size doesn't match, do necessary adjustments. This is especially important when a content process suddenly crashes while it was writing to a quota managed file.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1809066 - Check the file size after closing RemoteQuotaObjectParent; r=#dom-storage

Depends on D166347

Comment 2

[Jan Varga [:janv]]

Other quota clients don't have this problem because they write to files in the main process only, so if the main process crashes, entire app goes down. Quota information will be loaded/reconstructed during storage initialization when the app is started again.
OPFS writes to files in content processes and uses IPC to update usage and check limits in the parent process. If a content process crashes after increasing usage in the parent, but before writing to the file (or writing only partial data), the parent will have incorrect information about usage for given file which will lead to problems with usage tracking and quota checks when the file is used again by a new content process. That can manifest as a debug only assertion, diagnostic assertion or reporting wrong usage. I believe this should be fixed before enabling OPFS on Release.

Comment 3

[Jari Jalkanen]

The solution for OPFS proposed in https://phabricator.services.mozilla.com/D162584 is

• to record a flag for each file locked for writing, and
• clear the flag when all usage records are updated and the file is unlocked.
• If there is a crash, the flag will remain.
• The flags are collected and usage records updated during the page load and if an attempt is made to lock the file for writing.
• If the usage still cannot be updated, the write lock cannot be acquired and the usage will not increase.

In this model, the latest known usage will remain until an update can be made or the user deletes the file. I think we should avoid deleting the files as much as it is possible as it is hard to estimate what damage this might cause to the user, even though it is technically allowed by the storage specification.

Comment 4

[Jan Varga [:janv]]

The content process lying case doesn't depend on a content process crash. A compromised content process can somehow avoid quota checks and create huge files and can eventually end up with filled disk. We can't delay handling of such situation. If a content process is not compromised, it will always use quota stream wrappers for writing to a file, so it will always check quota before any file write operation preventing the file to grow over the limits. This guarantees that the final MaybeUpdateSize call in RemoteQuotaObjectParent::CheckFileAfterClose will always return true. It can only return false if the content process avoided quota checks.

Comment 5

[Jan Varga [:janv]]

(In reply to Jari Jalkanen from comment #3)

The solution for OPFS proposed in https://phabricator.services.mozilla.com/D162584 is

• to record a flag for each file locked for writing, and
• clear the flag when all usage records are updated and the file is unlocked.
• If there is a crash, the flag will remain.
• The flags are collected and usage records updated during the page load and if an attempt is made to lock the file for writing.
• If the usage still cannot be updated, the write lock cannot be acquired and the usage will not increase.

In this model, the latest known usage will remain until an update can be made or the user deletes the file. I think we should avoid deleting the files as much as it is possible as it is hard to estimate what damage this might cause to the user, even though it is technically allowed by the storage specification.

That patch handles something else. It handles incorrect file usage stored in the OPFS database after a main process crash. So when the entire app crashes and there's was an open sync access handle or writable file stream we need to update the usage in the database based on the file size to provide correct information to quota manager during temporary storage initialization.

The patch attached to this bug handles content process crashes and the usage which is updated directly in quota manager. When a content process crashes the file usage in the OPFS database will be correctly updated during unlocking in the data manager (the unlocking will be correctly triggered from the ActorDestroy method which is called either when a protocol is correctly closed or entire content process crashes).

Comment 6

[Jan Varga [:janv]]

I forgot to mention that with the current architecture we first call RemoteQuotaObjectParent::CheckFileAfterClose, so any unexpected huge file is removed first and then we unlock the file which will update the file usage in the OPFS database.

Comment 7

[Jan Varga [:janv]]

And I apologize for not mentioning the content process lying case in the bug description, that came a bit later when I was working on the patch. So the patch actually handles two cases:

1. Sudden content process crash
2. Content process avoids quota checks

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bce12b7e4e40
Check the file size after closing RemoteQuotaObjectParent; r=dom-storage-reviewers,jari

Comment 9

[Cristian Tuns]

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