TL;DR: if you’re familiar with any Mozilla (C++ or JS) code that opens an async database connection, please go and check that it calls asyncClose() to close the connection; not doing so can lead to crashes and/or memory leaks.
Specifically, in Firefox 6, when such a connection is destroyed (because it’s explicitly deallocated or its refcount drops to zero in C++, or it’s garbage collected in JS) it’ll currently cause sporadic crashes in about:memory or telemetry code or Test Pilot code. This is because memory reporters end up pointing to connections that have been freed, and so when they are used they end up (innocently) accessing memory they shouldn’t.
I’ve opened bug 662989 to avoid the crashes, but even once it’s implemented, I think that if you fail to call asyncClose() it will still cause a memory leak, because sqlite3_close() is never called on the connection and so SQLite won’t free up memory used by the connection. Also, connections that needlessly remain open can have active locks that can starve out other connections or cause extreme growth of the write-ahead-log.
As I write this, thanks to Marco Bonardo, I’m aware of three places in JS code that fail to call asyncClose() when they should:
- browser/components/preferences/aboutPermissions.js. Bug 654573 covers this, I’m about to land a patch to fix it.
- toolkit/components/contentprefs/nsContentPrefService.js. Bug 662986 covers this.
- browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/experiment_data_store.js. Bug 662985 covers this. Note that Test Pilot seems to fail to use asyncClose() when it should, and it also uses memory reporters. So it’s unclear which of these two things is responsible for the Test Pilot crashes of this sort seen so far; in other words, Test Pilot may be a culprit or an innocent bystander, or both.
These were found via a crude MXR search. I haven’t looked for C++ code that makes this mistake.
If you only do synchronous transactions over your database connection, the connection will be cleaned up properly, even if you don’t call close(), when it is deallocated or garbage collected. However, it’s better to close the connection as soon as you can so that the resources (memory, locks) are freed immediately.
See this MDC page for more details about database connections and the relevant APIs. It appears that these APIs are somewhat error-prone. As it happens, an NS_ENSURE_TRUE macro will fail when an async connection is destroyed without having been asyncClose’d first, and this causes a warning message to be printed to stderr. Unfortunately, stderr is spammed with all sorts of warnings (something that was discussed in the comments of a previous post of mine), and so this warning gets lost among the noise. Addressing this problem is something I’ll write about shortly.
Thanks to Andrew Sutherland and Marco Bonardo for helping me greatly with the bugs mentioned above. Any errors in this post are mine!
UPDATE: I did some investigation and found that about:permissions leaked about 180KB of memory on my machine every time it failed to asyncClose a DB connection. This showed up under the explicit/storage/sqlite/other reporter in about:memory.
10 replies on “Asynchronous database connections must be closed with asyncClose()”
This is a strange use of NS_ENSURE_FALSE indeed. It’s at the end of the function, so there’s no early return.
It sounds like it would be sensible to replace
NS_ENSURE_FALSE(mAsyncExecutionThread, NS_ERROR_UNEXPECTED);
with
NS_ASSERTION(!mAsyncExecutionThread, “Someone forgot to call asyncClose; leaking database connection”);
Jesse: the NS_ENSURE_FALSE is not at the end of the function.
NS_IMETHODIMP
Connection::Close()
{
if (!mDBConn)
return NS_ERROR_NOT_INITIALIZED;
{ // Make sure we have not executed any asynchronous statements.
MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
NS_ENSURE_FALSE(mAsyncExecutionThread, NS_ERROR_UNEXPECTED);
}
nsresult rv = setClosedState();
NS_ENSURE_SUCCESS(rv, rv);
return internalClose();
}
Oh. In that case, I guess we want an explicit NS_ERROR and and explicit return.
if (!mAsyncExecutionThread) {
NS_ERROR(“Someone forgot to call asyncClose; leaking database connection”);
return NS_ERROR_UNEXPECTED;
}
Jesse: AFAICT, NS_ERROR is for “critical” errors and NS_WARNING is for “non-critical” errors, but they behave the same (send a message to stderr) so the distinction appears pointless.
NS_ERROR is for bugs and NS_WARNING is for non-bugs.
I’ve never heard this distinction between “critical errors” and “non-critical errors”.
In some edge-case situations, they both send a message to stderr and that’s the end of it. In the situations that matter, regression testing and fuzzing, NS_ERROR results in a bug report and NS_WARNING doesn’t.
Jesse: I got the critical/non-critical distinction here:
https://developer.mozilla.org/en/NS_ERROR
https://developer.mozilla.org/en/NS_WARNING
My idea would be to make close/asyncClose the same/synonymous, except for the optional callback.
NS_IMETHODIMP
Connection::Close()
{
return AsyncClose(nsnull);
}
+ in AsyncClose
if (!asyncThread) {
if (callback)
// execute callback now
return internalClose();
}
else {
// completeEvent dispatch stuff
}
Nils: I don’t think that’ll work, see https://bugzilla.mozilla.org/show_bug.cgi?id=654573#c56 for why.
The more I look at this, the more confused I am.
In my add-ons, I open the database connection and don’t close it.
I was told there’s really no reason to close the database connection since it will be closed when the browser closes.
So when would I call asyncClose?
And why would it leak if I don’t close it since the browser is going away anyway?
Mike: if your connection’s lifetime matches the browser’s lifetime, you’re right, there’s not much to be gained from calling asyncClose.
It’s a much bigger issue if your connection’s lifetime is shorter than that. For example, the about:permissions page had a connection whose lifetime ended when the page was closed; the failure to asyncClose when the about:permissions was unloaded led to the crashes and leaks.