webext_storage::Store::close landed in https://github.com/mozilla/application-services/pull/3042, let's upgrade our vendored a-s version to support that.
One thing I've been thinking about over the last couple of days is how to handle errors from
teardown. Rusqlite connections have a
Drop implementation that tries to automatically close the connection, and panic if that fails. On Desktop, this will cause a crash, since we compile with
panic = "abort".
Even if we don't crash, though, dropping the last reference to a Rust
StorageSyncArea XPCOM class without calling
teardown will drop its
LazyStore, which drops its
webext_storage::Store, which drops its
rusqlite::Connection, which drops its
InnerConnection...which closes the connection on the main thread.
While this isn't ideal, I think it's OK as a last resort—especially since mozStorage does something similar. Consumers should always call
teardown explicitly, and not rely on automatic cleanup by the last reference falling out of scope. As Mark points out in this comment, "the last reference falling out of scope" is a hazy concept, because XPCOM objects in JS are GCed—meaning XPConnect will release the object sometime later, not immediately.
It's also an edge case within an edge case: 1) we'll only synchronously close the connection on the main thread if a) someone is misusing a
StorageSyncArea, which shouldn't happen given that we have our
StorageSyncService always holding one strong reference, and that's what Sync and
ExtensionStorageSync.jsm go through, or b) dispatching the
teardown runnable fails, which should only happen at shutdown, and causes a leak, anyway...and 2) we'll only crash if
close itself fails, which it shouldn't given that we serialize all operations on the task queue, and
rusqlite::Connection::close already cleans up cached statements before trying to close the connection.
That's a really, really wordy explanation for a dozen-line change, but the two issues there are subtle, and I wanted to capture them somewhere.