Bug 870460 - Part 1: Let cookie db startup-read off-main-thread. r=nwgh, r=jdm, data-r=francois

extensions/cookie/test/unit/test_cookies_async_failure.js

@@ -232,20 +232,9 @@ function* run_test_2(generator)
   // succeeded.
   do_check_false(do_get_backup_file(profile).exists());
 
-  // Synchronously read in the first cookie. This will cause it to go into the
-  // cookie table, whereupon it will be written out during database rebuild.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-
-  // Wait for the asynchronous read to choke, at which point the backup file
-  // will be created and the database rebuilt.
-  new _observer(sub_generator, "cookie-db-rebuilding");
-  yield;
-  do_execute_soon(function() { do_run_generator(sub_generator); });
-  yield;
-
-  // At this point, the cookies should still be in memory.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-  do_check_eq(do_count_cookies(), 1);
+  // Recreate a new database since it was corrupted
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 0);
+  do_check_eq(do_count_cookies(), 0);
 
   // Close the profile.
   do_close_profile(sub_generator);
@@ -255,13 +244,11 @@ function* run_test_2(generator)
   do_check_true(do_get_backup_file(profile).exists());
   do_check_eq(do_get_backup_file(profile).fileSize, size);
   let db = Services.storage.openDatabase(do_get_cookie_file(profile));
-  do_check_eq(do_count_cookies_in_db(db, "0.com"), 1);
   db.close();
 
-  // Load the profile, and check that it contains the new cookie.
   do_load_profile();
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-  do_check_eq(do_count_cookies(), 1);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 0);
+  do_check_eq(do_count_cookies(), 0);
 
   // Close the profile.
   do_close_profile(sub_generator);
@@ -310,25 +297,16 @@ function* run_test_3(generator)
   // succeeded.
   do_check_false(do_get_backup_file(profile).exists());
 
-  // Synchronously read in the cookies for our two domains. The first should
-  // succeed, but the second should fail midway through, resulting in none of
-  // those cookies being present.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("hither.com"), 10);
+  // Recreate a new database since it was corrupted
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("hither.com"), 0);
   do_check_eq(Services.cookiemgr.countCookiesFromHost("haithur.com"), 0);
 
-  // Wait for the backup file to be created and the database rebuilt.
-  do_check_false(do_get_backup_file(profile).exists());
-  new _observer(sub_generator, "cookie-db-rebuilding");
-  yield;
-  do_execute_soon(function() { do_run_generator(sub_generator); });
-  yield;
-
   // Close the profile.
   do_close_profile(sub_generator);
   yield;
   let db = Services.storage.openDatabase(do_get_cookie_file(profile));
-  do_check_eq(do_count_cookies_in_db(db, "hither.com"), 10);
-  do_check_eq(do_count_cookies_in_db(db), 10);
+  do_check_eq(do_count_cookies_in_db(db, "hither.com"), 0);
+  do_check_eq(do_count_cookies_in_db(db), 0);
   db.close();
 
   // Check that the original database was renamed.
@@ -346,13 +324,6 @@ function* run_test_3(generator)
   // Synchronously read in everything.
   do_check_eq(do_count_cookies(), 0);
 
-  // Wait for the backup file to be created and the database rebuilt.
-  do_check_false(do_get_backup_file(profile).exists());
-  new _observer(sub_generator, "cookie-db-rebuilding");
-  yield;
-  do_execute_soon(function() { do_run_generator(sub_generator); });
-  yield;
-
   // Close the profile.
   do_close_profile(sub_generator);
   yield;
@@ -397,26 +368,17 @@ function* run_test_4(generator)
   // succeeded.
   do_check_false(do_get_backup_file(profile).exists());
 
-  // Synchronously read in the first cookie. This will cause it to go into the
-  // cookie table, whereupon it will be written out during database rebuild.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
+  // Recreate a new database since it was corrupted
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 0);
 
   // Queue up an INSERT for the same base domain. This should also go into
   // memory and be written out during database rebuild.
   let uri = NetUtil.newURI("http://0.com/");
   Services.cookies.setCookieString(uri, null, "oh2=hai; max-age=1000", null);
 
-  // Wait for the asynchronous read to choke and the insert to fail shortly
-  // thereafter, at which point the backup file will be created and the database
-  // rebuilt.
-  new _observer(sub_generator, "cookie-db-rebuilding");
-  yield;
-  do_execute_soon(function() { do_run_generator(sub_generator); });
-  yield;
-
   // At this point, the cookies should still be in memory.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 2);
-  do_check_eq(do_count_cookies(), 2);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
+  do_check_eq(do_count_cookies(), 1);
 
   // Close the profile.
   do_close_profile(sub_generator);
