Bug 1076597 - Fix Settings API shutdown race condition. r=bent
☠☠ backed out by 030cad353b62 ☠ ☠
authorAlexandre Lissy <lissyx@lissyx.dyndns.org>
Thu, 09 Oct 2014 10:51:34 +0200
changeset 209547 318da8a0ab9006b0454bf148640619e34ddf6609
parent 209546 dbd9a3b1ba7084ad1bbd0ba6583f541667cc3a72
child 209548 f50d71868097efd302004e78989aeb9fc8a1dadf
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersbent
bugs1076597
milestone35.0a1
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
dom/settings/moz.build
dom/settings/tests/unit/test_settingsrequestmanager_messages.js
dom/settings/tests/unit/xpcshell.ini
--- 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