Bug 894042 - Disallow Places from creating async statements after asyncClose().
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 19 Jul 2013 15:51:40 +0200
changeset 139244 c2b8067fd55c31f57cf8ef9cf00ee3f6f1e9b197
parent 139243 7fb8ea5cd2fc17f15a12178987d5af2f0fcfd227
child 139245 d0d492aa6e63206302fe27f2e74855d0db704fed
push id1890
push userryanvm@gmail.com
push dateFri, 19 Jul 2013 17:44:21 +0000
treeherderfx-team@20848adc9980 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs894042
milestone25.0a1
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")