@@ -425,14 +387,11 @@ function* run_test_4(generator)
   // Check that the original database was renamed.
   do_check_true(do_get_backup_file(profile).exists());
   do_check_eq(do_get_backup_file(profile).fileSize, size);
-  let db = Services.storage.openDatabase(do_get_cookie_file(profile));
-  do_check_eq(do_count_cookies_in_db(db, "0.com"), 2);
-  db.close();
 
   // Load the profile, and check that it contains the new cookie.
   do_load_profile();
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 2);
-  do_check_eq(do_count_cookies(), 2);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
+  do_check_eq(do_count_cookies(), 1);
 
   // Close the profile.
   do_close_profile(sub_generator);
@@ -474,22 +433,10 @@ function* run_test_5(generator)
   // succeeded.
   do_check_false(do_get_backup_file(profile).exists());
 
-  // Synchronously read in the first two cookies. This will cause them to go
-  // into the cookie table, whereupon it will be written out during database
-  // rebuild.
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("bar.com"), 1);
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-
-  // Wait for the asynchronous read to choke, at which point the backup file
-  // will be created and a new connection opened.
-  new _observer(sub_generator, "cookie-db-rebuilding");
-  yield;
-
-  // At this point, the cookies should still be in memory. (Note that these
-  // calls are re-entrant into the cookie service, but it's OK!)
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("bar.com"), 1);
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-  do_check_eq(do_count_cookies(), 2);
+  // Recreate a new database since it was corrupted
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("bar.com"), 0);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 0);
+  do_check_eq(do_count_cookies(), 0);
   do_check_true(do_get_backup_file(profile).exists());
   do_check_eq(do_get_backup_file(profile).fileSize, size);
   do_check_false(do_get_rebuild_backup_file(profile).exists());
@@ -502,39 +449,23 @@ function* run_test_5(generator)
   do_check_eq(do_count_cookies_in_db(db.db), 1);
   db.close();
 
-  // Wait for the rebuild to bail and the database to be closed.
-  new _observer(sub_generator, "cookie-db-closed");
-  yield;
-
   // Check that the original backup and the database itself are gone.
-  do_check_true(do_get_rebuild_backup_file(profile).exists());
   do_check_true(do_get_backup_file(profile).exists());
   do_check_eq(do_get_backup_file(profile).fileSize, size);
-  do_check_false(do_get_cookie_file(profile).exists());
 
-  // Check that the rebuild backup has the original bar.com cookie, and possibly
-  // a 0.com cookie depending on whether it got written out first or second.
-  db = new CookieDatabaseConnection(do_get_rebuild_backup_file(profile), 4);
-  do_check_eq(do_count_cookies_in_db(db.db, "bar.com"), 1);
-  let count = do_count_cookies_in_db(db.db);
-  do_check_true(count == 1 ||
-    count == 2 && do_count_cookies_in_db(db.db, "0.com") == 1);
-  db.close();
-
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("bar.com"), 1);
-  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 1);
-  do_check_eq(do_count_cookies(), 2);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("bar.com"), 0);
+  do_check_eq(Services.cookiemgr.countCookiesFromHost("0.com"), 0);
+  do_check_eq(do_count_cookies(), 0);
 
   // Close the profile. We do not need to wait for completion, because the
   // database has already been closed.
   do_close_profile();
 
   // Clean up.
+  do_get_cookie_file(profile).remove(false);
   do_get_backup_file(profile).remove(false);
-  do_get_rebuild_backup_file(profile).remove(false);
   do_check_false(do_get_cookie_file(profile).exists());
   do_check_false(do_get_backup_file(profile).exists());
-  do_check_false(do_get_rebuild_backup_file(profile).exists());
   do_run_generator(generator);
 }
 

extensions/cookie/test/unit/test_cookies_read.js

