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](https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/third_party/sqlite3/src/sqlite3.c#63220-63237) 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. Places uses 4MiB, local storage is using 1,5MiB, dom cache is setting it to 512KiB. 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?
Bug 1820478 Comment 0 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
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](https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/third_party/sqlite3/src/sqlite3.c#63220-63237) 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. Places uses 4MiB, local storage is using 1,5MiB, dom cache is setting it to 512KiB. 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?
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](https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/third_party/sqlite3/src/sqlite3.c#63220-63237) 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. 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?
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](https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/third_party/sqlite3/src/sqlite3.c#63220-63237) 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?