Bug 1275878 - Part 2: Replace places-will-close-connection notification with a shutdown blocker. r=adw

MozReview-Commit-ID: A2sn2OreX4K

--HG--
extra : rebase_source : 6c9dd729c2aad0b5870f0cbf1e565a33376c4f56

browser/components/places/tests/unit/test_clearHistory_shutdown.js

@@ -19,7 +19,6 @@ const TOPIC_CONNECTION_CLOSED = "places-connection-closed";
 
 var EXPECTED_NOTIFICATIONS = [
   "places-shutdown",
-  "places-will-close-connection",
   "places-expiration-finished",
   "places-connection-closed"
 ];

browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js

@@ -6,67 +6,40 @@
 // private browsing mode.
 "use strict";
 
-Cu.import("resource://gre/modules/Services.jsm");
-Cu.import("resource://gre/modules/PlacesUtils.jsm");
-
 add_task(async function test() {
   const TEST_URL = "http://mochi.test:8888/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.html"
-  const TEST_URI = Services.io.newURI(TEST_URL);
   const TITLE_1 = "Title 1";
   const TITLE_2 = "Title 2";
 
-  function waitForTitleChanged() {
-    return new Promise(resolve => {
-      let historyObserver = {
-        onTitleChanged(uri, pageTitle) {
-          PlacesUtils.history.removeObserver(historyObserver, false);
-          resolve({uri, pageTitle});
-        },
-        onBeginUpdateBatch() {},
-        onEndUpdateBatch() {},
-        onVisit() {},
-        onDeleteURI() {},
-        onClearHistory() {},
-        onPageChanged() {},
-        onDeleteVisits() {},
-        QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver])
-      };
+  await PlacesUtils.history.clear();
 
-      PlacesUtils.history.addObserver(historyObserver);
-    });
-  }
-
-  await PlacesTestUtils.clearHistory();
-
-  let tabToClose = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, TEST_URL);
-  await waitForTitleChanged();
-  is(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1, "The title matches the orignal title after first visit");
-
-  let place = {
-    uri: TEST_URI,
-    title: TITLE_2,
-    visits: [{
-      visitDate: Date.now() * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK
-    }]
-  };
-  PlacesUtils.asyncHistory.updatePlaces(place, {
-    handleError: () => ok(false, "Unexpected error in adding visit."),
-    handleResult() { },
-    handleCompletion() {}
+  let promiseTitleChanged = PlacesTestUtils.waitForNotification(
+    "onTitleChanged", (uri, title) => uri.spec == TEST_URL, "history");
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
+  registerCleanupFunction(async () => {
+    await BrowserTestUtils.removeTab(tab);
   });
+  info("Wait for a title change notification.");
+  await promiseTitleChanged;
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, TITLE_1,
+     "The title matches the orignal title after first visit");
 
-  await waitForTitleChanged();
-  is(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_2, "The title matches the updated title after updating visit");
+  promiseTitleChanged = PlacesTestUtils.waitForNotification(
+    "onTitleChanged", (uri, title) => uri.spec == TEST_URL, "history");
+  await PlacesTestUtils.addVisits({ uri: TEST_URL, title: TITLE_2 });
+  info("Wait for a title change notification.");
+  await promiseTitleChanged;
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, TITLE_2,
+     "The title matches the orignal title after updating visit");
 
   let privateWin = await BrowserTestUtils.openNewBrowserWindow({private: true});
-  await BrowserTestUtils.browserLoaded(privateWin.gBrowser.addTab(TEST_URL).linkedBrowser);
-
-  is(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_2, "The title remains the same after visiting in private window");
-  await PlacesTestUtils.clearHistory();
-
-  // Cleanup
-  BrowserTestUtils.closeWindow(privateWin);
-  gBrowser.removeTab(tabToClose);
+  registerCleanupFunction(async () => {
+    await BrowserTestUtils.closeWindow(privateWin);
+  });
+  await BrowserTestUtils.openNewForegroundTab(privateWin.gBrowser, TEST_URL);
+  // Wait long enough to be sure history didn't set a title.
+  await new Promise(resolve => setTimeout(resolve, 1000));
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, TITLE_2,
+     "The title remains the same after visiting in private window");
 });
 

browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js

