Bug 1275878 - Part 2: Replace places-will-close-connection notification with a shutdown blocker. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 19 May 2016 23:50:27 +0200
changeset 614229 6c9dd729c2aad0b5870f0cbf1e565a33376c4f56
parent 614228 f257f99aff6eebc44fac46d0e2d2579dae50ace2
child 638816 0408adb0a76bc98fa83b7329e9db5a31153ddf83
push id69958
push usermak77@bonardo.net
push dateMon, 24 Jul 2017 09:35:11 +0000
reviewersadw
bugs1275878
milestone56.0a1
Bug 1275878 - Part 2: Replace places-will-close-connection notification with a shutdown blocker. r=adw MozReview-Commit-ID: A2sn2OreX4K
browser/components/places/tests/unit/test_clearHistory_shutdown.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
browser/components/privatebrowsing/test/browser/head.js
toolkit/components/asyncshutdown/AsyncShutdown.jsm
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/Shutdown.cpp
toolkit/components/places/Shutdown.h
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsPIPlacesDatabase.idl
toolkit/components/places/nsPlacesExpiration.js
toolkit/components/places/tests/bookmarks/test_async_observers.js
toolkit/components/places/tests/expiration/head_expiration.js
--- a/browser/components/places/tests/unit/test_clearHistory_shutdown.js
+++ b/browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ -14,17 +14,16 @@ const URIS = [
   "http://b.example2.com/",
   "http://c.example3.com/"
 ];
 
 const TOPIC_CONNECTION_CLOSED = "places-connection-closed";
 
 var EXPECTED_NOTIFICATIONS = [
   "places-shutdown",
-  "places-will-close-connection",
   "places-expiration-finished",
   "places-connection-closed"
 ];
 
 const UNEXPECTED_NOTIFICATIONS = [
   "xpcom-shutdown"
 ];
 
--- a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
@@ -1,72 +1,45 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // Test to make sure that the visited page titles do not get updated inside the
 // 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])
-      };
-
-      PlacesUtils.history.addObserver(historyObserver);
-    });
-  }
+  await PlacesUtils.history.clear();
 
-  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 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");
 
-  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() {}
-  });
-
-  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");
 });
 
--- a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
@@ -13,83 +13,53 @@ add_task(async function test() {
   function cleanup() {
     // delete all cookies
     cm.removeAll();
     // delete all history items
     return PlacesTestUtils.clearHistory();
   }
 
   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});
-
-  let private_tab = win2.gBrowser.addTab(TEST_URL);
-  win2.gBrowser.selectedTab = private_tab;
-  await BrowserTestUtils.browserLoaded(private_tab.linkedBrowser);
+  registerCleanupFunction(async () => {
+    let promisePBExit = TestUtils.topicObserved("last-pb-context-exited");
+    await BrowserTestUtils.closeWindow(win2);
+    await promisePBExit;
+  });
 
-  // 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");
 });
