Bug 1076597 - Fix Settings API shutdown race condition. r=bent
When child message manager dies, it sends a child-process-shutdown
message to the SettingsRequestManager. This would just close the locks
and tasks of this message manager. The race happens when some
applications can shutdown quickly: settings requests will never be
committed to the database. One example is the callscreen. The fix,
provided by qDot, simply put those tasks in a finalize state to make
sure they are properly executed and committed.
--- a/dom/settings/SettingsRequestManager.jsm
+++ b/dom/settings/SettingsRequestManager.jsm
@@ -699,17 +699,19 @@ let SettingsRequestManager = {
this.children.push(aMsgMgr);
this.mmPrincipals.set(aMsgMgr, aPrincipal);
}
},
removeObserver: function(aMsgMgr) {
if (DEBUG) {
let principal = this.mmPrincipals.get(aMsgMgr);
- debug("Remove observer for " + principal.origin);
+ if (principal) {
+ debug("Remove observer for " + principal.origin);
+ }
}
let index = this.children.indexOf(aMsgMgr);
if (index != -1) {
this.children.splice(index, 1);
this.mmPrincipals.delete(aMsgMgr);
}
if (DEBUG) debug("Principal/MessageManager pairs left: " + this.mmPrincipals.size);
},
@@ -738,30 +740,48 @@ let SettingsRequestManager = {
// If index is 0, the lock we just removed was at the head of
// the queue, so possibly queue the next lock if it's
// consumable.
if (index == 0) {
this.queueConsume();
}
},
- removeMessageManager: function(aMsgMgr){
+ removeMessageManager: function(aMsgMgr, aPrincipal) {
if (DEBUG) debug("Removing message manager");
+ let msgMgrPrincipal = this.mmPrincipals.get(aMsgMgr);
this.removeObserver(aMsgMgr);
- let closedLockIDs = [];
+
let lockIDs = Object.keys(this.lockInfo);
for (let i in lockIDs) {
- if (this.lockInfo[lockIDs[i]]._mm == aMsgMgr) {
- if (DEBUG) debug("Removing lock " + lockIDs[i] + " due to process close/crash");
- closedLockIDs.push(lockIDs[i]);
+ let lockId = lockIDs[i];
+ let lock = this.lockInfo[lockId];
+ if (lock._mm === aMsgMgr && msgMgrPrincipal === aPrincipal) {
+ let is_finalizing = false;
+ let task_index;
+ // Go in reverse order because finalize should be the last one
+ for (task_index = lock.tasks.length; task_index >= 0; task_index--) {
+ if (lock.tasks[task_index]
+ && lock.tasks[task_index].operation === "finalize") {
+ is_finalizing = true;
+ break;
+ }
+ }
+ if (!is_finalizing) {
+ this.queueTask("finalize", {lockID: lockId}, aPrincipal).then(
+ function() {
+ if (DEBUG) debug("Lock " + lockId + " with dead message manager finalized");
+ },
+ function(error) {
+ if (DEBUG) debug("Lock " + lockId + " with dead message manager NOT FINALIZED due to error: " + error);
+ }
+ );
+ }
}
}
- for (let i in closedLockIDs) {
- this.removeLock(closedLockIDs[i]);
- }
},
receiveMessage: function(aMessage) {
if (DEBUG) debug("receiveMessage " + aMessage.name);
let msg = aMessage.data;
let mm = aMessage.target;
@@ -807,17 +827,17 @@ let SettingsRequestManager = {
}
default:
break;
}
switch (aMessage.name) {
case "child-process-shutdown":
if (DEBUG) debug("Child process shutdown received.");
- this.removeMessageManager(mm);
+ this.removeMessageManager(mm, aMessage.principal);
break;
case "Settings:RegisterForMessages":
if (!SettingsPermissions.hasSomeReadPermission(aMessage.principal)) {
Cu.reportError("Settings message " + aMessage.name +
" from a content process with no 'settings-api-read' privileges.");
aMessage.target.assertPermission("message-manager-no-read-kill");
return;
}
--- a/dom/settings/moz.build
+++ b/dom/settings/moz.build
@@ -18,8 +18,10 @@ if CONFIG['MOZ_B2G']:
MOCHITEST_CHROME_MANIFESTS += ['tests/chrome.ini']
EXTRA_JS_MODULES += [
'SettingsDB.jsm',
'SettingsRequestManager.jsm'
]
MOCHITEST_MANIFESTS += ['tests/mochitest.ini']
+
+XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']
new file mode 100644
--- /dev/null
+++ b/dom/settings/tests/unit/test_settingsrequestmanager_messages.js
@@ -0,0 +1,154 @@
+"use strict";
+
+const Cu = Components.utils;
+
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
+
+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
+ "@mozilla.org/childprocessmessagemanager;1",
+ "nsIMessageSender");
+
+let principal = Services.scriptSecurityManager.getSystemPrincipal();
+let lockID = "{435d2192-4f21-48d4-90b7-285f147a56be}";
+
+// Helper to start the Settings Request Manager
+function startSettingsRequestManager() {
+ Cu.import("resource://gre/modules/SettingsRequestManager.jsm");
+}
+
+// Helper function to add a listener, send message and treat the reply
+function addAndSend(msg, reply, callback, payload, runNext = true) {
+ let handler = {
+ receiveMessage: function(message) {
+ if (message.name === reply) {
+ cpmm.removeMessageListener(reply, handler);
+ callback(message);
+ if (runNext) {
+ run_next_test();
+ }
+ }
+ }
+ };
+ cpmm.addMessageListener(reply, handler);
+ cpmm.sendAsyncMessage(msg, payload, undefined, principal);
+}
+
+// We need to trigger a Settings:Run message to make the queue progress
+function send_settingsRun() {
+ let msg = {lockID: lockID, isServiceLock: true};
+ cpmm.sendAsyncMessage("Settings:Run", msg, undefined, principal);
+}
+
+function kill_child() {
+ let msg = {lockID: lockID, isServiceLock: true};
+ cpmm.sendAsyncMessage("child-process-shutdown", msg, undefined, principal);
+}
+
+function run_test() {
+ do_get_profile();
+ startSettingsRequestManager();
+ run_next_test();
+}
+
+add_test(function test_createLock() {
+ let msg = {lockID: lockID, isServiceLock: true};
+ cpmm.sendAsyncMessage("Settings:CreateLock", msg, undefined, principal);
+ cpmm.sendAsyncMessage(
+ "Settings:RegisterForMessages", undefined, undefined, principal);
+ ok(true);
+ run_next_test();
+});
+
+add_test(function test_get_empty() {
+ let requestID = 10;
+ let msgReply = "Settings:Get:OK";
+ let msgHandler = function(message) {
+ equal(requestID, message.data.requestID);
+ equal(lockID, message.data.lockID);
+ ok(Object.keys(message.data.settings).length >= 0);
+ };
+
+ addAndSend("Settings:Get", msgReply, msgHandler, {
+ requestID: requestID,
+ lockID: lockID,
+ name: "language.current"
+ });
+
+ send_settingsRun();
+});
+
+add_test(function test_set_get_nonempty() {
+ let settings = { "language.current": "fr-FR:XPC" };
+ let requestIDSet = 20;
+ let msgReplySet = "Settings:Set:OK";
+ let msgHandlerSet = function(message) {
+ equal(requestIDSet, message.data.requestID);
+ equal(lockID, message.data.lockID);
+ };
+
+ addAndSend("Settings:Set", msgReplySet, msgHandlerSet, {
+ requestID: requestIDSet,
+ lockID: lockID,
+ settings: settings
+ }, false);
+
+ let requestIDGet = 25;
+ let msgReplyGet = "Settings:Get:OK";
+ let msgHandlerGet = function(message) {
+ equal(requestIDGet, message.data.requestID);
+ equal(lockID, message.data.lockID);
+ for(let p in settings) {
+ equal(settings[p], message.data.settings[p]);
+ }
+ };
+
+ addAndSend("Settings:Get", msgReplyGet, msgHandlerGet, {
+ requestID: requestIDGet,
+ lockID: lockID,
+ name: Object.keys(settings)[0]
+ });
+
+ // Set and Get have been push into the queue, let's run
+ send_settingsRun();
+});
+
+// This test exposes bug 1076597 behavior
+add_test(function test_wait_for_finalize() {
+ let settings = { "language.current": "en-US:XPC" };
+ let requestIDSet = 30;
+ let msgReplySet = "Settings:Set:OK";
+ let msgHandlerSet = function(message) {
+ equal(requestIDSet, message.data.requestID);
+ equal(lockID, message.data.lockID);
+ };
+
+ addAndSend("Settings:Set", msgReplySet, msgHandlerSet, {
+ requestID: requestIDSet,
+ lockID: lockID,
+ settings: settings
+ }, false);
+
+ let requestIDGet = 35;
+ let msgReplyGet = "Settings:Get:OK";
+ let msgHandlerGet = function(message) {
+ equal(requestIDGet, message.data.requestID);
+ equal(lockID, message.data.lockID);
+ for(let p in settings) {
+ equal(settings[p], message.data.settings[p]);
+ }
+ };
+
+ addAndSend("Settings:Get", msgReplyGet, msgHandlerGet, {
+ requestID: requestIDGet,
+ lockID: lockID,
+ name: Object.keys(settings)[0]
+ });
+
+ // We simulate a child death, which will force previous requests to be set
+ // into finalize state
+ kill_child();
+
+ // Then when we issue Settings:Run, those finalized should be triggered
+ send_settingsRun();
+});
new file mode 100644
--- /dev/null
+++ b/dom/settings/tests/unit/xpcshell.ini
@@ -0,0 +1,6 @@
+[DEFAULT]
+head =
+tail =
+
+[test_settingsrequestmanager_messages.js]
+skip-if = (toolkit == 'gonk' && debug) # bug 1080377: for some reason, this is not working on emulator-b2g debug build