1633943 - Support tearing down a Rust `StorageSyncArea`

Status: RESOLVED FIXED

(WebExtensions :: Storage, task, P1)

Description

[Lina Butler [:lina]]

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 close/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.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1633943 - Support tearing down a Rust `StorageSyncArea`. r?markh!

This commit wires up StorageSyncArea::teardown() to call the new
webext_storage::Store::close() method.

It also changes teardown to drop the LazyStore, and thus close its
database connection, on the main thread if dispatching to the task
queue fails. Dispatch should only fail at shutdown, and putting the
owned reference back only adds indirection, since the StorageSyncArea
will still drop its LazyStore later in shutdown.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

One thing I've been thinking about over the last couple of days is how to handle errors from close/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".

Worth noting that this behavior is not set in stone. There's an open rusqlite bug about avoiding panics for essentially this reason: https://github.com/rusqlite/rusqlite/issues/688. The drop panic is one I was thinking about when I filed that issue. In all likelihood turning it to a warning is probably acceptable.

That said I don't have any objections here, the rest of your rationale is good.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfdd3c486d42
Support tearing down a Rust `StorageSyncArea`. r=markh

Comment 4

[Lina Butler [:lina]]

Thanks, Thom, that was exactly the ticket I was looking for! 💜 Replacing the panics would be great to do eventually, but definitely not urgent, I know there's a pile of fixes you'd like to get in there first.

I also thought about wrapping the Rusqlite connection in a ManuallyDrop, and having the wrapper's destructor leak it if close() failed, but it felt like such an edge case that it didn't seem to be worth the acrobatics.

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/cfdd3c486d42