@@ -27,7 +27,8 @@ function* do_run_test() {
   Services.prefs.setIntPref("network.cookie.cookieBehavior", 0);
 
   // Start the cookieservice, to force creation of a database.
-  Services.cookies;
+  // Get the sessionEnumerator to join the initialization in cookie thread
+  Services.cookiemgr.sessionEnumerator;
 
   // Open a database connection now, after synchronous initialization has
   // completed. We may not be able to open one later once asynchronous writing

extensions/cookie/test/unit/test_schema_2_migration.js

@@ -22,6 +22,17 @@ function* do_run_test() {
   // Set up a profile.
   let profile = do_get_profile();
 
+  // Start the cookieservice, to force creation of a database.
+  // Get the sessionEnumerator to join the initialization in cookie thread
+  Services.cookiemgr.sessionEnumerator;
+
+  // Close the profile.
+  do_close_profile(test_generator);
+  yield;
+
+  // Remove the cookie file in order to create another database file.
+  do_get_cookie_file(profile).remove(false);
+
   // Create a schema 2 database.
   let schema2db = new CookieDatabaseConnection(do_get_cookie_file(profile), 2);
 
@@ -79,6 +90,8 @@ function* do_run_test() {
 
   // Load the database, forcing migration to the current schema version. Then
   // test the expected set of cookies:
+  do_load_profile();
+
   // 1) All unexpired, unique cookies exist.
   do_check_eq(Services.cookiemgr.countCookiesFromHost("foo.com"), 20);
 

extensions/cookie/test/unit/test_schema_3_migration.js

@@ -22,6 +22,17 @@ function* do_run_test() {
   // Set up a profile.
   let profile = do_get_profile();
 
+  // Start the cookieservice, to force creation of a database.
+  // Get the sessionEnumerator to join the initialization in cookie thread
+  Services.cookiemgr.sessionEnumerator;
+
+  // Close the profile.
+  do_close_profile(test_generator);
+  yield;
+
+  // Remove the cookie file in order to create another database file.
+  do_get_cookie_file(profile).remove(false);
+
   // Create a schema 3 database.
   let schema3db = new CookieDatabaseConnection(do_get_cookie_file(profile), 3);
 
@@ -79,6 +90,8 @@ function* do_run_test() {
 
   // Load the database, forcing migration to the current schema version. Then
   // test the expected set of cookies:
+  do_load_profile();
+
   // 1) All unexpired, unique cookies exist.
   do_check_eq(Services.cookiemgr.countCookiesFromHost("foo.com"), 20);
 

netwerk/base/nsIOService.cpp

@@ -1489,6 +1489,11 @@ nsIOService::Observe(nsISupports *subject,
             nsCOMPtr<nsIPrefBranch> prefBranch;
             GetPrefBranch(getter_AddRefs(prefBranch));
             PrefsChanged(prefBranch, MANAGE_OFFLINE_STATUS_PREF);
+
+            // Bug 870460 - Read cookie database at an early-as-possible time
+            // off main thread. Hence, we have more chance to finish db query
+            // before something calls into the cookie service.
+            nsCOMPtr<nsISupports> cookieServ = do_GetService(NS_COOKIESERVICE_CONTRACTID);
         }
     } else if (!strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
         // Remember we passed XPCOM shutdown notification to prevent any

netwerk/cookie/nsCookieService.h

@@ -29,9 +29,13 @@
 #include "mozIStorageFunction.h"
 #include "nsIVariant.h"
 #include "nsIFile.h"
+#include "mozilla/Atomics.h"
 #include "mozilla/BasePrincipal.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Maybe.h"
+#include "mozilla/Monitor.h"
+#include "mozilla/UniquePtr.h"
+
 
 using mozilla::OriginAttributes;
 
@@ -43,6 +47,7 @@ class nsIObserverService;
 class nsIURI;
 class nsIChannel;
 class nsIArray;
+class nsIThread;
 class mozIStorageService;
 class mozIThirdPartyUtil;
 class ReadCookieDBListener;
@@ -88,13 +93,52 @@ class nsCookieEntry : public nsCookieKey
     ArrayType mCookies;
 };
 
+// struct for a constant cookie for threadsafe
+struct ConstCookie
+{
+  ConstCookie(const nsCString& aName,
+              const nsCString& aValue,
+              const nsCString& aHost,
+              const nsCString& aPath,
+              int64_t aExpiry,
+              int64_t aLastAccessed,
+              int64_t aCreationTime,
+              bool aIsSecure,
+              bool aIsHttpOnly,
+              const OriginAttributes &aOriginAttributes,
+              int32_t aSameSite)
+    : name(aName)
+    , value(aValue)
+    , host(aHost)
+    , path(aPath)
+    , expiry(aExpiry)
+    , lastAccessed(aLastAccessed)
+    , creationTime(aCreationTime)
+    , isSecure(aIsSecure)
+    , isHttpOnly(aIsHttpOnly)
+    , originAttributes(aOriginAttributes)
+    , sameSite(aSameSite)
+  {
+  }
+
+  const nsCString name;
+  const nsCString value;
+  const nsCString host;
+  const nsCString path;
+  const int64_t expiry;
+  const int64_t lastAccessed;
+  const int64_t creationTime;
+  const bool isSecure;
+  const bool isHttpOnly;
+  const OriginAttributes originAttributes;
+  const int32_t sameSite;
+};
+
 // encapsulates a (key, nsCookie) tuple for temporary storage purposes.
 struct CookieDomainTuple
 {
   nsCookieKey key;
-  RefPtr<nsCookie> cookie;
-
-  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
+  mozilla::UniquePtr<ConstCookie> cookie;
 };
 
 // encapsulates in-memory and on-disk DB states, so we can
@@ -137,17 +181,9 @@ public:
   // while the background read is taking place.
   nsCOMPtr<mozIStorageConnection>       syncConn;
   nsCOMPtr<mozIStorageStatement>        stmtReadDomain;
-  nsCOMPtr<mozIStoragePendingStatement> pendingRead;
   // The asynchronous read listener. This is a weak ref (storage has ownership)
   // since it may need to outlive the DBState's database connection.
   ReadCookieDBListener*                 readListener;
-  // An array of (baseDomain, cookie) tuples representing data read in
-  // asynchronously. This is merged into hostTable once read is complete.
-  nsTArray<CookieDomainTuple>           hostArray;
-  // A hashset of baseDomains read in synchronously, while the async read is
-  // in flight. This is used to keep track of which data in hostArray is stale
-  // when the time comes to merge.
-  nsTHashtable<nsCookieKey>        readSet;
 
   // DB completion handlers.
   nsCOMPtr<mozIStorageStatementCallback>  insertListener;
@@ -242,6 +278,8 @@ class nsCookieService final : public nsICookieService
     void                          PrefChanged(nsIPrefBranch *aPrefBranch);
     void                          InitDBStates();
     OpenDBResult                  TryInitDB(bool aDeleteExistingDB);
+    void                          InitDBConn();
+    nsresult                      InitDBConnInternal();
     nsresult                      CreateTableWorker(const char* aName);
     nsresult                      CreateIndex();
     nsresult                      CreateTable();
@@ -254,11 +292,8 @@ class nsCookieService final : public nsICookieService
     void                          HandleCorruptDB(DBState* aDBState);
     void                          RebuildCorruptDB(DBState* aDBState);
     OpenDBResult                  Read();
-    template<class T> nsCookie*   GetCookieFromRow(T &aRow, const OriginAttributes& aOriginAttributes);
-    void                          AsyncReadComplete();
-    void                          CancelAsyncRead(bool aPurgeReadSet);
-    void                          EnsureReadDomain(const nsCookieKey &aKey);
-    void                          EnsureReadComplete();
+    mozilla::UniquePtr<ConstCookie> GetCookieFromRow(mozIStorageStatement *aRow, const OriginAttributes &aOriginAttributes);
+    void                          EnsureReadComplete(bool aInitDBConn);
     nsresult                      NormalizeHost(nsCString &aHost);
     nsresult                      GetCookieStringCommon(nsIURI *aHostURI, nsIChannel *aChannel, bool aHttpBound, char** aCookie);
     void                          GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, const OriginAttributes& aOriginAttrs, nsCString &aCookie);
@@ -326,6 +361,14 @@ class nsCookieService final : public nsICookieService
     uint16_t                      mMaxCookiesPerHost;
     int64_t                       mCookiePurgeAge;
 
+    // thread
+    nsCOMPtr<nsIThread>           mThread;
+    mozilla::Monitor              mMonitor;
+    mozilla::Atomic<bool>         mInitializedDBStates;
+    mozilla::Atomic<bool>         mInitializedDBConn;
+    bool                          mAccumulatedWaitTelemetry;
+    nsTArray<CookieDomainTuple>   mReadArray;
+
     // friends!
     friend class DBListenerErrorHandler;
     friend class ReadCookieDBListener;

netwerk/cookie/test/unit/test_bug1321912.js

@@ -50,9 +50,9 @@ conn.executeSimpleSQL("INSERT INTO moz_cookies(" +
   now + ", " + now + ", " + now + ", 1, 1)");
 
 // Now start the cookie service, and then check the fields in the table.
-
-const cs = Cc["@mozilla.org/cookieService;1"].
-           getService(Ci.nsICookieService);
+// Get sessionEnumerator to wait for the initialization in cookie thread
+const enumerator = Cc["@mozilla.org/cookieService;1"].
+                   getService(Ci.nsICookieManager).sessionEnumerator;
 
 do_check_true(conn.schemaVersion, 8);
 let stmt = conn.createStatement("SELECT sql FROM sqlite_master " +

toolkit/components/telemetry/Histograms.json

@@ -4404,6 +4404,16 @@
     "n_buckets": 10,
     "description": "Time spent on SQLite read() (ms) *** No longer needed (bug 1156565). Delete histogram and accumulation code! ***"
   },
+  "MOZ_SQLITE_COOKIES_BLOCK_MAIN_THREAD_MS": {
+    "record_in_processes": ["main"],
+    "expires_in_version": "never",
+    "kind": "exponential",
+    "alert_emails": ["necko@mozilla.com", "junior@mozilla.com"],
+    "bug_numbers": [870460],
+    "high": 3000,
+    "n_buckets": 10,
+    "description": "Time spent on blocking main thread by startup cookie database read (ms)"
+  },
   "MOZ_SQLITE_COOKIES_READ_MAIN_THREAD_MS": {
     "record_in_processes": ["main", "content"],
     "expires_in_version": "40",