Bug 662989 - Avoid crashing if a DB connection isn't asyncClose()d when it should be. r=mak.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 04 Aug 2011 16:15:03 -0700
changeset 73881 e820bee09d57ba78d2033bcbdc182518879968f4
parent 73880 706c47369f83952e91d010a0508e7bae6f27ad29
child 73882 fafce4553fd584068f313e2d3dcd2d57a93b946c
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
reviewersmak
bugs662989
milestone8.0a1
Bug 662989 - Avoid crashing if a DB connection isn't asyncClose()d when it should be. r=mak.
storage/src/mozStorageConnection.cpp
storage/src/mozStorageConnection.h
toolkit/components/aboutmemory/tests/chrome/Makefile.in
toolkit/components/aboutmemory/tests/chrome/test_asyncClose_leak.xul
--- a/storage/src/mozStorageConnection.cpp
+++ b/storage/src/mozStorageConnection.cpp
@@ -335,34 +335,39 @@ public:
   NS_DECL_ISUPPORTS
 
   enum ReporterType {
     Cache_Used,
     Schema_Used,
     Stmt_Used
   };
 
-  StorageMemoryReporter(Connection &aDBConn,
+  StorageMemoryReporter(sqlite3 *aDBConn,
+                        const nsCString &aFileName,
                         ReporterType aType)
   : mDBConn(aDBConn)
+  , mFileName(aFileName)
   , mType(aType)
+  , mHasLeaked(false)
   {
   }
 
-
   NS_IMETHOD GetProcess(nsACString &process)
   {
     process.Truncate();
     return NS_OK;
   }
 
   NS_IMETHOD GetPath(nsACString &path)
   {
     path.AssignLiteral("explicit/storage/sqlite/");
-    path.Append(mDBConn.getFilename());
+    path.Append(mFileName);
+    if (mHasLeaked) {
+      path.AppendLiteral("-LEAKED");
+    }
 
     if (mType == Cache_Used) {
       path.AppendLiteral("/cache-used");
     }
     else if (mType == Schema_Used) {
       path.AppendLiteral("/schema-used");
     }
     else if (mType == Stmt_Used) {
@@ -400,31 +405,42 @@ public:
     int rc = ::sqlite3_db_status(mDBConn, type, &cur, &max, 0);
     *amount = cur;
     return convertResultCode(rc);
   }
 
   NS_IMETHOD GetDescription(nsACString &desc)
   {
     if (mType == Cache_Used) {
-      desc.AssignLiteral("Memory (approximate) used by all pager caches.");
+      desc.AssignLiteral("Memory (approximate) used by all pager caches used "
+                         "by connections to this database.");
     }
     else if (mType == Schema_Used) {
       desc.AssignLiteral("Memory (approximate) used to store the schema "
-                          "for all databases associated with the connection");
+                         "for all databases associated with connections to "
+                         "this database.");
     }
     else if (mType == Stmt_Used) {
-      desc.AssignLiteral("Memory (approximate) used by all prepared statements");
+      desc.AssignLiteral("Memory (approximate) used by all prepared statements "
+                         "used by connections to this database.");
     }
     return NS_OK;
   }
 
-  Connection &mDBConn;
+  // We call this when we know we've leaked a connection.
+  void markAsLeaked()
+  {
+    mHasLeaked = true;
+  }
+
+private:
+  sqlite3 *mDBConn;
   nsCString mFileName;
   ReporterType mType;
+  bool mHasLeaked;
 };
 NS_IMPL_THREADSAFE_ISUPPORTS1(
   StorageMemoryReporter
 , nsIMemoryReporter
 )
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Connection
@@ -442,16 +458,27 @@ Connection::Connection(Service *aService
 , mStorageService(aService)
 {
   mFunctions.Init();
 }
 
 Connection::~Connection()
 {
   (void)Close();
+
+  // The memory reporters should have been already unregistered if the APIs
+  // have been used properly.  But if an async connection hasn't been closed
+  // with asyncClose(), the connection is about to leak and it's too late to do
+  // anything about it.  So we mark the memory reporters accordingly so that
+  // the leak will be obvious in about:memory.
+  for (PRUint32 i = 0; i < mMemoryReporters.Length(); i++) {
+    if (mMemoryReporters[i]) {
+      mMemoryReporters[i]->markAsLeaked();
+    }
+  }
 }
 
 NS_IMPL_THREADSAFE_ISUPPORTS2(
   Connection,
   mozIStorageConnection,
   nsIInterfaceRequestor
 )
 
@@ -576,28 +603,32 @@ Connection::initialize(nsIFile *aDatabas
       break;
     case 1:
     default:
       (void)ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "PRAGMA synchronous = NORMAL;"));
       break;
   }
 
