Bug 1338280 - Use generation numbers to allow multiple Places restarts. draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 22 Feb 2017 14:17:53 -0800
changeset 488406 d30e2adc8d6ff44a8028fcdbbfea5965862e194c
parent 488405 c2933bd01d9cf00ba23a506e84918a8b57f3d750
child 488407 03e6a3f5f3782719de19bb103e1d722073050e41
push id46512
push userbmo:kit@mozilla.com
push dateThu, 23 Feb 2017 02:18:17 +0000
bugs1338280
milestone54.0a1
Bug 1338280 - Use generation numbers to allow multiple Places restarts. In non-test code, the database is a singleton initialized at startup, and torn down on shutdown. This doesn't play well with tests that need to restart the database multiple times. To make these tests work, we use a "generation number": a counter that's incremented in lockstep between the database and its two shutdown blockers. The blockers maintain a corresponding counter that's incremented when shutdown begins. At that point, `Database::sGeneration < PlacesShutdownBlocker::sGeneration`, so we know the current database must be shutting down. When the current database is destroyed, we bump the database generation number for the next restart. During shutdown, `ConnectionShutdownBlocker` also fires two observer notifications, which are exposed as `AsyncShutdown` phases. These phases aren't designed to fire more than once, so we introduce a per-database tag passed as the `data` parameter to the observer. In the next patch, we'll change `AsyncShutdown` to support phases with tags. MozReview-Commit-ID: IXPyrfe5qOf
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/Shutdown.cpp
toolkit/components/places/Shutdown.h
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsPIPlacesDatabase.idl
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -323,30 +323,49 @@ NS_IMPL_ISUPPORTS(Database
 , nsIObserver
 , nsISupportsWeakReference
 )
 
 Database::Database()
   : mMainThreadStatements(mMainConn)
   , mMainThreadAsyncStatements(mMainConn)
   , mAsyncThreadStatements(mMainConn)
+  , mGeneration(sGeneration)
   , mDBPageSize(0)
   , mDatabaseStatus(nsINavHistoryService::DATABASE_STATUS_OK)
   , mClosed(false)
   , mClientsShutdown(new ClientsShutdownBlocker())
   , mConnectionShutdown(new ConnectionShutdownBlocker(this))
   , mMaxUrlLength(0)
 {
   MOZ_ASSERT(!XRE_IsContentProcess(),
              "Cannot instantiate Places in the content process");
   // Attempting to create two instances of the service?
   MOZ_ASSERT(!gDatabase);
   gDatabase = this;
 }
 
