1820478 - Shutdown crash while Sqlite is trying to remove -shm or -wal files

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

Most (80% in Nighly) of the crash reports in Bug 1395542 show a mozStorage thread (likely the main places connection helper thread) in DeleteFileW invoked by sqlite3WalClose(Wal*, sqlite3*, int, int, unsigned char*).

We should either stop awaiting for the conn.close() callback, and assume once we call it we're done, or use SQLITE_FCNTL_PERSIST_WAL that on shutdown persists -wal and -shm.

The former won't solve similar issues in other consumers.

The latter may have advantages in both shutdown and startup, while the disadvantage is those files persisting in the profile folder.
It's unclear from the Sqlite documentation whether SQLITE_FCNTL_PERSIST_WAL may increase the risk of corruption (especially if the user copies only the .sqlite file, without the auxiliary files), from a quick read of the code it sounds like if journal_size_limit is set, it will pretty much replace Delete with Truncate, that should be safe enough. And we could set SQLITE_DEFAULT_JOURNAL_SIZE_LIMIT as a whole. That would also prevent polluting the profile folder.

Thus, I'd be prone to:

1. Set SQLITE_DEFAULT_JOURNAL_SIZE_LIMIT, this is not an hard limiter, the journal can grow over it when necessary, but if it does Sqlite will truncate it once the journal is not hot anymore. Places uses 4MiB, local storage is using 1,5MiB, dom cache is setting it to 512KiB (other consumers completely forgot about it).
   Truncating too often has a cost, due to the truncate + next grow operations, thus I suspect the dom cache limit is a bit small. We could use a default value that is sufficient for dom storage and dom cache and remove that setting from there. 1,5MB sounds good to me.
   Consumers could still overwrite it, if wanted.
2. use SQLITE_FCNTL_PERSIST_WAL in the baseVFS so we truncate instead of deleting.

Andrew, what do you think?

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

These both sound like good suggestions! Thank you!

Re: DOM Cache, I think in general, a major design concern was about disk usage for when the Cache isn't being meaningfully used but has been touched. DOM Cache is somewhat eager about creating the database and I think that relates to the priority placed on an empty database not using a lot of disk space. Specifically, if you issue requests against the Cache API that don't add data, the database gets created even though in theory it could specialize these cases (queries, deletions). So it's probably worth keeping an eye on the impact of the changes (which the DOM storage team can do as part of the review), but I would presume it should be fine because 1) we should not be causing the WAL to grow much in the first place and 2) we do want the Cache API database to open as quickly as possible for things like ServiceWorker interception, so this would likely be a fine and beneficial trade-off.

Comment 2

[Marco Bonardo [:mak]]

Cool, thank you.
This is something we can try as soon as we merge for the next Nightly.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1820478 - Set a default Sqlite journal size limit, and persist auxiliary files. r=asuth

Set a default journal_size_limit, so journals are always truncated to a sensible
max size. Change existing consumers to just use the default, but Places that is
using a larger 4MiB limit.
Change auxiliary files (-shm, -journal, -wal, ...) persistance on disk, to
avoid the cost of creating and removing them. Since there is a journal_size_limit
they will be truncated instead of deleted.

Depends on D172016

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30946b5801d7
Set a default Sqlite journal size limit, and persist auxiliary files. r=asuth

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/30946b5801d7