1687254 - Deal with short-lived files during GetBodyUsage

Status: RESOLVED FIXED

(Core :: Storage: Cache API, defect, P2)

Description

[Simon Giesecke [:sg] [he/him]]

The IsDirectory check at https://searchfox.org/mozilla-central/rev/dac45cc7020dfddbcc937827810dd11550c07dc3/dom/cache/QuotaClient.cpp#62-63 sometimes fails with NS_ERROR_FILE_TARGET_DOES_NOT_EXIST/NS_ERROR_FILE_NOT_FOUND because the file was deleted already. In this case, the entry should just be skipped. There already exists a handling of the situation, if it existed during this check, but removing it afterwards fails.

The remainder of the handling assumes that it is a directory. This handling can stay unmodified for now, since the short-lived entries do seem to always be regular files rather than directories.

Comment 1

[Jan Varga [:janv]]

Simon, if you are planning to fix this, can you briefly describe the strategy first ?

Comment 2

[Simon Giesecke [:sg] [he/him]]

My current idea as mentioned above is:

In this case, the entry should just be skipped.

Do you see any other option?

Comment 3

[Jan Varga [:janv]]

It would be even better to load all directory entries into an array and then iterate the array. Some time ago I experienced a problem described in bug 1638831. However, I currently can't quickly reproduce it again. My conclusion was that in general it's not a good idea to modify stuff on disk during direct directory traversals. GetBodyUsage seems to remove entries during traversals.
The array could later also contain information like isDirectory, so the final consumer wouldn't have to consider the case when the file was already removed, etc.
This would provide a middle-step between current QM and QMv4 which won't do any directory traversals (only in recovery mode when the main storage.sqlite is lost).
There's already a case when we load directory entries into memory structures:
https://searchfox.org/mozilla-central/rev/dac45cc7020dfddbcc937827810dd11550c07dc3/dom/indexedDB/ActorsParent.cpp#13414
The case in bug 1638831 was/is quite concerning because if a directory traversal prematurely finishes, the temporary storage initialization doesn't have to fail because of that, but our quota management can become broken later, because we didn't load all information about files.
However, this doesn't belong directly to this bug, that's why I asked about "strategy".

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Jan Varga [:janv] from comment #3)

It would be even better to load all directory entries into an array and then iterate the array.

This deserves a separate bug (note we have centralized directory entry traversal in QuotaCommon.h now, in ReduceEachFileAtomicCancelable and CollectEachFile, where we could apply this change if we chose to). While due to the shorter time delta this probably reduces the chance of hitting this problem, but it does not solve it in general. We will still need to handle the case described here.

Bug 1638831 indeed seems somewhat concerning. As described, it sounds like a general issue of nsLocalFile::GetDirectoryEntries, but I'd rather imagine that it's a bug in our components, which just doesn't propagate the error.

Comment 5

[Jan Varga [:janv]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

(In reply to Jan Varga [:janv] from comment #3)

It would be even better to load all directory entries into an array and then iterate the array.

This deserves a separate bug (note we have centralized directory entry traversal in QuotaCommon.h now, in ReduceEachFileAtomicCancelable and CollectEachFile, where we could apply this change if we chose to). While due to the shorter time delta this probably reduces the chance of hitting this problem, but it does not solve it in general. We will still need to handle the case described here.

Sounds good.

Bug 1638831 indeed seems somewhat concerning. As described, it sounds like a general issue of nsLocalFile::GetDirectoryEntries, but I'd rather imagine that it's a bug in our components, which just doesn't propagate the error.

We will have to investigate that again at some point.

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1687254 - Add GetDirEntryKind utility function. r=#dom-workers-and-storage

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1687254 - Skip directory entries that no longer exist in GetBodyUsage. r=#dom-workers-and-storage

Depends on D102203

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/809efc5db77f
Add GetDirEntryKind utility function. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/5da3ef7c4cb9
Skip directory entries that no longer exist in GetBodyUsage. r=dom-workers-and-storage-reviewers,janv

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/809efc5db77f
https://hg.mozilla.org/mozilla-central/rev/5da3ef7c4cb9