@@ -18,78 +18,48 @@ add_task(async function test() {
   }
 
   await cleanup();
-
-  let deferredFirst = PromiseUtils.defer();
-  let deferredSecond = PromiseUtils.defer();
-  let deferredThird = PromiseUtils.defer();
-
-  let testNumber = 0;
-  let historyObserver = {
-    onTitleChanged(aURI, aPageTitle) {
-      if (aURI.spec != TEST_URL)
-        return;
-      switch (++testNumber) {
-        case 1:
-          // The first time that the page is loaded
-          deferredFirst.resolve(aPageTitle);
-          break;
-        case 2:
-          // The second time that the page is loaded
-          deferredSecond.resolve(aPageTitle);
-          break;
-        case 3:
-          // After clean up
-          deferredThird.resolve(aPageTitle);
-          break;
-        default:
-          // Checks that opening the page in a private window should not fire a
-          // title change.
-          ok(false, "Title changed. Unexpected pass: " + testNumber);
-      }
-    },
-
-    onBeginUpdateBatch() {},
-    onEndUpdateBatch() {},
-    onVisit() {},
-    onDeleteURI() {},
-    onClearHistory() {},
-    onPageChanged() {},
-    onDeleteVisits() {},
-    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver])
-  };
-  PlacesUtils.history.addObserver(historyObserver);
-
+  registerCleanupFunction(cleanup);
 
   let win = await BrowserTestUtils.openNewBrowserWindow();
-  win.gBrowser.selectedTab = win.gBrowser.addTab(TEST_URL);
-  let aPageTitle = await deferredFirst.promise;
-  // The first time that the page is loaded
-  is(aPageTitle, "No Cookie",
+  registerCleanupFunction(async () => {
+    await BrowserTestUtils.closeWindow(win);
+  });
+
+  let promiseTitleChanged = PlacesTestUtils.waitForNotification(
+    "onTitleChanged", (uri, title) => uri.spec == TEST_URL, "history");
+  await BrowserTestUtils.openNewForegroundTab(win.gBrowser, TEST_URL);
+  await promiseTitleChanged;
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, "No Cookie",
      "The page should be loaded without any cookie for the first time");
 
-  win.gBrowser.selectedTab = win.gBrowser.addTab(TEST_URL);
-  aPageTitle = await deferredSecond.promise;
-  // The second time that the page is loaded
-  is(aPageTitle, "Cookie",
+  promiseTitleChanged = PlacesTestUtils.waitForNotification(
+    "onTitleChanged", (uri, title) => uri.spec == TEST_URL, "history");
+  await BrowserTestUtils.openNewForegroundTab(win.gBrowser, TEST_URL);
+  await promiseTitleChanged;
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, "Cookie",
      "The page should be loaded with a cookie for the second time");
 
   await cleanup();
 
-  win.gBrowser.selectedTab = win.gBrowser.addTab(TEST_URL);
-  aPageTitle = await deferredThird.promise;
-  // After clean up
-  is(aPageTitle, "No Cookie",
+  promiseTitleChanged = PlacesTestUtils.waitForNotification(
+    "onTitleChanged", (uri, title) => uri.spec == TEST_URL, "history");
+  await BrowserTestUtils.openNewForegroundTab(win.gBrowser, TEST_URL);
+  await promiseTitleChanged;
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, "No Cookie",
      "The page should be loaded without any cookie again");
 
+  // Reopen the page in a private browser window, it should not notify a title
+  // change.
   let win2 = await BrowserTestUtils.openNewBrowserWindow({private: true});
+  registerCleanupFunction(async () => {
+    let promisePBExit = TestUtils.topicObserved("last-pb-context-exited");
+    await BrowserTestUtils.closeWindow(win2);
+    await promisePBExit;
+  });
 
-  let private_tab = win2.gBrowser.addTab(TEST_URL);
-  win2.gBrowser.selectedTab = private_tab;
-  await BrowserTestUtils.browserLoaded(private_tab.linkedBrowser);
-
-  // Cleanup
-  await cleanup();
-  PlacesUtils.history.removeObserver(historyObserver);
-  await BrowserTestUtils.closeWindow(win);
-  await BrowserTestUtils.closeWindow(win2);
+  await BrowserTestUtils.openNewForegroundTab(win2.gBrowser, TEST_URL);
+  // Wait long enough to be sure history didn't set a title.
+  await new Promise(resolve => setTimeout(resolve, 1000));
+  is((await PlacesUtils.history.fetch(TEST_URL)).title, "No Cookie",
+     "The title remains the same after visiting in private window");
 });

browser/components/privatebrowsing/test/browser/head.js