--- a/browser/components/privatebrowsing/test/browser/head.js
+++ b/browser/components/privatebrowsing/test/browser/head.js
@@ -1,14 +1,19 @@
 /* Any copyright is dedicated to the Public Domain.
  * 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);
   let focused = SimpleTest.promiseFocus(win);
   let startupFinished = TestUtils.topicObserved("browser-delayed-startup-finished",
                                                 subject => subject == win).then(() => win);
   Promise.all([focused, startupFinished])
     .then(results => executeSoon(() => aCallback(results[1])));
--- a/toolkit/components/asyncshutdown/AsyncShutdown.jsm
+++ b/toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ -998,17 +998,16 @@ Barrier.prototype = Object.freeze({
 // Ideally, phases should be registered from the component that decides
 // when they start/stop. For compatibility with existing startup/shutdown
 // mechanisms, we register a few phases here.
 
 // Parent process
 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");
 }
 
 // Notifications that fire in the parent and content process, but should
 // only have phases in the parent process.
 if (!isContent) {
   this.AsyncShutdown.quitApplicationGranted = getPhase("quit-application-granted");
 }
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -451,18 +451,27 @@ Database::GetStatement(const nsACString&
   // already.
   MOZ_ASSERT(mMainConn);
   return mAsyncThreadStatements.GetCachedStatement(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
 already_AddRefed<Database>
 Database::GetDatabase()
 {
   if (PlacesShutdownBlocker::IsStarted()) {
     return nullptr;
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -27,20 +27,16 @@
 // initial shutdown work and notifies TOPIC_PLACES_SHUTDOWN to all listeners.
 // Any shutdown work that requires the Places APIs should happen here.
 #define TOPIC_PROFILE_CHANGE_TEARDOWN "profile-change-teardown"
 // Fired when Places is shutting down.  Any code should stop accessing Places
 // APIs after this notification.  If you need to listen for Places shutdown
 // 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"
 
 // Simulate profile-before-change. This topic may only be used by
 // calling `observe` directly on the database. Used for testing only.
 #define TOPIC_SIMULATE_PLACES_SHUTDOWN "test-simulate-places-shutdown"
 
 class nsIRunnable;
@@ -82,16 +78,21 @@ public:
   nsresult Init();
 
   /**
    * The AsyncShutdown client used by clients of this API to be informed of shutdown.
    */
   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.
    *
    * @return Singleton instance of this class.
    */
   static already_AddRefed<Database> GetDatabase();
 
   /**
    * Actually initialized the connection on first need.
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -2137,17 +2137,18 @@ XPCOMUtils.defineLazyGetter(this, "bundl
 function setupDbForShutdown(conn, name) {
   try {
     let state = "0. Not started.";
     let promiseClosed = new Promise((resolve, reject) => {
       // The service initiates shutdown.
       // 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";
 
             // At this stage, all external clients have finished using the
             // database. We just need to close the high-level connection.
             await conn.close();
             state = "2. Closed Sqlite.jsm connection.";
 
--- a/toolkit/components/places/Shutdown.cpp
+++ b/toolkit/components/places/Shutdown.cpp
@@ -17,16 +17,29 @@ PlacesShutdownBlocker::PlacesShutdownBlo
   , mCounter(sCounter++)
 {
   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);
   }
+  // 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
 NS_IMETHODIMP
 PlacesShutdownBlocker::GetName(nsAString& aName)
 {
   aName = mName;
   return NS_OK;
@@ -66,60 +79,29 @@ PlacesShutdownBlocker::GetState(nsIPrope
   if (NS_WARN_IF(NS_FAILED(rv))) return rv;
   rv = static_cast<nsIWritablePropertyBag2*>(*_state)->SetPropertyAsInterface(
     NS_LITERAL_STRING("Barrier"), barrier);
   if (NS_WARN_IF(NS_FAILED(rv))) return rv;
 
   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) {
     MOZ_ALWAYS_SUCCEEDS(mBarrier->GetClient(getter_AddRefs(client)));
   }
   return client.forget();
 }
 
 // nsIAsyncShutdownBlocker
 NS_IMETHODIMP
-ClientsShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
+PlacesShutdownBlocker::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
 {
   MOZ_ASSERT(NS_IsMainThread());
   mParentClient = new nsMainThreadPtrHolder<nsIAsyncShutdownClient>(
     "ClientsShutdownBlocker::mParentClient", aParentClient);
   mState = RECEIVED_BLOCK_SHUTDOWN;
 
   if (NS_WARN_IF(!mBarrier)) {
     return NS_ERROR_NOT_AVAILABLE;
@@ -129,75 +111,89 @@ ClientsShutdownBlocker::BlockShutdown(ns
   MOZ_ALWAYS_SUCCEEDS(mBarrier->Wait(this));
 
   mState = CALLED_WAIT_CLIENTS;
   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()
 {
   // At this point all the clients are done, we can stop blocking the shutdown
   // phase.
   mState = RECEIVED_DONE;
 
   // mParentClient is nullptr in tests.
   if (mParentClient) {
     nsresult rv = mParentClient->RemoveBlocker(this);
     if (NS_WARN_IF(NS_FAILED(rv))) return rv;
     mParentClient = nullptr;
   }
   mBarrier = nullptr;
   return NS_OK;
 }
 
-NS_IMPL_ISUPPORTS_INHERITED(
+NS_IMPL_ISUPPORTS_INHERITED0(
   ClientsShutdownBlocker,
-  PlacesShutdownBlocker,
-  nsIAsyncShutdownCompletionCallback
+  PlacesShutdownBlocker
 )
 
 ////////////////////////////////////////////////////////////////////////////////
 
 ConnectionShutdownBlocker::ConnectionShutdownBlocker(Database* aDatabase)
   : PlacesShutdownBlocker(NS_LITERAL_STRING("Places Connection shutdown"))
   , mDatabase(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.
   MOZ_ASSERT(Database::gDatabase == nullptr || Database::gDatabase == mDatabase);
   Database::gDatabase = nullptr;
 
   // Database::Shutdown will invoke Complete once the connection is closed.
   mDatabase->Shutdown();
   mState = CALLED_STORAGESHUTDOWN;
+  mBarrier = nullptr;
   return NS_OK;
 }
 
 // mozIStorageCompletionCallback
 NS_IMETHODIMP
 ConnectionShutdownBlocker::Complete(nsresult, nsISupports*)
 {
   MOZ_ASSERT(NS_IsMainThread());
--- a/toolkit/components/places/Shutdown.h
+++ b/toolkit/components/places/Shutdown.h
@@ -38,38 +38,38 @@ class Database;
  * its `Done` method is invoked, and it stops blocking the shutdown phase, so
  * that it can continue.
  *
  * PHASE 3 (Connection shutdown)
  * ConnectionBlocker is registered as a profile-before-change blocker in
  * 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
  * ConnectionBlocker::Complete. At this point a final
  * places-connection-closed notification is sent, for testing purposes.
  */
 
 /**
  * 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.
    */
   static bool IsStarted() {
     return sIsStarted;
   }
 
