1920270 - Consider executing Skv `nsIKeyValueDatabase` operations serially

Status: NEW

(Core :: SQLite and Embedded Database Bindings, task, P3)

Description

[Lina Butler (ex-Mozilla)]

Rob pointed out that this logic in ExtensionPermissions assumes that nsIKeyValueDatabase operations execute serially [1].

This is an observable (though undocumented) behavior of rkv-backed databases: their operations run on a private background task queue. But Skv databases use moz_task::spawn_blocking to run their operations, which can run concurrently. This difference means that the ExtensionPermissions logic will now race: the key / value pair might be deleted first, and await removed will resolve to null.

Should Skv match rkv's behavior, and serialize its database operations? Let's talk about it!

At first glance, I think it's a good idea. We already protect the SQLite database connection with a mutex (multiple threads can share an SQLite connection, but not use it concurrently), so database operations are effectively serialized, anyway. The only difference is that, with moz_task::spawn_blocking, it's unpredictable which operation will start, and take the mutex, first. But once it starts, all other operations—with three exceptions—will need to wait on it.

That said, I can think of a few downsides:

1. It's inconsistent with how other asynchronous operations behave. For example, if you started two fetch() calls concurrently, or did a PlacesUtils.bookmarks.fetch() followed by a concurrent PlacesUtils.bookmarks.remove(), the execution order isn't guaranteed. The latter is actually a perfect analogy: if you called PU.b.fetch() without awaiting it, then called PU.b.remove(), the bookmark might be deleted by the time you await the PU.b.fetch() promise.

2. nsIKeyValueDatabase.enumerate() uses a second, read-only connection to read from the database, so that batched reads don't need to wait on writes [2]. These would need to run concurrently; serializing these operations would lose the benefit of a separate reader, because now enumerate() would need to wait on writes and other reads.

3. Closing a database, either explicitly via nsIKeyValueDatabase.close() or implicitly at shutdown, should prevent any more database operations on the connection from starting—even ones that are already queued, but not yet started. close() would need to run concurrently for that to work.

4. Any Skv nsIKeyValueDatabase operation can trigger a maintenance check, and close the database automatically if the check detects corruption. These checks can take a while, so we want any other operations started during maintenance to fail immediately with a "busy" error—and not to have them wait on maintenance to finish. This only makes sense in a world where operations can run concurrently; if they were serial, they'd need to wait on maintenance to finish.

Earlier, I posted a sketch of how we could solve this...but that sketch actually had a bug; it wouldn't have solved our problem. With these complications in mind, I wonder if it's better to leave this bug as WONTFIX, and change the code in ExtensionPermissions to:

let removed = await store.get(extensionId);
await store.delete(extensionId);

[1]: For this specific snippet in ExtensionPermissions, I wonder if either changing delete() to return the removed key/value pair; adding a new method like nsIKeyValueDatabase.take() to delete and return the old value; or even prioritizing a transaction API (bug 1499238) would actually be the more targeted fix...but that feels orthogonal to the discussion in this bug.

[2]: I'd like to make this a little more explicit in the future, but that's also outside the scope of this bug.