Bug 547031 - Improve async error handling in cookies. Part 3: Get the sync database connection early so it can't fail. r=sdwilsh, a=betaN+
authorDan Witte <dwitte@mozilla.com>
Fri, 12 Nov 2010 09:32:35 -0800
changeset 57407 09bb5890500d91c69b3474b08660cbe915e3b12d
parent 57406 adcc4d2f4070d04c182c1c19b171bfe6bb1260f3
child 57408 3e8a89a8be5651029c7ff5175654690cc137022e
push id16911
push userdwitte@mozilla.com
push dateFri, 12 Nov 2010 17:33:05 +0000
treeherdermozilla-central@0525032a59a3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssdwilsh, betaN
bugs547031
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 547031 - Improve async error handling in cookies. Part 3: Get the sync database connection early so it can't fail. r=sdwilsh, a=betaN+
extensions/cookie/test/unit/head_cookies.js
extensions/cookie/test/unit/test_cookies_read.js
extensions/cookie/test/unit/test_schema_2_migration.js
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
--- a/extensions/cookie/test/unit/head_cookies.js
+++ b/extensions/cookie/test/unit/head_cookies.js
@@ -177,29 +177,24 @@ function Cookie(name,
 }
 
 // Object representing a database connection and associated statements. The
 // implementation varies depending on schema version.
 function CookieDatabaseConnection(profile, schema)
 {
   // Manually generate a cookies.sqlite file with appropriate rows, columns,
   // and schema version. If it already exists, just set up our statements.
-  let file = profile.clone();
-  file.append("cookies.sqlite");
+  let file = do_get_cookie_file(profile);
   let exists = file.exists();
 
   this.db = Services.storage.openDatabase(file);
   this.schema = schema;
   if (!exists)
     this.db.schemaVersion = schema;
 
-  // Open an exclusive connection, so we error out if the database is open
-  // by another reader.
-  this.db.executeSimpleSQL("PRAGMA locking_mode = EXCLUSIVE");
-
   switch (schema) {
   case 1:
     {
       if (!exists) {
         this.db.executeSimpleSQL(
           "CREATE TABLE moz_cookies (       \
              id INTEGER PRIMARY KEY,        \
              name TEXT,                     \
@@ -533,39 +528,41 @@ CookieDatabaseConnection.prototype =
 
     this.stmtInsert = null;
     this.stmtDelete = null;
     this.stmtUpdate = null;
     this.db = null;
   }
 }
 
-// Count the cookies from 'host' in a database. If 'host' is null, count all
-// cookies.
-function do_count_cookies_in_db(profile, host)
+function do_get_cookie_file(profile)
 {
   let file = profile.clone();
   file.append("cookies.sqlite");
-  let connection = Services.storage.openDatabase(file);
+  return file;
+}
 
+// Count the cookies from 'host' in a database. If 'host' is null, count all
+// cookies.
+function do_count_cookies_in_db(connection, host)
+{
   let select = null;
   if (host) {
     select = connection.createStatement(
       "SELECT COUNT(1) FROM moz_cookies WHERE host = :host");
     select.bindByName("host", host);
   } else {
     select = connection.createStatement(
       "SELECT COUNT(1) FROM moz_cookies");
   }
 
   select.executeStep();
   let result = select.getInt32(0);
   select.reset();
   select.finalize();
-  connection.close();
   return result;
 }
 
 // Execute 'stmt', ensuring that we reset it if it throws.
 function do_execute_stmt(stmt)
 {
   try {
     stmt.executeStep();
--- a/extensions/cookie/test/unit/test_cookies_read.js
+++ b/extensions/cookie/test/unit/test_cookies_read.js
@@ -17,26 +17,34 @@ function finish_test() {
     do_test_finished();
   });
 }
 
 function do_run_test() {
   // Set up a profile.
   let profile = do_get_profile();
 
+  // Start the cookieservice, to force creation of a database.
+  Services.cookies;
+
+  // Open a database connection now, after synchronous initialization has
+  // completed. We may not be able to open one later once asynchronous writing
+  // begins.
+  do_check_true(do_get_cookie_file(profile).exists());
+  let db = new CookieDatabaseConnection(profile, 4);
+
   for (let i = 0; i < 3000; ++i) {
     let uri = NetUtil.newURI("http://" + i + ".com/");
     Services.cookies.setCookieString(uri, null, "oh=hai; max-age=1000", null);
   }
 
   do_check_eq(do_count_cookies(), 3000);
 
   // Wait until all 3000 cookies have been written out to the database.
-  let db = new CookieDatabaseConnection(profile, 4);
-  while (do_count_cookies_in_db(profile) < 3000) {
+  while (do_count_cookies_in_db(db.db) < 3000) {
     do_execute_soon(function() {
       do_run_generator(test_generator);
     });
     yield;
   }
 
   // Check the WAL file size. We set it to 16 pages of 32k, which means it
   // should be around 500k.
--- a/extensions/cookie/test/unit/test_schema_2_migration.js
+++ b/extensions/cookie/test/unit/test_schema_2_migration.js
@@ -117,17 +117,17 @@ function do_run_test() {
   // cookie. This should succeed since we have a REPLACE clause for conflict on
   // the unique index.
   let cookie = new Cookie("oh", "hai", "baz.com", "/",
                           futureExpiry, now, now + 100, false, false, false);
 
   schema2db.insertCookie(cookie);
 
   // Check that there is, indeed, a singular cookie for baz.com.
-  do_check_eq(do_count_cookies_in_db(profile, "baz.com"), 1);
+  do_check_eq(do_count_cookies_in_db(schema2db.db, "baz.com"), 1);
 
   // Close it.
   schema2db.close();
   schema2db = null;
 
   // Load the database, forcing a purge of the newly-added cookies. (Their
   // baseDomain column will be NULL.)
   do_load_profile();
@@ -136,13 +136,15 @@ function do_run_test() {
   do_check_eq(Services.cookiemgr.countCookiesFromHost("foo.com"), 20);
   do_check_eq(Services.cookiemgr.countCookiesFromHost("cat.com"), 0);
   do_check_eq(Services.cookiemgr.countCookiesFromHost("baz.com"), 0);
 
   do_close_profile(test_generator);
   yield;
 
   // Open the database and prove that they were deleted.
-  do_check_eq(do_count_cookies_in_db(profile, "cat.com"), 0);
+  schema2db = new CookieDatabaseConnection(profile, 2);
+  do_check_eq(do_count_cookies_in_db(schema2db.db, "cat.com"), 0);
+  schema2db.close();
 
   finish_test();
 }
 
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -1121,17 +1121,16 @@ nsCookieService::CloseDBStates()
   if (!mDefaultDBState)
     return;
 
   if (mDefaultDBState->dbConn) {
     // Cancel any pending read. No further results will be received by our
     // read listener.
     if (mDefaultDBState->pendingRead) {
       CancelAsyncRead(PR_TRUE);
-      mDefaultDBState->syncConn = nsnull;
     }
 
     // Asynchronously close the connection. We will null it below.
     mDefaultDBState->dbConn->AsyncClose(mDefaultDBState->closeListener);
   }
 
   CloseDefaultDBConnection();
 
@@ -1144,20 +1143,21 @@ nsCookieService::CloseDBStates()
 void
 nsCookieService::CloseDefaultDBConnection()
 {
   // Destroy our statements before we close the db.
   mDefaultDBState->stmtInsert = NULL;
   mDefaultDBState->stmtDelete = NULL;
   mDefaultDBState->stmtUpdate = NULL;
 
-  // Null out the db. If it has not been used for any asynchronous operations
-  // yet, this will synchronously close it; otherwise, it's expected that the
-  // caller has performed an AsyncClose prior.
+  // Null out the database connections. If 'dbConn' has not been used for any
+  // asynchronous operations yet, this will synchronously close it; otherwise,
+  // it's expected that the caller has performed an AsyncClose prior.
   mDefaultDBState->dbConn = NULL;
+  mDefaultDBState->syncConn = NULL;
 
   // Manually null out our listeners. This is necessary because they hold a
   // strong ref to the DBState itself.
   mDefaultDBState->readListener = NULL;
   mDefaultDBState->insertListener = NULL;
   mDefaultDBState->updateListener = NULL;
   mDefaultDBState->removeListener = NULL;
   mDefaultDBState->closeListener = NULL;
@@ -1447,17 +1447,16 @@ nsCookieService::RemoveAll()
   // clear the cookie file
   if (mDBState->dbConn) {
     NS_ASSERTION(mDBState == mDefaultDBState, "not in default DB state");
 
     // Cancel any pending read. No further results will be received by our
     // read listener.
     if (mDefaultDBState->pendingRead) {
       CancelAsyncRead(PR_TRUE);
-      mDefaultDBState->syncConn = nsnull;
     }
 
     // XXX Ignore corruption for now. See bug 547031.
     nsCOMPtr<mozIStorageStatement> stmt;
     nsresult rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
       "DELETE FROM moz_cookies"), getter_AddRefs(stmt));
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1612,16 +1611,23 @@ nsCookieService::Read()
   // understand the 'baseDomain' column, where the database schema version
   // is from one that does. (This would occur when downgrading.)
   nsCOMPtr<mozIStorageStatement> stmtDeleteNull;
   rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
     "DELETE FROM moz_cookies WHERE baseDomain ISNULL"),
     getter_AddRefs(stmtDeleteNull));
   NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
+  // Start a new connection for sync reads, to reduce contention with the
+  // background thread. We need to do this before we kick off write statements,
+  // since they can lock the database and prevent connections from being opened.
+  rv = mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile,
+    getter_AddRefs(mDefaultDBState->syncConn));
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
+
   // Init our readSet hash and execute the statements. Note that, after this
   // point, we cannot fail without altering the cleanup code in InitDBStates()
   // to handle closing of the now-asynchronous connection.
   mDefaultDBState->readSet.Init();
 
   mDefaultDBState->readListener = new ReadCookieDBListener(mDefaultDBState);
   rv = stmtRead->ExecuteAsync(mDefaultDBState->readListener,
     getter_AddRefs(mDefaultDBState->pendingRead));
@@ -1728,35 +1734,16 @@ nsCookieService::CancelAsyncRead(PRBool 
   mDefaultDBState->hostArray.Clear();
 
   // Only clear the 'readSet' table if we no longer need to know what set of
   // data is already accounted for.
   if (aPurgeReadSet)
     mDefaultDBState->readSet.Clear();
 }
 
-mozIStorageConnection*
-nsCookieService::GetSyncDBConn()
-{
-  NS_ASSERTION(!mDefaultDBState->syncConn, "already have sync db connection");
-  NS_ASSERTION(mDefaultDBState->cookieFile, "no cookie file");
-
-  // Start a new connection for sync reads to reduce contention with the
-  // background thread.
-  mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile,
-    getter_AddRefs(mDefaultDBState->syncConn));
-  if (!mDefaultDBState->syncConn) {
-    NS_ERROR("can't open sync db connection");
-    COOKIE_LOGSTRING(PR_LOG_DEBUG,
-      ("GetSyncDBConn(): can't open sync db connection"));
-  }
-
-  return mDefaultDBState->syncConn;
-}
-
 void
 nsCookieService::EnsureReadDomain(const nsCString &aBaseDomain)
 {
   NS_ASSERTION(!mDBState->dbConn || mDBState == mDefaultDBState,
     "not in default db state");
 
   // Fast path 1: nothing to read, or we've already finished reading.
   if (NS_LIKELY(!mDBState->dbConn || !mDefaultDBState->pendingRead))
@@ -1764,19 +1751,16 @@ nsCookieService::EnsureReadDomain(const 
 
   // Fast path 2: already read in this particular domain.
   if (NS_LIKELY(mDefaultDBState->readSet.GetEntry(aBaseDomain)))
     return;
 
   // Read in the data synchronously.
   nsresult rv;
   if (!mDefaultDBState->stmtReadDomain) {
-    if (!GetSyncDBConn())
-      return;
-
     // Cache the statement, since it's likely to be used again.
     rv = mDefaultDBState->syncConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT "
         "name, "
         "value, "
         "host, "
         "path, "
         "expiry, "
@@ -1832,19 +1816,16 @@ nsCookieService::EnsureReadComplete()
 
   // Fast path 1: nothing to read, or we've already finished reading.
   if (NS_LIKELY(!mDBState->dbConn || !mDefaultDBState->pendingRead))
     return;
 
   // Cancel the pending read, so we don't get any more results.
   CancelAsyncRead(PR_FALSE);
 
-  if (!mDefaultDBState->syncConn && !GetSyncDBConn())
-    return;
-
   // Read in the data synchronously.
   nsCOMPtr<mozIStorageStatement> stmt;
   nsresult rv = mDefaultDBState->syncConn->CreateStatement(NS_LITERAL_CSTRING(
     "SELECT "
       "name, "
       "value, "
       "host, "
       "path, "
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -240,17 +240,16 @@ class nsCookieService : public nsICookie
     OpenDBResult                  TryInitDB(bool aDeleteExistingDB);
     nsresult                      CreateTable();
     void                          CloseDBStates();
     void                          CloseDefaultDBConnection();
     OpenDBResult                  Read();
     template<class T> nsCookie*   GetCookieFromRow(T &aRow);
     void                          AsyncReadComplete();
     void                          CancelAsyncRead(PRBool aPurgeReadSet);
-    mozIStorageConnection*        GetSyncDBConn();
     void                          EnsureReadDomain(const nsCString &aBaseDomain);
     void                          EnsureReadComplete();
     nsresult                      NormalizeHost(nsCString &aHost);
     nsresult                      GetBaseDomain(nsIURI *aHostURI, nsCString &aBaseDomain, PRBool &aRequireHostMatch);
     nsresult                      GetBaseDomainFromHost(const nsACString &aHost, nsCString &aBaseDomain);
     nsresult                      GetCookieStringCommon(nsIURI *aHostURI, nsIChannel *aChannel, bool aHttpBound, char** aCookie);
     void                          GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, PRBool aHttpBound, nsCString &aCookie);
     nsresult                      SetCookieStringCommon(nsIURI *aHostURI, const char *aCookieHeader, const char *aServerTime, nsIChannel *aChannel, bool aFromHttp);