@@ -121,45 +121,40 @@ protected:
 
   virtual ~PlacesShutdownBlocker() {}
 };
 
 /**
  * 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() {}
 };
 
 /**
  * Blocker used to wait when closing the database connection.
  */
 class ConnectionShutdownBlocker final : public PlacesShutdownBlocker
                                       , public mozIStorageCompletionCallback
 {
 public:
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_MOZISTORAGECOMPLETIONCALLBACK
 
-  NS_IMETHOD BlockShutdown(nsIAsyncShutdownClient* aParentClient) override;
+  explicit ConnectionShutdownBlocker(mozilla::places::Database* aDatabase);
 
-  explicit ConnectionShutdownBlocker(mozilla::places::Database* aDatabase);
+  NS_IMETHOD Done() override;
 
 private:
   ~ConnectionShutdownBlocker() {}
 
   // The owning database.
   // The cycle is broken in method Complete(), once the connection
   // has been closed by mozStorage.
   RefPtr<mozilla::places::Database> mDatabase;
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -2900,20 +2900,33 @@ nsNavHistory::GetDBConnection(mozIStorag
 
   return NS_OK;
 }
 
 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;
 }
 
 NS_IMETHODIMP
 nsNavHistory::AsyncExecuteLegacyQueries(nsINavHistoryQuery** aQueries,
                                         uint32_t aQueryCount,
                                         nsINavHistoryQueryOptions* aOptions,
                                         mozIStorageStatementCallback* aCallback,
--- a/toolkit/components/places/nsPIPlacesDatabase.idl
+++ b/toolkit/components/places/nsPIPlacesDatabase.idl
@@ -42,11 +42,19 @@ interface nsPIPlacesDatabase : nsISuppor
     [array, size_is(aQueryCount)] in nsINavHistoryQuery aQueries,
     in unsigned long aQueryCount,
     in nsINavHistoryQueryOptions aOptions,
     in mozIStorageStatementCallback aCallback);
 
   /**
    * 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;
 };
--- a/toolkit/components/places/nsPlacesExpiration.js
+++ b/toolkit/components/places/nsPlacesExpiration.js
@@ -27,17 +27,16 @@ Cu.import("resource://gre/modules/XPCOMU
 Cu.import("resource://gre/modules/Services.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
   "resource://gre/modules/PlacesUtils.jsm");
 
 // 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";
 const TOPIC_IDLE_BEGIN = "idle";
 const TOPIC_IDLE_END = "active";
 const TOPIC_IDLE_DAILY = "idle-daily";
 const TOPIC_TESTING_MODE = "testing-mode";
 const TOPIC_TEST_INTERVAL_CHANGED = "test-interval-changed";
@@ -428,47 +427,47 @@ function nsPlacesExpiration() {
     // Observe our preferences branch for changes.
     this._prefBranch.addObserver("", this, true);
 
     // Create our expiration timer.
     this._newTimer();
   }, 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.
           this._newTimer();
         }
       }, Cu.reportError);
     } else if (aTopic == TOPIC_DEBUG_START_EXPIRATION) {
       // The passed-in limit is the maximum number of visits to expire when
--- a/toolkit/components/places/tests/bookmarks/test_async_observers.js
+++ b/toolkit/components/places/tests/bookmarks/test_async_observers.js
@@ -86,33 +86,25 @@ add_task(async function test_remove_page
 
 add_task(async 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
+  // 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);
+    });
 });
-
-
-
-
--- a/toolkit/components/places/tests/expiration/head_expiration.js
+++ b/toolkit/components/places/tests/expiration/head_expiration.js
@@ -16,24 +16,16 @@ Cu.import("resource://gre/modules/Servic
   /* import-globals-from ../head_common.js */
   let commonFile = do_get_file("../head_common.js", false);
   let uri = Services.io.newFileURI(commonFile);
   Services.scriptloader.loadSubScript(uri.spec, this);
 }
 
 // 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.
  */
 function force_expiration_start() {
   Cc["@mozilla.org/places/expiration;1"]
     .getService(Ci.nsIObserver)
     .observe(null, "testing-mode", null);