Closed Bug 662986 Opened 13 years ago Closed 3 years ago

nsContentPrefService.js should asyncClose() its DB connection

Categories

(Toolkit :: Preferences, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla6

People

(Reporter: n.nethercote, Unassigned)

Details

Any DB connection on which asynchronous transactions were executed must be closed with asyncClose(), otherwise bad things can happen (eg. bug 654573).

It appears that nsContestPrefService.js fails to do this (see bug 654573 comment 61).  When the relevant connection(s) is GC'd, close() is (implicitly) called on it instead of asyncClose().
mak, asuth:  will async connections that aren't closed with asyncClose() cause memory leaks?  sqlite3_close() is never called in that case on such a connection.

If so, it would be good to get an idea of how bad this leak is.  What browser action causes content-prefs.sqlite connections to be opened and closed?  I expect that doing that action over and over will cause the explicit/storage/sqlite/content-prefs.sqlite/* numbers in about:memory to creep up and up...
Also, in bug 654573 comment 59, asuth said:  "Connections manually maintaining transactions can hang around with active locks which could starve out other connections or cause extreme growth of the write-ahead-log."   So I guess that's more potential bad behaviour here.
(In reply to comment #1)
> 
> If so, it would be good to get an idea of how bad this leak is.

Bug 662989 comment 1 suggests that an unclosed SQLite connection leaks around 180KB of memory.

This looks like the place where the missing asyncClose() call should be:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.js#181 

It looks like this only occurs at xpcom shutdown, so it's not a big deal.  Still seems worth fixing, though.
(In reply to comment #3)
> It looks like this only occurs at xpcom shutdown, so it's not a big deal. 
> Still seems worth fixing, though.

Apart that it's wrong to do shutdown at xpcom-shutdown if the service is using profile files. This shutdown stuff should be moved to profile-before-change.
Regarding WAL, only Places and cookies use it right now.

10 years with no action, closing.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX

This is fixed anyway since now the code uses Sqlite.jsm

You need to log in before you can comment on or make changes to this bug.