-  nsRefPtr<nsIMemoryReporter> reporter;
+  nsRefPtr<StorageMemoryReporter> reporter;
+  nsCString filename = this->getFilename();
 
   reporter =
-    new StorageMemoryReporter(*this, StorageMemoryReporter::Cache_Used);
+    new StorageMemoryReporter(this->mDBConn, filename,
+                              StorageMemoryReporter::Cache_Used);
   mMemoryReporters.AppendElement(reporter);
 
   reporter =
-    new StorageMemoryReporter(*this, StorageMemoryReporter::Schema_Used);
+    new StorageMemoryReporter(this->mDBConn, filename,
+                              StorageMemoryReporter::Schema_Used);
   mMemoryReporters.AppendElement(reporter);
 
   reporter =
-    new StorageMemoryReporter(*this, StorageMemoryReporter::Stmt_Used);
+    new StorageMemoryReporter(this->mDBConn, filename,
+                              StorageMemoryReporter::Stmt_Used);
   mMemoryReporters.AppendElement(reporter);
 
   for (PRUint32 i = 0; i < mMemoryReporters.Length(); i++) {
     (void)::NS_RegisterMemoryReporter(mMemoryReporters[i]);
   }
 
   return NS_OK;
 }
@@ -732,16 +763,17 @@ Connection::internalClose()
                               ::sqlite3_sql(stmt));
     NS_WARNING(msg);
     ::PR_smprintf_free(msg);
   }
 #endif
 
   for (PRUint32 i = 0; i < mMemoryReporters.Length(); i++) {
     (void)::NS_UnregisterMemoryReporter(mMemoryReporters[i]);
+    mMemoryReporters[i] = nsnull;
   }
 
   int srv = ::sqlite3_close(mDBConn);
   NS_ASSERTION(srv == SQLITE_OK,
                "sqlite3_close failed. There are probably outstanding statements that are listed above!");
 
   mDBConn = NULL;
   return convertResultCode(srv);