+// In non-test code, the database is a singleton initialized at startup, and
+// torn down on shutdown. This doesn't play well with tests that need to
+// shut down and restart the database multiple times. To make these tests work,
+// we use a "generation number": an incrementing counter that's shared between
+// the database and its two shutdown blockers. The blockers maintain a
+// corresponding counter that's incremented when shutdown begins. At that point,
+// `Database::sGeneration < PlacesShutdownBlocker::sGeneration`, so we know the
+// current database must be shutting down. When the current database is
+// destroyed, we bump the database generation number for the next restart.
+Atomic<uint32_t> Database::sGeneration(0);
+
+void
+Database::Tag(nsAString& aTag)
+{
+  aTag.AssignLiteral("Places database");
+  aTag.AppendInt(mGeneration);
+}
+
 already_AddRefed<nsIAsyncShutdownClient>
 Database::GetProfileChangeTeardownPhase()
 {
   nsCOMPtr<nsIAsyncShutdownService> asyncShutdownSvc = services::GetAsyncShutdown();
   MOZ_ASSERT(asyncShutdownSvc);
   if (NS_WARN_IF(!asyncShutdownSvc)) {
     return nullptr;
   }
@@ -373,26 +392,27 @@ Database::GetProfileBeforeChangePhase()
   DebugOnly<nsresult> rv = asyncShutdownSvc->
     GetProfileBeforeChange(getter_AddRefs(shutdownPhase));
   MOZ_ASSERT(NS_SUCCEEDED(rv));
   return shutdownPhase.forget();
 }
 
 Database::~Database()
 {
+  sGeneration++;
 }
 
 bool
 Database::IsShutdownStarted() const
 {
   if (!mConnectionShutdown) {
     // We have already broken the cycle between `this` and `mConnectionShutdown`.
     return true;
   }
-  return mConnectionShutdown->IsStarted();
+  return mConnectionShutdown->IsStarted(mGeneration);
 }
 
 already_AddRefed<mozIStorageAsyncStatement>
 Database::GetAsyncStatement(const nsACString& aQuery) const
 {
   if (IsShutdownStarted()) {
     return nullptr;
   }
@@ -418,17 +438,19 @@ Database::GetClientsShutdown()
   MOZ_ASSERT(mClientsShutdown);
   return mClientsShutdown->GetClient();
 }
 
 // static
 already_AddRefed<Database>
 Database::GetDatabase()
 {
-  if (PlacesShutdownBlocker::IsStarted()) {
+  // This function can be called from multiple threads; it's important that
+  // it not call any main thread-only functions.
+  if (PlacesShutdownBlocker::IsStarted(sGeneration)) {
     return nullptr;
   }
   return GetSingleton();
 }
 
 nsresult
 Database::Init()
 {
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -194,16 +194,18 @@ public:
    * @return The cached statement.
    * @note Always null check the result.
    * @note AsyncStatements are automatically reset on execution.
    */
   already_AddRefed<mozIStorageAsyncStatement> GetAsyncStatement(const nsACString& aQuery) const;
 
   uint32_t MaxUrlLength();
 
+  void Tag(nsAString& aTag);
+
 protected:
   /**
    * Finalizes the cached statements and closes the database connection.
    * A TOPIC_PLACES_CONNECTION_CLOSED notification is fired when done.
    */
   void Shutdown();
 
   bool IsShutdownStarted() const;
@@ -292,23 +294,25 @@ private:
   ~Database();
 
   /**
    * Singleton getter, invoked by class instantiation.
    */
   static already_AddRefed<Database> GetSingleton();
 
   static Database* gDatabase;
+  static Atomic<uint32_t> sGeneration;
 
   nsCOMPtr<mozIStorageConnection> mMainConn;
 
   mutable StatementCache mMainThreadStatements;
   mutable AsyncStatementCache mMainThreadAsyncStatements;
   mutable StatementCache mAsyncThreadStatements;
 
+  uint32_t mGeneration;
   int32_t mDBPageSize;
   uint16_t mDatabaseStatus;
   bool mClosed;
 
   /**
    * Phases for shutting down the Database.
    * See Shutdown.h for further details about the shutdown procedure.
    */
--- a/toolkit/components/places/Shutdown.cpp
+++ b/toolkit/components/places/Shutdown.cpp
@@ -3,29 +3,28 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "Shutdown.h"
 #include "mozilla/Unused.h"
 
 namespace mozilla {
 namespace places {
 
-uint16_t PlacesShutdownBlocker::sCounter = 0;
-Atomic<bool> PlacesShutdownBlocker::sIsStarted(false);
+Atomic<uint32_t> PlacesShutdownBlocker::sGeneration(0);
 
 PlacesShutdownBlocker::PlacesShutdownBlocker(const nsString& aName)
   : mName(aName)
   , mState(NOT_STARTED)
-  , mCounter(sCounter++)
+  , mGeneration(sGeneration)
 {
   MOZ_ASSERT(NS_IsMainThread());
   // During tests, we can end up with the Database singleton being resurrected.
   // Make sure that each instance of DatabaseShutdown has a unique name.
-  if (mCounter > 1) {
-    mName.AppendInt(mCounter);
+  if (mGeneration > 0) {
+    mName.AppendInt(mGeneration);
   }
 }
 
 // nsIAsyncShutdownBlocker
 NS_IMETHODIMP
 PlacesShutdownBlocker::GetName(nsAString& aName)
 {
   aName = mName;
@@ -166,23 +165,25 @@ ConnectionShutdownBlocker::ConnectionShu
 // nsIAsyncShutdownBlocker
 NS_IMETHODIMP
 ConnectionShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
 {
   MOZ_ASSERT(NS_IsMainThread());
   mParentClient = new nsMainThreadPtrHolder<nsIAsyncShutdownClient>(aParentClient);
   mState = RECEIVED_BLOCK_SHUTDOWN;
   // Annotate that Database shutdown started.
-  sIsStarted = true;
+  sGeneration.compareExchange(mGeneration, mGeneration + 1);
 
   // Fire internal database closing notification.
   nsCOMPtr<nsIObserverService> os = services::GetObserverService();
   MOZ_ASSERT(os);
   if (os) {
-    Unused << os->NotifyObservers(nullptr, TOPIC_PLACES_WILL_CLOSE_CONNECTION, nullptr);
+    nsAutoString tag;
+    mDatabase->Tag(tag);
+    Unused << os->NotifyObservers(nullptr, TOPIC_PLACES_WILL_CLOSE_CONNECTION, tag.get());
   }
   mState = NOTIFIED_OBSERVERS_PLACES_WILL_CLOSE_CONNECTION;
 
   // At this stage, any use of this database is forbidden. Get rid of
   // `gDatabase`. Note, however, that the database could be
   // resurrected.  This can happen in particular during tests.
   MOZ_ASSERT(Database::gDatabase == nullptr || Database::gDatabase == mDatabase);
   Database::gDatabase = nullptr;
@@ -195,27 +196,30 @@ ConnectionShutdownBlocker::BlockShutdown
 
 // mozIStorageCompletionCallback
 NS_IMETHODIMP
 ConnectionShutdownBlocker::Complete(nsresult, nsISupports*)
 {
   MOZ_ASSERT(NS_IsMainThread());
   mState = RECEIVED_STORAGESHUTDOWN_COMPLETE;
 
+  nsAutoString tag;
+  mDatabase->Tag(tag);
+
   // The connection is closed, the Database has no more use, so we can break
   // possible cycles.
   mDatabase = nullptr;
 
   // Notify the connection has gone.
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   MOZ_ASSERT(os);
   if (os) {
     MOZ_ALWAYS_SUCCEEDS(os->NotifyObservers(nullptr,
 					    TOPIC_PLACES_CONNECTION_CLOSED,
-					    nullptr));
+					    tag.get()));
   }
   mState = NOTIFIED_OBSERVERS_PLACES_CONNECTION_CLOSED;
 
   // mParentClient is nullptr in tests
   if (mParentClient) {
     nsresult rv = mParentClient->RemoveBlocker(this);
     if (NS_WARN_IF(NS_FAILED(rv))) return rv;
     mParentClient = nullptr;
--- a/toolkit/components/places/Shutdown.h
+++ b/toolkit/components/places/Shutdown.h
@@ -64,18 +64,18 @@ public:
   NS_DECL_NSIASYNCSHUTDOWNBLOCKER
 
   explicit PlacesShutdownBlocker(const nsString& aName);
 
   /**
    * `true` if we have not started shutdown, i.e.  if
    * `BlockShutdown()` hasn't been called yet, false otherwise.
    */
-  static bool IsStarted() {
-    return sIsStarted;
+  static bool IsStarted(uint32_t aDBGeneration) {
+    return aDBGeneration < sGeneration;
   }
 
   // The current state, used internally and for forensics/debugging purposes.
   // Not all the states make sense for all the derived classes.
   enum States {
     NOT_STARTED,
     // Execution of `BlockShutdown` in progress.
     RECEIVED_BLOCK_SHUTDOWN,
@@ -107,22 +107,22 @@ protected:
   nsString mName;
   // The current state, see States.
   States mState;
   // The barrier optionally used to wait for clients.
   nsMainThreadPtrHandle<nsIAsyncShutdownBarrier> mBarrier;
   // The parent object who registered this as a blocker.
   nsMainThreadPtrHandle<nsIAsyncShutdownClient> mParentClient;
 
-  // As tests may resurrect a dead `Database`, we use a counter to
-  // give the instances of `PlacesShutdownBlocker` unique names.
-  uint16_t mCounter;
-  static uint16_t sCounter;
-
-  static Atomic<bool> sIsStarted;
+  // As tests may resurrect a dead `Database`, we use the generation
+  // number to give the instances of `PlacesShutdownBlocker` unique names.
+  // We also use this to determine whether a database has started
+  // shutdown.
+  uint32_t mGeneration;
+  static Atomic<uint32_t> sGeneration;
 
   virtual ~PlacesShutdownBlocker() {}
 };
 
 /**
  * Blocker also used to wait for clients, through an owned barrier.
  */
 class ClientsShutdownBlocker final : public PlacesShutdownBlocker
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -2944,16 +2944,25 @@ nsNavHistory::GetDBConnection(mozIStorag
   NS_ENSURE_STATE(DB);
   RefPtr<mozIStorageConnection> connection = DB->MainConn();
   connection.forget(_DBConnection);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
+nsNavHistory::GetDBTag(nsAString& aTag)
+{
+  RefPtr<Database> DB = GetDatabase();
+  NS_ENSURE_STATE(DB);
+  DB->Tag(aTag);
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 nsNavHistory::GetShutdownClient(nsIAsyncShutdownClient **_shutdownClient)
 {
   NS_ENSURE_ARG_POINTER(_shutdownClient);
   RefPtr<Database> DB = GetDatabase();
   NS_ENSURE_STATE(DB);
   RefPtr<nsIAsyncShutdownClient> client = DB->GetClientsShutdown();
   MOZ_ASSERT(client);
   client.forget(_shutdownClient);
--- a/toolkit/components/places/nsPIPlacesDatabase.idl
+++ b/toolkit/components/places/nsPIPlacesDatabase.idl
@@ -21,16 +21,18 @@ interface nsIAsyncShutdownClient;
 [scriptable, uuid(366ee63e-a413-477d-9ad6-8d6863e89401)]
 interface nsPIPlacesDatabase : nsISupports
 {
   /**
    * The database connection used by Places.
    */
   readonly attribute mozIStorageConnection DBConnection;
 
+  readonly attribute AString DBTag;
+
   /**
    * Asynchronously executes the statement created from queries.
    *
    * @see nsINavHistoryService::executeQueries
    * @note THIS IS A TEMPORARY API.  Don't rely on it, since it will be replaced
    *       in future versions by a real async querying API.
    * @note Results obtained from this method differ from results obtained from
    *       executeQueries, because there is additional filtering and sorting