1524698 - Extension storage should detect and replace corrupt SQLite databases

Status: NEW

(WebExtensions :: Storage, enhancement, P3)

Description

[Lina Butler (ex-Mozilla)]

A user reported in https://github.com/nt1m/livemarks/issues/147#issuecomment-459876225 that extension storage was broken because storage-sync.sqlite was corrupt. IIUC, we don't handle that in extension storage or the Kinto adapter yet.

I think, if trying to open the DB throws a corruption error (https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/toolkit/components/places/SyncedBookmarksMirror.jsm#1734-1744 is how we detect that in Sync), we could either trash or rename the file, then try to open the connection again to get unstuck.

Comment 1

[Lina Butler (ex-Mozilla)]

Actually, this is probably a better example, since the adapter uses openConnection, and Sync does something slightly different.

Comment 2

[Marco Bonardo [:mak]]

To sum up where we are with corruption handling, at this time Storage leaves to the consumer to manage its own way to recover.

In general it boils down to detecting Cr.NS_ERROR_FILE_CORRUPTED on database open, but that's not enough, indeed there is a group of corruptions that are only detected when a certain query runs.
For this, different consumers have different behaviors, Cookies just throws away the database at any error (I personally don't think it's the way to go), Places has an idle maintenance task that detects corruption by running an integrity_check, a REINDEX (if the check fails) and a second integrity_check.
The defect of that approach is that it doesn't happen in a timely manner, it's possible the user is left with a broken db for days, until the maintenance runs.
To have a better detection, we filed bug 1125157, the idea is that as soon as Storage sees a SQLITE_CORRUPT error at any call, it notifies connection observers (the consumer will have to register one) and the observer will decide how to proceed (likely remove the broken database or try to fix it first). There is some threading management risks to take into account, but it should not be extremely hard. Unfortunately we're low on resources.

Without that fix, the only thing we can suggest is to check for Cr.NS_ERROR_FILE_CORRUPTED on database open, and have an idle task running PRAGMA integrity_check on dbs, though note a failing integrity_check doesn't necessarily mean the db is corrupt, a REINDEX can often fix things, PlacesDBUtils.jsm has some example on how to run those.

Comment 3

[Marco Bonardo [:mak]]

The database ntim sent me wasn't a very bad corruption, it has one page out of order, running a vacuum over it rebuilds all the pages and fixes it. Anyway it was readable, probably the out of order page made it not expandable/writable.

It's clearly the case we don't detect on opening, this is a corruption of a random page in the middle of the db.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Mathieu,
based on comment 3 it seems that it would be reasonable to handle this kind of issues in the kinto adapter,
which is the right bugzilla component where we should move this?

Comment 5

[Mathieu Leplatre [:leplatrem]]

Unfortunately we don't have any dedicated component for the Kinto SQLite adapter.

And as far as I know WebExtensions::Storage is the only place where it's used (:glasserc opened Bug 1461435 in that component for example), since Remote Settings uses the default IndexedDB adapter.

Note: it would probably make sense to move the kinto-storage-adapter.js file to toolkit/components/extensions/ to remove confusion.