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

@@ -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)) {

dom/settings/moz.build

@@ -23,3 +23,5 @@ EXTRA_JS_MODULES += [
 ]
 
 MOCHITEST_MANIFESTS += ['tests/mochitest.ini']
+
+XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']

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();
+});

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