@@ -778,18 +810,21 @@ Connection::GetInterface(const nsIID &aI
 
 NS_IMETHODIMP
 Connection::Close()
 {
   if (!mDBConn)
     return NS_ERROR_NOT_INITIALIZED;
 
   { // Make sure we have not executed any asynchronous statements.
+    // If this fails, the connection will be left open!  See ~Connection() for
+    // more details.
     MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
-    NS_ENSURE_FALSE(mAsyncExecutionThread, NS_ERROR_UNEXPECTED);
+    bool asyncCloseWasCalled = !mAsyncExecutionThread;
+    NS_ENSURE_TRUE(asyncCloseWasCalled, NS_ERROR_UNEXPECTED);
   }
 
   nsresult rv = setClosedState();
   NS_ENSURE_SUCCESS(rv, rv);
 
   return internalClose();
 }
 
--- a/storage/src/mozStorageConnection.h
+++ b/storage/src/mozStorageConnection.h
@@ -60,16 +60,18 @@ struct PRLock;
 class nsIFile;
 class nsIEventTarget;
 class nsIThread;
 class nsIMemoryReporter;
 
 namespace mozilla {
 namespace storage {
 
+class StorageMemoryReporter;
+
 class Connection : public mozIStorageConnection
                  , public nsIInterfaceRequestor
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_MOZISTORAGECONNECTION
   NS_DECL_NSIINTERFACEREQUESTOR
 
@@ -197,17 +199,17 @@ private:
   // Generic progress handler
   // Dispatch call to registered progress handler,
   // if there is one. Do nothing in other cases.
   int progressHandler();
 
   sqlite3 *mDBConn;
   nsCOMPtr<nsIFile> mDatabaseFile;
 
-  nsTArray<nsCOMPtr<nsIMemoryReporter> > mMemoryReporters;
+  nsTArray<nsRefPtr<StorageMemoryReporter> > mMemoryReporters;
 
   /**
    * Lazily created thread for asynchronous statement execution.  Consumers
    * should use getAsyncExecutionTarget rather than directly accessing this
    * field.
    */
   nsCOMPtr<nsIThread> mAsyncExecutionThread;
   /**
--- a/toolkit/components/aboutmemory/tests/chrome/Makefile.in
+++ b/toolkit/components/aboutmemory/tests/chrome/Makefile.in
@@ -41,13 +41,14 @@ srcdir		= @srcdir@
 VPATH		= @srcdir@
 relativesrcdir  = toolkit/components/aboutmemory/tests/chrome
 
 include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
 _CHROME_FILES	= \
 		test_aboutmemory.xul \
+		test_asyncClose_leak.xul \
 		$(NULL)
 
 libs:: $(_CHROME_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/aboutmemory/tests/chrome/test_asyncClose_leak.xul
@@ -0,0 +1,111 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
+<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
+<window title="about:memory"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <script type="application/javascript" src="chrome://mochikit/content/MochiKit/packed.js"/>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
+
+  <!-- test results are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml"></body>
+
+  <!-- test code goes here -->
+  <script type="application/javascript">
+  <![CDATA[
+
+  // Nb: this test is all JS and so should be done with an xpcshell test,
+  // but bug 671753 is preventing the memory-reporter-manager from being
+  // accessed from xpcshell.
+
+  const Cc = Components.classes;
+  const Ci = Components.interfaces;
+  const Cu = Components.utils;
+
+  // Make a fake DB file.
+  var file = Cc["@mozilla.org/file/directory_service;1"].
+             getService(Ci.nsIProperties).
+             get("ProfD", Ci.nsIFile);
+  file.append("asyncClose_leak-fake-DB-tmp.sqlite");
+
+  var storage = Cc["@mozilla.org/storage/service;1"].
+                getService(Ci.mozIStorageService);
+  var db = storage.openDatabase(file);
+
+  // A statement, the exact form doesn't matter, it just has to be
+  // asynchronous.
+  var stmt = db.createAsyncStatement("SELECT * FROM sqlite_master");
+  try {
+    stmt.executeAsync({
+      handleResult: function(aResults) {
+      },
+      handleError: function(aError) {
+        Cu.reportError("db error: " + aError);
+      },
+      handleCompletion: function(aReason) {
+      }
+    });
+  } catch (e) {
+  } finally {
+    stmt.finalize();
+  }
+  // Correct code would call db.asyncClose() here.  But the point of this
+  // test is to see what happens when we forget this call.
+  //db.asyncClose();
+
+  // We need db to be collected, otherwise this test will fail.  Schedule
+  // three GC+CC runs on the main thread followed by the final check.
+  stmt = null;
+  db = null;
+  function forceCollectionsThenCheck()
+  {
+    function runSoon(f)
+    {
+      var tm = Cc["@mozilla.org/thread-manager;1"]
+                .getService(Ci.nsIThreadManager);
+
+      tm.mainThread.dispatch({ run: f }, Ci.nsIThread.DISPATCH_NORMAL);
+    }
+
+    function doGCandCC()
+    {
+      window.QueryInterface(Ci.nsIInterfaceRequestor)
+            .getInterface(Ci.nsIDOMWindowUtils)
+            .garbageCollect();
+
+      if (++j < 3)
+        runSoon(doGCandCC);
+      else
+        runSoon(doCheck);
+    }
+
+    var j = 0;
+    doGCandCC();
+  }
+
+  // Because we forgot to asyncClose the db, we end up with three "LEAKED"
+  // reporters.
+  function doCheck() {
+    var numLeaked = 0;
+    var mgr = Cc["@mozilla.org/memory-reporter-manager;1"].
+              getService(Ci.nsIMemoryReporterManager);
+    var e = mgr.enumerateReporters(); 
+    while (e.hasMoreElements()) {
+      var r = e.getNext().QueryInterface(Ci.nsIMemoryReporter);
+      if (r.path.search(/sqlite-LEAKED/) != -1) {
+        numLeaked++;
+        // Unregister the leaked reporter, so that if the test is run more
+        // than once, we don't end up with too many of them.
+        mgr.unregisterReporter(r);
+      }
+    }
+    is(numLeaked, 3, "Looking for sqlite-LEAKED reporters");
+    SimpleTest.finish();
+  }
+
+  forceCollectionsThenCheck();
+
+  SimpleTest.waitForExplicitFinish();
+  ]]>
+  </script>
+</window>