Bug 1338280 - Add `AsyncShutdown` phase tags; use tags to shut down wrapped Places connections.
A tag is a unique ID passed as the `data` parameter to a shutdown
notification. This makes it possible to restart Places multiple times,
without invalidating the blocker after the first shutdown.
MozReview-Commit-ID: Cos072ZDUAO
--- a/toolkit/components/asyncshutdown/AsyncShutdown.jsm
+++ b/toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ -368,28 +368,39 @@ this.AsyncShutdown = {
}
if (accepted) {
return getPhase;
}
return undefined;
}
};
+function makePhaseKey(topic, tag) {
+ if (tag === null) {
+ return topic;
+ }
+ if (typeof tag != "string") {
+ throw new TypeError(`Tag ${tag} must be a string`);
+ }
+ return topic + "|" + tag;
+}
+
/**
* Register a new phase.
*
* @param {string} topic The notification topic for this Phase.
* @see {https://developer.mozilla.org/en-US/docs/Observer_Notifications}
*/
-function getPhase(topic) {
- let phase = gPhases.get(topic);
+function getPhase(topic, tag = null) {
+ let key = makePhaseKey(topic, tag);
+ let phase = gPhases.get(key);
if (phase) {
return phase;
}
- let spinner = new Spinner(topic);
+ let spinner = new Spinner(topic, tag);
phase = Object.freeze({
/**
* Register a blocker for the completion of a phase.
*
* @param {string} name The human-readable name of the blocker. Used
* for debugging/error reporting. Please make sure that the name
* respects the following model: "Some Service: some action in progress" -
* for instance "OS.File: flushing all pending I/O";
@@ -471,29 +482,32 @@ function getPhase(topic) {
// Ignore errors
}
if (accepted) {
return () => spinner.observe();
}
return undefined;
}
});
- gPhases.set(topic, phase);
+ gPhases.set(key, phase);
return phase;
}
/**
* Utility class used to spin the event loop until all blockers for a
* Phase are satisfied.
*
* @param {string} topic The xpcom notification for that phase.
+ * @param {string} [tag] Tag for this phase, passed as the `data` parameter to
+ * the observer.
*/
-function Spinner(topic) {
+function Spinner(topic, tag = null) {
this._barrier = new Barrier(topic);
this._topic = topic;
+ this._tag = tag;
Services.obs.addObserver(this, topic, false);
}
Spinner.prototype = {
/**
* Register a new condition for this phase.
*
* See the documentation of `addBlocker` in property `client`
@@ -517,18 +531,20 @@ Spinner.prototype = {
return this._barrier.client.removeBlocker(condition);
},
get name() {
return this._barrier.client.name;
},
// nsIObserver.observe
- observe() {
- let topic = this._topic;
+ observe(subject, topic, data) {
+ if (data !== this._tag) {
+ return;
+ }
debug(`Starting phase ${ topic }`);
Services.obs.removeObserver(this, topic);
let satisfied = false; // |true| once we have satisfied all conditions
let promise;
try {
promise = this._barrier.wait({
warnAfterMS: DELAY_WARNING_MS,
@@ -1014,17 +1030,18 @@ 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.getPlacesClosingInternalConnectionPhase =
+ dbTag => getPhase("places-will-close-connection", dbTag);
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/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -330,16 +330,21 @@ this.PlacesUtils = {
TOPIC_EXPIRATION_FINISHED: "places-expiration-finished",
TOPIC_FEEDBACK_UPDATED: "places-autocomplete-feedback-updated",
TOPIC_FAVICONS_EXPIRED: "places-favicons-expired",
TOPIC_VACUUM_STARTING: "places-vacuum-starting",
TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin",
TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success",
TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed",
+ PREF_DATABASE_TESTING_ENABLED: "places.database.testing.enabled",
+
+ _asyncDBConnPromised: null,
+ _asyncDBWrapperPromised: null,
+
asContainer: aNode => asContainer(aNode),
asQuery: aNode => asQuery(aNode),
endl: NEWLINE,
/**
* Makes a URI from a spec.
* @param aSpec
@@ -1398,31 +1403,61 @@ this.PlacesUtils = {
if (!wasOpen) {
root.containerOpen = false;
if (!didSuppressNotifications)
result.suppressNotifications = false;
}
return urls;
},
+ // Indicates whether testing conveniences are enabled. If true, the wrapped
+ // database connection will be reset on shutdown, allowing tests to completely
+ // shut down and restart Places.
+ _isDBTestingEnabled() {
+ let testingEnabled = false;
+ try {
+ testingEnabled =
+ Services.prefs.getBoolPref(this.PREF_DATABASE_TESTING_ENABLED);
+ } catch (ex) {}
+ return testingEnabled;
+ },
+
/**
* Gets a shared Sqlite.jsm readonly connection to the Places database,
* usable only for SELECT queries.
*
* This is intended to be used mostly internally, components outside of
* Places should, when possible, use API calls and file bugs to get proper
* APIs, where they are missing.
* Keep in mind the Places DB schema is by no means frozen or even stable.
* Your custom queries can - and will - break overtime.
*
* Example:
* let db = yield PlacesUtils.promiseDBConnection();
* let rows = yield db.executeCached(sql, params);
*/
- promiseDBConnection: () => gAsyncDBConnPromised,
+ promiseDBConnection() {
+ if (!this._asyncDBConnPromised) {
+ // Check if testing is enabled before registering the shutdown blocker,
+ // for consistency with the C++ database code.
+ let testingEnabled = this._isDBTestingEnabled();
+ this._asyncDBConnPromised = Sqlite.cloneStorageConnection({
+ connection: PlacesUtils.history.DBConnection,
+ readOnly: true
+ }).then(conn => {
+ setupDbForShutdown(conn, PlacesUtils.history.DBTag, "PlacesUtils read-only connection", () => {
+ if (testingEnabled) {
+ this._asyncDBConnPromised = null;
+ }
+ });
+ return conn;
+ }).catch(Cu.reportError);
+ }
+ return this._asyncDBConnPromised;
+ },
/**
* Performs a read/write operation on the Places database through a Sqlite.jsm
* wrapped connection to the Places database.
*
* This is intended to be used only by Places itself, always use APIs if you
* need to modify the Places database. Use promiseDBConnection if you need to
* SELECT from the database and there's no covering API.
@@ -1441,22 +1476,34 @@ this.PlacesUtils = {
* }));
*
* @param {string} name The name of the operation. Used for debugging, logging
* and crash reporting.
* @param {function(db)} task A function that takes as argument a Sqlite.jsm
* connection and returns a Promise. Shutdown is guaranteed to not interrupt
* execution of `task`.
*/
- withConnectionWrapper: (name, task) => {
+ withConnectionWrapper(name, task) {
if (!name) {
throw new TypeError("Expecting a user-readable name");
}
- return Task.spawn(function*() {
- let db = yield gAsyncDBWrapperPromised;
+ if (!this._asyncDBWrapperPromised) {
+ let testingEnabled = this._isDBTestingEnabled();
+ this._asyncDBWrapperPromised = Sqlite.wrapStorageConnection({
+ connection: PlacesUtils.history.DBConnection,
+ }).then(conn => {
+ setupDbForShutdown(conn, PlacesUtils.history.DBTag, "PlacesUtils wrapped connection", () => {
+ if (testingEnabled) {
+ this._asyncDBWrapperPromised = null;
+ }
+ });
+ return conn;
+ }).catch(Cu.reportError);
+ }
+ return this._asyncDBWrapperPromised.then(function(db) {
return db.executeBeforeShutdown(name, task);
});
},
/**
* Given a uri returns list of itemIds associated to it.
*
* @param aURI
@@ -2054,76 +2101,61 @@ XPCOMUtils.defineLazyGetter(this, "bundl
*
* 1. Places initiates shutdown.
* 2. Before places can move to the step where it closes the low-level connection,
* we need to make sure that we have closed `conn`.
* 3. Before we can close `conn`, we need to make sure that all external clients
* have stopped using `conn`.
* 4. Before we can close Sqlite, we need to close `conn`.
*/
-function setupDbForShutdown(conn, name) {
+function setupDbForShutdown(conn, dbTag, name, afterShutdown) {
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`,
+ let phase = AsyncShutdown.getPlacesClosingInternalConnectionPhase(dbTag);
+ phase.addBlocker(`${name} closing as part of ${dbTag} shutdown`,
Task.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.
yield conn.close();
state = "2. Closed Sqlite.jsm connection.";
+ afterShutdown();
resolve();
}),
() => state
);
} catch (ex) {
// It's too late to block shutdown, just close the connection.
conn.close();
+ afterShutdown();
reject(ex);
}
});
// Make sure that Sqlite.jsm doesn't close until we are done
// with the high-level connection.
Sqlite.shutdown.addBlocker(`${name} must be closed before Sqlite.jsm`,
() => promiseClosed.catch(Cu.reportError),
() => state
);
} catch (ex) {
// It's too late to block shutdown, just close the connection.
conn.close();
+ afterShutdown();
throw ex;
}
}
-XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised",
- () => Sqlite.cloneStorageConnection({
- connection: PlacesUtils.history.DBConnection,
- readOnly: true
- }).then(conn => {
- setupDbForShutdown(conn, "PlacesUtils read-only connection");
- return conn;
- }).catch(Cu.reportError)
-);
-
-XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised",
- () => Sqlite.wrapStorageConnection({
- connection: PlacesUtils.history.DBConnection,
- }).then(conn => {
- setupDbForShutdown(conn, "PlacesUtils wrapped connection");
- return conn;
- }).catch(Cu.reportError)
-);
-
/**
* Keywords management API.
* Sooner or later these keywords will merge with search keywords, this is an
* interim API that should then be replaced by a unified one.
* Keywords are associated with URLs and can have POST data.
* A single URL can have multiple keywords, provided they differ by POST data.
*/
var Keywords = {