@@ -2,8 +2,13 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 var {PromiseUtils} = Cu.import("resource://gre/modules/PromiseUtils.jsm", {});
+Cu.import("resource://gre/modules/Services.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
+  "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
   "resource://testing-common/PlacesTestUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "TestUtils",
+  "resource://testing-common/TestUtils.jsm");
 
 function whenNewWindowLoaded(aOptions, aCallback) {
   let win = OpenBrowserWindow(aOptions);

toolkit/components/asyncshutdown/AsyncShutdown.jsm

@@ -1003,7 +1003,6 @@ Barrier.prototype = Object.freeze({
 if (!isContent) {
   this.AsyncShutdown.profileChangeTeardown = getPhase("profile-change-teardown");
   this.AsyncShutdown.profileBeforeChange = getPhase("profile-before-change");
-  this.AsyncShutdown.placesClosingInternalConnection = getPhase("places-will-close-connection");
   this.AsyncShutdown.sendTelemetry = getPhase("profile-before-change-telemetry");
 }
 

toolkit/components/places/Database.cpp

@@ -456,8 +456,17 @@ Database::GetStatement(const nsACString& aQuery)
 already_AddRefed<nsIAsyncShutdownClient>
 Database::GetClientsShutdown()
 {
-  MOZ_ASSERT(mClientsShutdown);
-  return mClientsShutdown->GetClient();
+  if (mClientsShutdown)
+    return mClientsShutdown->GetClient();
+  return nullptr;
+}
+
+already_AddRefed<nsIAsyncShutdownClient>
+Database::GetConnectionShutdown()
+{
+  if (mConnectionShutdown)
+    return mConnectionShutdown->GetClient();
+  return nullptr;
 }
 
 // static

toolkit/components/places/Database.h

@@ -32,10 +32,6 @@
 // you should only use this notification, next ones are intended only for
 // internal Places use.
 #define TOPIC_PLACES_SHUTDOWN "places-shutdown"
-// For Internal use only.  Fired when connection is about to be closed, only
-// cleanup tasks should run at this stage, nothing should be added to the
-// database, nor APIs should be called.
-#define TOPIC_PLACES_WILL_CLOSE_CONNECTION "places-will-close-connection"
 // Fired when the connection has gone, nothing will work from now on.
 #define TOPIC_PLACES_CONNECTION_CLOSED "places-connection-closed"
 
@@ -86,6 +82,11 @@ public:
    */
   already_AddRefed<nsIAsyncShutdownClient> GetClientsShutdown();
 
+  /**
+   * The AsyncShutdown client used by clients of this API to be informed of connection shutdown.
+   */
+  already_AddRefed<nsIAsyncShutdownClient> GetConnectionShutdown();
+
   /**
    * Getter to use when instantiating the class.
    *

toolkit/components/places/PlacesUtils.jsm

@@ -2142,7 +2142,8 @@ function setupDbForShutdown(conn, name) {
       // Before it can safely close its connection, we need to make sure
       // that we have closed the high-level connection.
       try {
-        AsyncShutdown.placesClosingInternalConnection.addBlocker(`${name} closing as part of Places shutdown`,
+        PlacesUtils.history.connectionShutdownClient.jsclient.addBlocker(
+          `${name} closing as part of Places shutdown`,
           async function() {
             state = "1. Service has initiated shutdown";
 

toolkit/components/places/Shutdown.cpp

@@ -22,6 +22,19 @@ PlacesShutdownBlocker::PlacesShutdownBlocker(const nsString& aName)
   if (mCounter > 1) {
     mName.AppendInt(mCounter);
   }
+  // Create a barrier that will be exposed to clients through GetClient(), so
+  // they can block Places shutdown.
+  nsCOMPtr<nsIAsyncShutdownService> asyncShutdown = services::GetAsyncShutdown();
+  MOZ_ASSERT(asyncShutdown);
+  if (asyncShutdown) {
+    nsCOMPtr<nsIAsyncShutdownBarrier> barrier;
+    nsresult rv = asyncShutdown->MakeBarrier(mName, getter_AddRefs(barrier));
+    MOZ_ALWAYS_SUCCEEDS(rv);
+    if (NS_SUCCEEDED(rv) && barrier) {
+      mBarrier = new nsMainThreadPtrHolder<nsIAsyncShutdownBarrier>(
+        "PlacesShutdownBlocker::mBarrier", barrier);
+    }
+  }
 }
 
 // nsIAsyncShutdownBlocker
@@ -71,39 +84,8 @@ PlacesShutdownBlocker::GetState(nsIPropertyBag** _state)
   return NS_OK;
 }
 
-// nsIAsyncShutdownBlocker
-NS_IMETHODIMP
-PlacesShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
-{
-  MOZ_ASSERT(false, "should always be overridden");
-  return NS_ERROR_NOT_IMPLEMENTED;
-}
-
-NS_IMPL_ISUPPORTS(
-  PlacesShutdownBlocker,
-  nsIAsyncShutdownBlocker
-)
-
-////////////////////////////////////////////////////////////////////////////////
-
-ClientsShutdownBlocker::ClientsShutdownBlocker()
-  : PlacesShutdownBlocker(NS_LITERAL_STRING("Places Clients shutdown"))
-{
-  MOZ_ASSERT(NS_IsMainThread());
-  // Create a barrier that will be exposed to clients through GetClient(), so
-  // they can block Places shutdown.
-  nsCOMPtr<nsIAsyncShutdownService> asyncShutdown = services::GetAsyncShutdown();
-  MOZ_ASSERT(asyncShutdown);
-  if (asyncShutdown) {
-    nsCOMPtr<nsIAsyncShutdownBarrier> barrier;
-    MOZ_ALWAYS_SUCCEEDS(asyncShutdown->MakeBarrier(mName, getter_AddRefs(barrier)));
-    mBarrier = new nsMainThreadPtrHolder<nsIAsyncShutdownBarrier>(
-      "ClientsShutdownBlocker::mBarrier", barrier);
-  }
-}
-
 already_AddRefed<nsIAsyncShutdownClient>
-ClientsShutdownBlocker::GetClient()
+PlacesShutdownBlocker::GetClient()
 {
   nsCOMPtr<nsIAsyncShutdownClient> client;
   if (mBarrier) {
@@ -114,7 +96,7 @@ ClientsShutdownBlocker::GetClient()
 
 // nsIAsyncShutdownBlocker
 NS_IMETHODIMP
-ClientsShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
+PlacesShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
 {
   MOZ_ASSERT(NS_IsMainThread());
   mParentClient = new nsMainThreadPtrHolder<nsIAsyncShutdownClient>(
@@ -132,6 +114,28 @@ ClientsShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
   return NS_OK;
 }
 
+// nsIAsyncShutdownCompletionCallback
+NS_IMETHODIMP
+PlacesShutdownBlocker::Done()
+{
+  MOZ_ASSERT(false, "Should always be overridden");
+  return NS_OK;
+}
+
+NS_IMPL_ISUPPORTS(
+  PlacesShutdownBlocker,
+  nsIAsyncShutdownBlocker,
+  nsIAsyncShutdownCompletionCallback
+)
+
+////////////////////////////////////////////////////////////////////////////////
+
+ClientsShutdownBlocker::ClientsShutdownBlocker()
+  : PlacesShutdownBlocker(NS_LITERAL_STRING("Places Clients shutdown"))
+{
+  // Do nothing.
+}
+
 // nsIAsyncShutdownCompletionCallback
 NS_IMETHODIMP
 ClientsShutdownBlocker::Done()
@@ -150,10 +154,9 @@ ClientsShutdownBlocker::Done()
   return NS_OK;
 }
 
-NS_IMPL_ISUPPORTS_INHERITED(
+NS_IMPL_ISUPPORTS_INHERITED0(
   ClientsShutdownBlocker,
-  PlacesShutdownBlocker,
-  nsIAsyncShutdownCompletionCallback
+  PlacesShutdownBlocker
 )
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -165,25 +168,17 @@ ConnectionShutdownBlocker::ConnectionShutdownBlocker(Database* aDatabase)
   // Do nothing.
 }
 
-// nsIAsyncShutdownBlocker
+// nsIAsyncShutdownCompletionCallback
 NS_IMETHODIMP
-ConnectionShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
+ConnectionShutdownBlocker::Done()
 {
-  MOZ_ASSERT(NS_IsMainThread());
-  mParentClient = new nsMainThreadPtrHolder<nsIAsyncShutdownClient>(
-    "ConnectionShutdownBlocker::mParentClient", aParentClient);
-  mState = RECEIVED_BLOCK_SHUTDOWN;
+  // At this point all the clients are done, we can stop blocking the shutdown
+  // phase.
+  mState = RECEIVED_DONE;
+
   // Annotate that Database shutdown started.
   sIsStarted = true;
 
-  // 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);
-  }
-  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.
@@ -193,6 +188,7 @@ ConnectionShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
   // Database::Shutdown will invoke Complete once the connection is closed.
   mDatabase->Shutdown();
   mState = CALLED_STORAGESHUTDOWN;
+  mBarrier = nullptr;
   return NS_OK;
 }
 

toolkit/components/places/Shutdown.h

@@ -43,10 +43,6 @@ class Database;
  * Database::Init (see Database::mConnectionShutdown).
  * When profile-before-change is observer by async shutdown, it calls
  * ConnectionShutdownBlocker::BlockShutdown.
- * This is the last chance for any Places internal work, like privacy cleanups,
- * before the connection is closed. This a places-will-close-connection
- * notification is sent to legacy clients that must complete any operation in
- * the same tick, since we won't wait for them.
  * Then the control is passed to Database::Shutdown, that executes some sanity
  * checks, clears cached statements and proceeds with asyncClose.
  * Once the connection is definitely closed, Database will call back
@@ -58,13 +54,17 @@ class Database;
  * A base AsyncShutdown blocker in charge of shutting down Places.
  */
 class PlacesShutdownBlocker : public nsIAsyncShutdownBlocker
+                            , public nsIAsyncShutdownCompletionCallback
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIASYNCSHUTDOWNBLOCKER
+  NS_DECL_NSIASYNCSHUTDOWNCOMPLETIONCALLBACK
 
   explicit PlacesShutdownBlocker(const nsString& aName);
 
+  already_AddRefed<nsIAsyncShutdownClient> GetClient();
+
   /**
    * `true` if we have not started shutdown, i.e.  if
    * `BlockShutdown()` hasn't been called yet, false otherwise.
@@ -126,18 +126,13 @@ protected:
  * Blocker also used to wait for clients, through an owned barrier.
  */
 class ClientsShutdownBlocker final : public PlacesShutdownBlocker
-                                   , public nsIAsyncShutdownCompletionCallback
 {
 public:
   NS_DECL_ISUPPORTS_INHERITED
-  NS_DECL_NSIASYNCSHUTDOWNCOMPLETIONCALLBACK
 
   explicit ClientsShutdownBlocker();
 
-  NS_IMETHOD BlockShutdown(nsIAsyncShutdownClient* aParentClient) override;
-
-  already_AddRefed<nsIAsyncShutdownClient> GetClient();
-
+  NS_IMETHOD Done() override;
 private:
   ~ClientsShutdownBlocker() {}
 };
@@ -152,10 +147,10 @@ public:
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_MOZISTORAGECOMPLETIONCALLBACK
 
-  NS_IMETHOD BlockShutdown(nsIAsyncShutdownClient* aParentClient) override;
-
   explicit ConnectionShutdownBlocker(mozilla::places::Database* aDatabase);
 
+  NS_IMETHOD Done() override;
+
 private:
   ~ConnectionShutdownBlocker() {}
 

toolkit/components/places/nsNavHistory.cpp

@@ -2905,10 +2905,23 @@ NS_IMETHODIMP
 nsNavHistory::GetShutdownClient(nsIAsyncShutdownClient **_shutdownClient)
 {
   NS_ENSURE_ARG_POINTER(_shutdownClient);
-  RefPtr<nsIAsyncShutdownClient> client = mDB->GetClientsShutdown();
-  MOZ_ASSERT(client);
+  nsCOMPtr<nsIAsyncShutdownClient> client = mDB->GetClientsShutdown();
+  if (!client) {
+    return NS_ERROR_UNEXPECTED;
+  }
   client.forget(_shutdownClient);
+  return NS_OK;
+}
 
+NS_IMETHODIMP
+nsNavHistory::GetConnectionShutdownClient(nsIAsyncShutdownClient **_shutdownClient)
+{
+  NS_ENSURE_ARG_POINTER(_shutdownClient);
+  nsCOMPtr<nsIAsyncShutdownClient> client = mDB->GetConnectionShutdown();
+  if (!client) {
+    return NS_ERROR_UNEXPECTED;
+  }
+  client.forget(_shutdownClient);
   return NS_OK;
 }
 

toolkit/components/places/nsPIPlacesDatabase.idl

@@ -47,6 +47,14 @@ interface nsPIPlacesDatabase : nsISupports
   /**
    * Hook for clients who need to perform actions during/by the end of
    * the shutdown of the database.
+   * May be null if it's too late to get one.
    */
   readonly attribute nsIAsyncShutdownClient shutdownClient;
+
+  /**
+   * Hook for internal clients who need to perform actions just before the
+   * connection gets closed.
+   * May be null if it's too late to get one.
+   */
+  readonly attribute nsIAsyncShutdownClient connectionShutdownClient;
 };

toolkit/components/places/nsPlacesExpiration.js

@@ -32,7 +32,6 @@ XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
 // Constants
 
 // Last expiration step should run before the final sync.
-const TOPIC_SHUTDOWN = "places-will-close-connection";
 const TOPIC_PREF_CHANGED = "nsPref:changed";
 const TOPIC_DEBUG_START_EXPIRATION = "places-debug-start-expiration";
 const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished";
@@ -433,37 +432,37 @@ function nsPlacesExpiration() {
   }, Cu.reportError);
 
   // Register topic observers.
-  Services.obs.addObserver(this, TOPIC_SHUTDOWN, true);
   Services.obs.addObserver(this, TOPIC_DEBUG_START_EXPIRATION, true);
   Services.obs.addObserver(this, TOPIC_IDLE_DAILY, true);
+
+  // Block shutdown.
+  let shutdownClient = PlacesUtils.history.connectionShutdownClient.jsclient;
+  shutdownClient.addBlocker("Places Expiration: shutdown", () => {
+    if (this._shuttingDown) {
+      return;
+    }
+    this._shuttingDown = true;
+    this.expireOnIdle = false;
+    if (this._timer) {
+      this._timer.cancel();
+      this._timer = null;
+    }
+    // If the database is dirty, we want to expire some entries, to speed up
+    // the expiration process.
+    if (this.status == STATUS.DIRTY) {
+      this._expireWithActionAndLimit(ACTION.SHUTDOWN_DIRTY, LIMIT.LARGE);
+    }
+    this._finalizeInternalStatements();
+  });
 }
 
 nsPlacesExpiration.prototype = {
-
-  // nsIObserver
-
   observe: function PEX_observe(aSubject, aTopic, aData) {
     if (this._shuttingDown) {
       return;
     }
 
-    if (aTopic == TOPIC_SHUTDOWN) {
-      this._shuttingDown = true;
-      this.expireOnIdle = false;
-
-      if (this._timer) {
-        this._timer.cancel();
-        this._timer = null;
-      }
-
-      // If the database is dirty, we want to expire some entries, to speed up
-      // the expiration process.
-      if (this.status == STATUS.DIRTY) {
-        this._expireWithActionAndLimit(ACTION.SHUTDOWN_DIRTY, LIMIT.LARGE);
-      }
-
-      this._finalizeInternalStatements();
-    } else if (aTopic == TOPIC_PREF_CHANGED) {
+    if (aTopic == TOPIC_PREF_CHANGED) {
       this._loadPrefsPromise = this._loadPrefs().then(() => {
         if (aData == PREF_INTERVAL_SECONDS) {
           // Renew the timer with the new interval value.

toolkit/components/places/tests/bookmarks/test_async_observers.js

@@ -91,28 +91,20 @@ add_task(async function shutdown() {
   // 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
+  // event as a shutdown blocker 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.
-  await new Promise(resolve => {
-    Services.obs.addObserver(function onNotification() {
-      Services.obs.removeObserver(onNotification, "places-will-close-connection");
-      do_check_true(true, "Observed fake places shutdown");
 
-      Services.tm.dispatchToMainThread(() => {
+  let shutdownClient = PlacesUtils.history.shutdownClient.jsclient;
+  shutdownClient.addBlocker("Places Expiration: shutdown",
+    function() {
+      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");
-        resolve(promiseTopicObserved("places-connection-closed"));
-      });
-    }, "places-will-close-connection");
-    shutdownPlaces();
-  });
+      }, Ci.nsIThread.DISPATCH_NORMAL);
+    });
 });
-
-
-
-

toolkit/components/places/tests/expiration/head_expiration.js

@@ -21,14 +21,6 @@ Cu.import("resource://gre/modules/Services.jsm");
 
 // Put any other stuff relative to this test folder below.
 
-
-// Simulates an expiration at shutdown.
-function shutdownExpiration() {
-  let expire = Cc["@mozilla.org/places/expiration;1"].getService(Ci.nsIObserver);
-  expire.observe(null, "places-will-close-connection", null);
-}
-
-
 /**
  * Causes expiration component to start, otherwise it would wait for the first
  * history notification.