From 8249ea276c3685c079ab043ab3eaece569036db5 Mon Sep 17 00:00:00 2001 From: Alexandre Lissy Date: Thu, 9 Oct 2014 10:51:34 +0200 Subject: [PATCH] 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. --- dom/settings/SettingsRequestManager.jsm | 40 +++-- dom/settings/moz.build | 2 + .../test_settingsrequestmanager_messages.js | 154 ++++++++++++++++++ dom/settings/tests/unit/xpcshell.ini | 6 + 4 files changed, 192 insertions(+), 10 deletions(-) create mode 100644 dom/settings/tests/unit/test_settingsrequestmanager_messages.js create mode 100644 dom/settings/tests/unit/xpcshell.ini diff --git a/dom/settings/SettingsRequestManager.jsm b/dom/settings/SettingsRequestManager.jsm index 21e7ac2028ba..575af149ef7c 100644 --- a/dom/settings/SettingsRequestManager.jsm +++ b/dom/settings/SettingsRequestManager.jsm @@ -704,7 +704,9 @@ let SettingsRequestManager = { 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) { @@ -743,20 +745,38 @@ let SettingsRequestManager = { } }, - 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) { @@ -812,7 +832,7 @@ let SettingsRequestManager = { 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)) { diff --git a/dom/settings/moz.build b/dom/settings/moz.build index c12a43e655eb..10a6055ba12c 100644 --- a/dom/settings/moz.build +++ b/dom/settings/moz.build @@ -23,3 +23,5 @@ EXTRA_JS_MODULES += [ ] MOCHITEST_MANIFESTS += ['tests/mochitest.ini'] + +XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini'] diff --git a/dom/settings/tests/unit/test_settingsrequestmanager_messages.js b/dom/settings/tests/unit/test_settingsrequestmanager_messages.js new file mode 100644 index 000000000000..e8fae7bc0377 --- /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(); +}); diff --git a/dom/settings/tests/unit/xpcshell.ini b/dom/settings/tests/unit/xpcshell.ini new file mode 100644 index 000000000000..41da0dad78a0 --- /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