Bug 1541934 - mozStorageConnection can crash on shutdown, r=asuth

Differential Revision: https://phabricator.services.mozilla.com/D26491

--HG--
extra : moz-landing-system : lando

extensions/cookie/nsPermissionManager.cpp

@@ -50,6 +50,10 @@
 
 static mozilla::StaticRefPtr<nsPermissionManager> gPermissionManager;
 
+// Initially, |false|. Set to |true| once shutdown has started, to avoid
+// reopening the database.
+static bool gIsShuttingDown = false;
+
 using namespace mozilla;
 using namespace mozilla::dom;
 
@@ -285,27 +289,6 @@ already_AddRefed<nsIPrincipal> GetNextSubDomainPrincipal(
   return principal.forget();
 }
 
-class ClearOriginDataObserver final : public nsIObserver {
-  ~ClearOriginDataObserver() {}
-
- public:
-  NS_DECL_ISUPPORTS
-
-  // nsIObserver implementation.
-  NS_IMETHOD
-  Observe(nsISupports* aSubject, const char* aTopic,
-          const char16_t* aData) override {
-    MOZ_ASSERT(!nsCRT::strcmp(aTopic, "clear-origin-attributes-data"));
-
-    nsCOMPtr<nsIPermissionManager> permManager =
-        do_GetService("@mozilla.org/permissionmanager;1");
-    return permManager->RemovePermissionsWithAttributes(
-        nsDependentString(aData));
-  }
-};
-
-NS_IMPL_ISUPPORTS(ClearOriginDataObserver, nsIObserver)
-
 class MOZ_STACK_CLASS UpgradeHostToOriginHelper {
  public:
   virtual nsresult Insert(const nsACString& aOrigin, const nsCString& aType,
@@ -838,7 +821,7 @@ NS_IMETHODIMP
 CloseDatabaseListener::Complete(nsresult, nsISupports*) {
   // Help breaking cycles
   RefPtr<nsPermissionManager> manager = mManager.forget();
-  if (mRebuildOnSuccess && !manager->mIsShuttingDown) {
+  if (mRebuildOnSuccess && !gIsShuttingDown) {
     return manager->InitDB(true);
   }
   return NS_OK;
@@ -897,12 +880,9 @@ NS_IMETHODIMP DeleteFromMozHostListener::HandleCompletion(uint16_t aReason) {
 }
 
 /* static */
-void nsPermissionManager::ClearOriginDataObserverInit() {
-  nsCOMPtr<nsIObserverService> observerService =
-      mozilla::services::GetObserverService();
-  observerService->AddObserver(new ClearOriginDataObserver(),
-                               "clear-origin-attributes-data",
-                               /* ownsWeak= */ false);
+void nsPermissionManager::Startup() {
+  nsCOMPtr<nsIPermissionManager> permManager =
+      do_GetService("@mozilla.org/permissionmanager;1");
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -923,7 +903,7 @@ NS_IMPL_ISUPPORTS(nsPermissionManager, nsIPermissionManager, nsIObserver,
                   nsISupportsWeakReference)
 
 nsPermissionManager::nsPermissionManager()
-    : mMemoryOnlyDB(false), mLargestID(0), mIsShuttingDown(false) {}
+    : mMemoryOnlyDB(false), mLargestID(0) {}
 
 nsPermissionManager::~nsPermissionManager() {
   // NOTE: Make sure to reject each of the promises in mPermissionKeyPromiseMap
@@ -949,6 +929,10 @@ nsPermissionManager::GetXPCOMSingleton() {
     return do_AddRef(gPermissionManager);
   }
 
+  if (gIsShuttingDown) {
+    return nullptr;
+  }
+
   // Create a new singleton nsPermissionManager.
   // We AddRef only once since XPCOM has rules about the ordering of module
   // teardowns - by the time our module destructor is called, it's too late to
@@ -1002,6 +986,9 @@ nsresult nsPermissionManager::Init() {
   if (observerService) {
     observerService->AddObserver(this, "profile-before-change", true);
     observerService->AddObserver(this, "profile-do-change", true);
+    observerService->AddObserver(this, "testonly-reload-permissions-from-disk",
+                                 true);
+    observerService->AddObserver(this, "clear-origin-attributes-data", true);
   }
 
   // ignore failure here, since it's non-fatal (we can run fine without
@@ -2687,12 +2674,25 @@ NS_IMETHODIMP nsPermissionManager::Observe(nsISupports* aSubject,
   if (!nsCRT::strcmp(aTopic, "profile-before-change")) {
     // The profile is about to change,
     // or is going away because the application is shutting down.
-    mIsShuttingDown = true;
+    gIsShuttingDown = true;
     RemoveAllFromMemory();
     CloseDB(false);
   } else if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
     // the profile has already changed; init the db from the new location
     InitDB(false);
+  } else if (!nsCRT::strcmp(aTopic, "testonly-reload-permissions-from-disk")) {
+    // Testing mechanism to reload all permissions from disk. Because the
+    // permission manager automatically initializes itself at startup, tests
+    // that directly manipulate the permissions database need some way to reload
+    // the database for their changes to have any effect. This mechanism was
+    // introduced when moving the permissions manager from on-demand startup to
+    // always being initialized. This is not guarded by a pref because it's not
+    // dangerous to reload permissions from disk, just bad for performance.
+    RemoveAllFromMemory();
+    CloseDB(false);
+    InitDB(false);
+  } else if (!nsCRT::strcmp(aTopic, "clear-origin-attributes-data")) {
+    return RemovePermissionsWithAttributes(nsDependentString(someData));
   }
 
   return NS_OK;

extensions/cookie/nsPermissionManager.h

@@ -193,12 +193,13 @@ class nsPermissionManager final : public nsIPermissionManager,
   }
 
   /**
-   * Initialize the "clear-origin-attributes-data" observing.
-   * Will create a nsPermissionManager instance if needed.
-   * That way, we can prevent have nsPermissionManager created at startup just
-   * to be able to clear data when an application is uninstalled.
+   * Initialize the permission-manager service.
+   * The permission manager is always initialized at startup because when it was
+   * lazy-initialized on demand, it was possible for it to be created once
+   * shutdown had begun, resulting in the manager failing to correctly shutdown
+   * because it missed its shutdown observer notification.
    */
-  static void ClearOriginDataObserverInit();
+  static void Startup();
 
   nsresult RemovePermissionsWithAttributes(
       mozilla::OriginAttributesPattern& aAttrs);
@@ -518,10 +519,6 @@ class nsPermissionManager final : public nsIPermissionManager,
   // a unique, monotonically increasing id used to identify each database entry
   int64_t mLargestID;
 
-  // Initially, |false|. Set to |true| once shutdown has started, to avoid
-  // reopening the database.
-  bool mIsShuttingDown;
-
   nsCOMPtr<nsIPrefBranch> mDefaultPrefBranch;
 
   // NOTE: Ensure this is the last member since it has a large inline buffer.

extensions/cookie/test/unit/test_permmanager_defaults.js

@@ -45,7 +45,9 @@ add_task(async function do_test() {
   // Set the preference used by the permission manager so the file is read.
   Services.prefs.setCharPref("permissions.manager.defaultsUrl", "file://" + file.path);
 
-  // initialize the permission manager service - it will read that default.
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   let pm = Cc["@mozilla.org/permissionmanager;1"].
            getService(Ci.nsIPermissionManager);
 

extensions/cookie/test/unit/test_permmanager_load_invalid_entries.js

@@ -19,6 +19,8 @@ function run_test() {
   Assert.ok(file.exists());
 
   connection.schemaVersion = 3;
+  connection.executeSimpleSQL(
+    "DROP TABLE moz_hosts");
   connection.executeSimpleSQL(
     "CREATE TABLE moz_hosts (" +
       " id INTEGER PRIMARY KEY" +
@@ -113,6 +115,9 @@ function run_test() {
     );
   }
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   let earliestNow = Number(Date.now());
   // Initialize the permission manager service
   var pm = Cc["@mozilla.org/permissionmanager;1"]

extensions/cookie/test/unit/test_permmanager_migrate_4-7.js

@@ -20,6 +20,8 @@ add_task(async function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 4;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   db.executeSimpleSQL(
     "CREATE TABLE moz_hosts (" +
@@ -159,6 +161,9 @@ add_task(async function test() {
   await PlacesTestUtils.addVisits(Services.io.newURI("https://foo.com/some/other/subdirectory"));
   await PlacesTestUtils.addVisits(Services.io.newURI("ftp://some.subdomain.of.foo.com:8000/some/subdirectory"));
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_4-7_no_history.js

@@ -56,6 +56,8 @@ add_task(function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 4;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   db.executeSimpleSQL(
     "CREATE TABLE moz_hosts (" +
@@ -175,6 +177,9 @@ add_task(function test() {
 
   let found = expected.map((it) => 0);
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_5-7a.js

@@ -20,6 +20,8 @@ add_task(async function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 5;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   /*
    * V5 table
@@ -218,6 +220,9 @@ add_task(async function test() {
   await PlacesTestUtils.addVisits(Services.io.newURI("https://foo.com/some/other/subdirectory"));
   await PlacesTestUtils.addVisits(Services.io.newURI("ftp://some.subdomain.of.foo.com:8000/some/subdirectory"));
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_5-7b.js

@@ -20,6 +20,8 @@ add_task(function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 5;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   /*
    * V5 table
@@ -100,6 +102,9 @@ add_task(function test() {
 
   let found = expected.map((it) => 0);
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_6-7a.js

@@ -20,6 +20,8 @@ add_task(async function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 6;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   /*
    * V5 table
@@ -218,6 +220,9 @@ add_task(async function test() {
   await PlacesTestUtils.addVisits(Services.io.newURI("https://foo.com/some/other/subdirectory"));
   await PlacesTestUtils.addVisits(Services.io.newURI("ftp://some.subdomain.of.foo.com:8000/some/subdirectory"));
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_6-7b.js

@@ -20,6 +20,8 @@ add_task(function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 6;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   /*
    * V5 table
@@ -94,6 +96,9 @@ add_task(function test() {
 
   let found = expected.map((it) => 0);
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

extensions/cookie/test/unit/test_permmanager_migrate_7-8.js

@@ -20,6 +20,8 @@ add_task(async function test() {
 
   let db = Services.storage.openDatabase(GetPermissionsFile(profile));
   db.schemaVersion = 7;
+  db.executeSimpleSQL("DROP TABLE moz_perms");
+  db.executeSimpleSQL("DROP TABLE moz_hosts");
 
   /*
    * V5 table
@@ -198,6 +200,9 @@ add_task(async function test() {
   await PlacesTestUtils.addVisits(Services.io.newURI("ftp://127.0.0.1:8080"));
   await PlacesTestUtils.addVisits(Services.io.newURI("https://localhost:8080"));
 
+  // This will force the permission-manager to reload the data.
+  Services.obs.notifyObservers(null, "testonly-reload-permissions-from-disk", "");
+
   // Force initialization of the nsPermissionManager
   for (let permission of Services.perms.enumerator) {
     let isExpected = false;

layout/build/nsLayoutStatics.cpp

@@ -256,7 +256,8 @@ nsresult nsLayoutStatics::Initialize() {
 
   ProcessPriorityManager::Init();
 
-  nsPermissionManager::ClearOriginDataObserverInit();
+  nsPermissionManager::Startup();
+
   nsCookieService::AppClearDataObserverInit();
   nsApplicationCacheService::AppClearDataObserverInit();
 

storage/test/gtest/moz.build

@@ -11,7 +11,6 @@ UNIFIED_SOURCES += [
     'test_binding_params.cpp',
     'test_file_perms.cpp',
     'test_mutex.cpp',
-    'test_service_init_background_thread.cpp',
     'test_spinningSynchronousClose.cpp',
     'test_statement_scoper.cpp',
     'test_StatementCache.cpp',

storage/test/gtest/test_service_init_background_thread.cpp

@@ -1,56 +0,0 @@
-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim set:sw=2 ts=2 et lcs=trail\:.,tab\:>~ : */
-/* 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/. */
-
-#include "storage_test_harness.h"
-
-#include "nsThreadUtils.h"
-
-/**
- * This file tests that the storage service cannot be initialized off the main
- * thread.
- */
-
-////////////////////////////////////////////////////////////////////////////////
-//// Helpers
-
-class ServiceInitializer : public mozilla::Runnable {
- public:
-  ServiceInitializer() : mozilla::Runnable("ServiceInitializer") {}
-  NS_IMETHOD Run() override {
-    // Use an explicit do_GetService instead of getService so that the check in
-    // getService doesn't blow up.
-    nsCOMPtr<mozIStorageService> service =
-        do_GetService("@mozilla.org/storage/service;1");
-    do_check_false(service);
-    return NS_OK;
-  }
-};
-
-////////////////////////////////////////////////////////////////////////////////
-//// Test Functions
-
-// HACK ALERT: this test fails if it runs after any of the other storage tests
-// because the storage service will have been initialized and the
-// do_GetService() call in ServiceInitializer::Run will succeed. So we need a
-// way to force it to run before all the other storage gtests. As it happens,
-// gtest has a concept of "death tests" that, among other things, run before
-// all non-death tests. By merely using a "DeathTest" suffix here this becomes
-// a death test -- even though it doesn't use any of the normal death test
-// features -- which ensures this is the first storage test to run.
-TEST(storage_service_init_background_thread_DeathTest, Test)
-{
-  nsCOMPtr<nsIRunnable> event = new ServiceInitializer();
-  do_check_true(event);
-
-  nsCOMPtr<nsIThread> thread;
-  do_check_success(NS_NewNamedThread("StorageService", getter_AddRefs(thread)));
-
-  do_check_success(thread->Dispatch(event, NS_DISPATCH_NORMAL));
-
-  // Shutting down the thread will spin the event loop until all work in its
-  // event queue is completed.  This will act as our thread synchronization.
-  do_check_success(thread->Shutdown());
-}

toolkit/forgetaboutsite/test/unit/head_forgetaboutsite.js

@@ -9,14 +9,13 @@ var profileDir = do_get_profile();
 /**
  * Removes any files that could make our tests fail.
  */
-function cleanUp() {
+async function cleanUp() {
   const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 
   let files = [
     "places.sqlite",
     "cookies.sqlite",
     "signons.sqlite",
-    "permissions.sqlite",
   ];
 
   for (let i = 0; i < files.length; i++) {
@@ -25,5 +24,9 @@ function cleanUp() {
     if (file.exists())
       file.remove(false);
   }
+
+  await new Promise(resolve => {
+    Services.clearData.deleteData(Ci.nsIClearDataService.CLEAR_PERMISSIONS, value => resolve());
+  });
 }
 cleanUp();