Bug 1284083 - Fix a race condition in nsPlacesExpiration that may cause intermittent failures. r=adw
MozReview-Commit-ID: GxLkokwrxuA
--- a/toolkit/components/places/nsPlacesExpiration.js
+++ b/toolkit/components/places/nsPlacesExpiration.js
@@ -492,17 +492,17 @@ function nsPlacesExpiration() {
XPCOMUtils.defineLazyServiceGetter(this, "_idle",
"@mozilla.org/widget/idleservice;1",
"nsIIdleService");
this._prefBranch = Cc["@mozilla.org/preferences-service;1"].
getService(Ci.nsIPrefService).
getBranch(PREF_BRANCH);
- this._loadPrefs().then(() => {
+ this._loadPrefsPromise = this._loadPrefs().then(() => {
// Observe our preferences branch for changes.
this._prefBranch.addObserver("", this, true);
// Create our expiration timer.
this._newTimer();
}, Cu.reportError);
// Register topic observers.
@@ -535,17 +535,17 @@ nsPlacesExpiration.prototype = {
Date.now() - this._lastClearHistoryTime <
SHUTDOWN_WITH_RECENT_CLEARHISTORY_TIMEOUT_SECONDS * 1000;
if (!hasRecentClearHistory && this.status == STATUS.DIRTY) {
this._expireWithActionAndLimit(ACTION.SHUTDOWN_DIRTY, LIMIT.LARGE);
}
this._finalizeInternalStatements();
} else if (aTopic == TOPIC_PREF_CHANGED) {
- this._loadPrefs().then(() => {
+ 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
// history is over capacity. Mind to correctly handle the NaN value.
@@ -700,17 +700,16 @@ nsPlacesExpiration.prototype = {
Cu.reportError("Async statement execution returned with '" +
aError.result + "', '" + aError.message + "'");
},
// Number of expiration steps needed to reach a CLEAN status.
_telemetrySteps: 1,
handleCompletion: function PEX_handleCompletion(aReason) {
if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
-
if (this._mostRecentExpiredVisitDays) {
try {
Services.telemetry
.getHistogramById("PLACES_MOST_RECENT_EXPIRED_VISIT_DAYS")
.add(this._mostRecentExpiredVisitDays);
} catch (ex) {
Components.utils.reportError("Unable to report telemetry.");
} finally {
@@ -799,22 +798,21 @@ nsPlacesExpiration.prototype = {
get expireOnIdle() {
return this._expireOnIdle;
},
_loadPrefs: Task.async(function* () {
// Get the user's limit, if it was set.
this._urisLimit = this._prefBranch.getIntPref(PREF_MAX_URIS,
PREF_MAX_URIS_NOTSET);
-
if (this._urisLimit < 0) {
// Some testing code expects a pref change to be synchronous, so
// temporarily set this to a large value, while we asynchronously update
// to the correct value.
- this._urisLimit = 300000;
+ this._urisLimit = 100000;
// The user didn't specify a custom limit, so we calculate the number of
// unique places that may fit an optimal database size on this hardware.
// Oldest pages over this threshold will be expired.
let memSizeBytes = MEMSIZE_FALLBACK_BYTES;
try {
// Limit the size on systems with small memory.
memSizeBytes = Services.sysinfo.getProperty("memsize");
@@ -836,29 +834,40 @@ nsPlacesExpiration.prototype = {
let optimalDatabaseSize = Math.min(
memSizeBytes * DATABASE_TO_MEMORY_PERC / 100,
diskAvailableBytes * DATABASE_TO_DISK_PERC / 100,
DATABASE_MAX_SIZE
);
// Calculate avg size of a URI in the database.
- let db = yield PlacesUtils.promiseDBConnection();
- let pageSize = (yield db.execute(`PRAGMA page_size`))[0].getResultByIndex(0);
- let pageCount = (yield db.execute(`PRAGMA page_count`))[0].getResultByIndex(0);
- let freelistCount = (yield db.execute(`PRAGMA freelist_count`))[0].getResultByIndex(0);
- let dbSize = (pageCount - freelistCount) * pageSize;
- let uriCount = (yield db.execute(`SELECT count(*) FROM moz_places`))[0].getResultByIndex(0);
- let avgURISize = Math.ceil(dbSize / uriCount);
- // For new profiles this value may be too large, due to the Sqlite header,
- // or Infinity when there are no pages. Thus we must limit it.
- if (avgURISize > (URIENTRY_AVG_SIZE * 3)) {
- avgURISize = URIENTRY_AVG_SIZE;
+ let db;
+ try {
+ db = yield PlacesUtils.promiseDBConnection();
+ } catch (ex) {
+ // We may have been initialized late in the shutdown process, maybe
+ // by a call to clear history on shutdown.
+ // If we're unable to get a connection clone, we'll just proceed with
+ // the default value, it should not be critical at this point in the
+ // application life-cycle.
}
- this._urisLimit = Math.ceil(optimalDatabaseSize / avgURISize);
+ if (db) {
+ let pageSize = (yield db.execute(`PRAGMA page_size`))[0].getResultByIndex(0);
+ let pageCount = (yield db.execute(`PRAGMA page_count`))[0].getResultByIndex(0);
+ let freelistCount = (yield db.execute(`PRAGMA freelist_count`))[0].getResultByIndex(0);
+ let dbSize = (pageCount - freelistCount) * pageSize;
+ let uriCount = (yield db.execute(`SELECT count(*) FROM moz_places`))[0].getResultByIndex(0);
+ let avgURISize = Math.ceil(dbSize / uriCount);
+ // For new profiles this value may be too large, due to the Sqlite header,
+ // or Infinity when there are no pages. Thus we must limit it.
+ if (avgURISize > (URIENTRY_AVG_SIZE * 3)) {
+ avgURISize = URIENTRY_AVG_SIZE;
+ }
+ this._urisLimit = Math.ceil(optimalDatabaseSize / avgURISize);
+ }
}
// Expose the calculated limit to other components.
this._prefBranch.setIntPref(PREF_READONLY_CALCULATED_MAX_URIS,
this._urisLimit);
// Get the expiration interval value.
this._interval = this._prefBranch.getIntPref(PREF_INTERVAL_SECONDS,
@@ -909,32 +918,42 @@ nsPlacesExpiration.prototype = {
* @param aAction
* The ACTION we are expiring for. See the ACTION const for values.
* @param aLimit
* Whether to use small, large or no limits when expiring. See the
* LIMIT const for values.
*/
_expireWithActionAndLimit:
function PEX__expireWithActionAndLimit(aAction, aLimit) {
- // Skip expiration during batch mode.
- if (this._inBatchMode)
- return;
- // Don't try to further expire after shutdown.
- if (this._shuttingDown && aAction != ACTION.SHUTDOWN_DIRTY) {
- return;
- }
+ Task.spawn(function*() {
+ // Ensure that we'll run statements with the most up-to-date pref values.
+ // On shutdown we cannot do this, since we must enqueue the expiration
+ // statements synchronously before the connection goes away.
+ // TODO (Bug 1275878): handle this properly through a shutdown blocker.
+ if (!this._shuttingDown)
+ yield this._loadPrefsPromise;
- let boundStatements = [];
- for (let queryType in EXPIRATION_QUERIES) {
- if (EXPIRATION_QUERIES[queryType].actions & aAction)
- boundStatements.push(this._getBoundStatement(queryType, aLimit, aAction));
- }
+ // Skip expiration during batch mode.
+ if (this._inBatchMode)
+ return;
+ // Don't try to further expire after shutdown.
+ if (this._shuttingDown && aAction != ACTION.SHUTDOWN_DIRTY) {
+ return;
+ }
- // Execute statements asynchronously in a transaction.
- this._db.executeAsync(boundStatements, boundStatements.length, this);
+ let boundStatements = [];
+ for (let queryType in EXPIRATION_QUERIES) {
+ if (EXPIRATION_QUERIES[queryType].actions & aAction)
+ boundStatements.push(this._getBoundStatement(queryType, aLimit, aAction));
+ }
+
+
+ // Execute statements asynchronously in a transaction.
+ this._db.executeAsync(boundStatements, boundStatements.length, this);
+ }.bind(this)).catch(Cu.reportError);
},
/**
* Finalizes all of our mozIStorageStatements so we can properly close the
* database.
*/
_finalizeInternalStatements: function PEX__finalizeInternalStatements() {
for (let queryType in this._cachedStatements) {
--- a/toolkit/components/places/tests/expiration/test_clearHistory.js
+++ b/toolkit/components/places/tests/expiration/test_clearHistory.js
@@ -69,20 +69,16 @@ function add_old_anno(aIdentifier, aName
stmt.params.anno_name = aName;
try {
stmt.executeStep();
} finally {
stmt.finalize();
}
}
-function run_test() {
- run_next_test();
-}
-
add_task(function* test_historyClear() {
// Set interval to a large value so we don't expire on it.
setInterval(3600); // 1h
// Expire all expirable pages.
setMaxPages(0);
// Add some bookmarked page with visit and annotations.
@@ -121,17 +117,17 @@ add_task(function* test_historyClear() {
as.setPageAnnotation(pageURI, "expire", "test", 0, as.EXPIRE_NEVER);
as.setPageAnnotation(pageURI, "expire_session", "test", 0, as.EXPIRE_SESSION);
add_old_anno(pageURI, "expire_days", "test", as.EXPIRE_DAYS, 8);
add_old_anno(pageURI, "expire_weeks", "test", as.EXPIRE_WEEKS, 31);
add_old_anno(pageURI, "expire_months", "test", as.EXPIRE_MONTHS, 181);
}
// Expire all visits for the bookmarks
- yield PlacesUtils.history.clear();
+ yield PlacesTestUtils.clearHistory();
["expire_days", "expire_weeks", "expire_months", "expire_session",
"expire"].forEach(function(aAnno) {
let pages = as.getPagesWithAnnotation(aAnno);
do_check_eq(pages.length, 0);
});
["expire_days", "expire_weeks", "expire_months", "expire_session",
--- a/toolkit/components/places/tests/expiration/test_pref_maxpages.js
+++ b/toolkit/components/places/tests/expiration/test_pref_maxpages.js
@@ -9,19 +9,16 @@
*
* Expiration will obey to hardware spec, but user can set a custom maximum
* number of pages to retain, to restrict history, through
* "places.history.expiration.max_pages".
* This limit is used at next expiration run.
* If the pref is set to a number < 0 we will use the default value.
*/
-var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
- getService(Ci.nsINavHistoryService);
-
var tests = [
{ desc: "Set max_pages to a negative value, with 1 page.",
maxPages: -1,
addPages: 1,
expectedNotifications: 0, // Will ignore and won't expire anything.
},
@@ -55,20 +52,16 @@ var tests = [
{ desc: "Set max_pages to 10 with 10 pages.",
maxPages: 10,
addPages: 10,
expectedNotifications: 0, // We are below the limit, won't expire anything.
},
];
-function run_test() {
- run_next_test();
-}
-
add_task(function* test_pref_maxpages() {
// The pref should not exist by default.
try {
getMaxPages();
do_throw("interval pref should not exist by default");
} catch (ex) {}
// Set interval to a large value so we don't expire on it.
@@ -78,43 +71,36 @@ add_task(function* test_pref_maxpages()
let currentTest = tests[testIndex - 1];
print("\nTEST " + testIndex + ": " + currentTest.desc);
currentTest.receivedNotifications = 0;
// Setup visits.
let now = getExpirablePRTime();
for (let i = 0; i < currentTest.addPages; i++) {
let page = "http://" + testIndex + "." + i + ".mozilla.org/";
- yield PlacesTestUtils.addVisits({ uri: uri(page), visitDate: now++ });
+ yield PlacesTestUtils.addVisits({ uri: uri(page), visitDate: now-- });
}
// Observe history.
- let historyObserver = {
- onBeginUpdateBatch: function PEX_onBeginUpdateBatch() {},
- onEndUpdateBatch: function PEX_onEndUpdateBatch() {},
- onClearHistory() {},
- onVisit() {},
- onTitleChanged() {},
- onDeleteURI(aURI) {
- print("onDeleteURI " + aURI.spec);
- currentTest.receivedNotifications++;
- },
- onPageChanged() {},
- onDeleteVisits(aURI, aTime) {
- print("onDeleteVisits " + aURI.spec + " " + aTime);
- },
+ let historyObserver = new NavHistoryObserver();
+ historyObserver.onDeleteURI = aURI => {
+ print("onDeleteURI " + aURI.spec);
+ currentTest.receivedNotifications++;
};
- hs.addObserver(historyObserver);
+ historyObserver.onDeleteVisits = (aURI, aTime) => {
+ print("onDeleteVisits " + aURI.spec + " " + aTime);
+ };
+ PlacesUtils.history.addObserver(historyObserver);
setMaxPages(currentTest.maxPages);
// Expire now.
yield promiseForceExpirationStep(-1);
- hs.removeObserver(historyObserver, false);
+ PlacesUtils.history.removeObserver(historyObserver, false);
do_check_eq(currentTest.receivedNotifications,
currentTest.expectedNotifications);
// Clean up.
yield PlacesTestUtils.clearHistory();
}
--- a/toolkit/components/places/tests/expiration/xpcshell.ini
+++ b/toolkit/components/places/tests/expiration/xpcshell.ini
@@ -11,9 +11,8 @@ skip-if = toolkit == 'android'
[test_debug_expiration.js]
[test_idle_daily.js]
[test_notifications.js]
[test_notifications_onDeleteURI.js]
[test_notifications_onDeleteVisits.js]
[test_outdated_analyze.js]
[test_pref_interval.js]
[test_pref_maxpages.js]
-skip-if = (os == 'win' && debug) || (os == 'linux') # bug 1284083