Bug 894042 - Disallow Places from creating async statements after asyncClose().
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 19 Jul 2013 15:51:40 +0200
changeset 151508 c2b8067fd55c31f57cf8ef9cf00ee3f6f1e9b197
parent 151507 7fb8ea5cd2fc17f15a12178987d5af2f0fcfd227
child 151509 d0d492aa6e63206302fe27f2e74855d0db704fed
push id2859
push userakeybl@mozilla.com
push dateMon, 16 Sep 2013 19:14:59 +0000
treeherdermozilla-beta@87d3c51cd2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs894042
milestone25.0a1
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 894042 - Disallow Places from creating async statements after asyncClose(). r=Mano
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/tests/bookmarks/test_async_observers.js
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -339,16 +339,17 @@ NS_IMPL_THREADSAFE_ISUPPORTS2(Database
 
 Database::Database()
   : mMainThreadStatements(mMainConn)
   , mMainThreadAsyncStatements(mMainConn)
   , mAsyncThreadStatements(mMainConn)
   , mDBPageSize(0)
   , mDatabaseStatus(nsINavHistoryService::DATABASE_STATUS_OK)
   , mShuttingDown(false)
+  , mClosed(false)
 {
   // Attempting to create two instances of the service?
   MOZ_ASSERT(!gDatabase);
   gDatabase = this;
 }
 
 Database::~Database()
 {
@@ -1909,34 +1910,35 @@ Database::MigrateV23Up()
   return NS_OK;
 }
 
 void
 Database::Shutdown()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!mShuttingDown);
+  MOZ_ASSERT(!mClosed);
+
+  mShuttingDown = true;
 
   mMainThreadStatements.FinalizeStatements();
   mMainThreadAsyncStatements.FinalizeStatements();
 
   nsRefPtr< FinalizeStatementCacheProxy<mozIStorageStatement> > event =
     new FinalizeStatementCacheProxy<mozIStorageStatement>(
           mAsyncThreadStatements, NS_ISUPPORTS_CAST(nsIObserver*, this)
         );
   DispatchToAsyncThread(event);
 
   nsRefPtr<BlockingConnectionCloseCallback> closeListener =
     new BlockingConnectionCloseCallback();
   (void)mMainConn->AsyncClose(closeListener);
   closeListener->Spin();
 
-  // Don't set this earlier, otherwise some internal helper used on shutdown
-  // may bail out.
-  mShuttingDown = true;
+  mClosed = true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //// nsIObserver
 
 NS_IMETHODIMP
 Database::Observe(nsISupports *aSubject,
                   const char *aTopic,
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -117,17 +117,17 @@ public:
    * Dispatches a runnable to the connection async thread, to be serialized
    * with async statements.
    *
    * @param aEvent
    *        The runnable to be dispatched.
    */
   void DispatchToAsyncThread(nsIRunnable* aEvent) const
   {
-    if (mShuttingDown) {
+    if (mClosed) {
       return;
     }
     nsCOMPtr<nsIEventTarget> target = do_GetInterface(mMainConn);
     if (target) {
       (void)target->Dispatch(aEvent, NS_DISPATCH_NORMAL);
     }
   }
 
@@ -292,14 +292,15 @@ private:
 
   mutable StatementCache mMainThreadStatements;
   mutable AsyncStatementCache mMainThreadAsyncStatements;
   mutable StatementCache mAsyncThreadStatements;
 
   int32_t mDBPageSize;
   uint16_t mDatabaseStatus;
   bool mShuttingDown;
+  bool mClosed;
 };
 
 } // namespace places
 } // namespace mozilla
 
 #endif // mozilla_places_Database_h_
--- a/toolkit/components/places/tests/bookmarks/test_async_observers.js
+++ b/toolkit/components/places/tests/bookmarks/test_async_observers.js
@@ -118,16 +118,48 @@ add_task(function test_remove_page()
   yield observerPromise;
 });
 
 add_task(function cleanup()
 {
   PlacesUtils.bookmarks.removeObserver(observer, false);
 });
 
+add_task(function shutdown()
+{
+  // Check that async observers don't try to create async statements after
+  // shutdown.  That would cause assertions, since the async thread is gone
+  // already.  Note that in such a case the notifications are not fired, so we
+  // cannot test for them.
+  // Put an history notification that triggers AsyncGetBookmarksForURI between
+  // asyncClose() and the actual connection closing.  Enqueuing a main-thread
+  // event just after places-will-close-connection should ensure it runs before
+  // places-connection-closed.
+  // Notice this code is not using helpers cause it depends on a very specific
+  // order, a change in the helpers code could make this test useless.
+  let deferred = Promise.defer();
+
+  Services.obs.addObserver(function onNotification() {
+    Services.obs.removeObserver(onNotification, "places-will-close-connection");
+    do_check_true(true, "Observed fake places shutdown");
+
+    Services.tm.mainThread.dispatch(() => {
+      // WARNING: this is very bad, never use out of testing code.
+      PlacesUtils.bookmarks.QueryInterface(Ci.nsINavHistoryObserver)
+                           .onPageChanged(NetUtil.newURI("http://book.ma.rk/"),
+                                          Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON,
+                                          "test", "test");
+      deferred.resolve(promiseTopicObserved("places-connection-closed"));
+    }, Ci.nsIThread.DISPATCH_NORMAL);
+  }, "places-will-close-connection", false);
+  shutdownPlaces();
+
+  yield deferred.promise;
+});
+
 function run_test()
 {
   // Add multiple bookmarks to the same uri.
   observer.bookmarks.push(
     PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
                                          NetUtil.newURI("http://book.ma.rk/"),
                                          PlacesUtils.bookmarks.DEFAULT_INDEX,
                                          "Bookmark")