Bug 345529 - "crash removing an observer during an nsPref:changed notification [@ pref_DoCallback] (node is 0xdddddddd)" [p=asqueella@gmail.com (Nickolay Ponomarev) r+sr=bsmedberg sr=darinf a=blocking1.9+]

modules/libpref/Makefile.in

@@ -42,6 +42,10 @@ VPATH		= @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
+ifdef ENABLE_TESTS
+TOOL_DIRS += test
+endif
+
 MODULE		= pref
 DIRS		= public src
 

modules/libpref/public/nsIPrefBranch2.idl

@@ -93,6 +93,21 @@ interface nsIPrefBranch2 : nsIPrefBranch
    *    registered. This insures that shorter lived objects (say one tied to an
    *    open window) will not fall into the cyclical reference trap.
    *
+   * @note
+   * The list of registered observers may be changed during the dispatch of
+   * nsPref:changed notification. However, the observers are not guaranteed
+   * to be notified in any particular order, so you can't be sure whether the
+   * added/removed observer will be called during the notification when it
+   * is added/removed.
+   *
+   * @note
+   * It is possible to change preferences during the notification.
+   *
+   * @note
+   * It is not safe to change observers during this callback in Gecko 
+   * releases before 1.9. If you want a safe way to remove a pref observer,
+   * please use an nsITimer.
+   *
    * @see nsIObserver
    * @see removeObserver
    */

modules/libpref/src/prefapi.cpp

@@ -122,6 +122,9 @@ PRBool              gDirty = PR_FALSE;
 static struct CallbackNode* gCallbacks = NULL;
 static PRBool       gCallbacksEnabled = PR_TRUE;
 static PRBool       gIsAnyPrefLocked = PR_FALSE;
+// These are only used during the call to pref_DoCallback
+static PRBool       gCallbacksInProgress = PR_FALSE;
+static PRBool       gShouldCleanupDeadNodes = PR_FALSE;
 
 
 static PLDHashTableOps     pref_HashTableOps = {
@@ -174,6 +177,9 @@ static PRBool pref_ValueChanged(PrefValue oldValue, PrefValue newValue, PrefType
 /* -- Privates */
 struct CallbackNode {
     char*                   domain;
+    // If someone attempts to remove the node from the callback list while
+    // pref_DoCallback is running, |func| is set to nsnull. Such nodes will
+    // be removed at the end of pref_DoCallback.
     PrefChangedFunc         func;
     void*                   data;
     struct CallbackNode*    next;
@@ -205,6 +211,8 @@ nsresult PREF_Init()
 /* Frees the callback list. */
 void PREF_Cleanup()
 {
+    NS_ASSERTION(!gCallbacksInProgress,
+        "PREF_Cleanup was called while gCallbacksInProgress is PR_TRUE!");
     struct CallbackNode* node = gCallbacks;
     struct CallbackNode* next_node;
 
@@ -818,6 +826,9 @@ PREF_RegisterCallback(const char *pref_node,
                        PrefChangedFunc callback,
                        void * instance_data)
 {
+    NS_PRECONDITION(pref_node, "pref_node must not be nsnull");
+    NS_PRECONDITION(callback, "callback must not be nsnull");
+
     struct CallbackNode* node = (struct CallbackNode*) malloc(sizeof(struct CallbackNode));
     if (node)
     {
@@ -830,7 +841,29 @@ PREF_RegisterCallback(const char *pref_node,
     return;
 }
 
-/* Deletes a node from the callback list. */
+/* Removes |node| from gCallbacks list.
+   Returns the node after the deleted one. */
+struct CallbackNode*
+pref_RemoveCallbackNode(struct CallbackNode* node,
+                        struct CallbackNode* prev_node)
+{
+    NS_PRECONDITION(!prev_node || prev_node->next == node, "invalid params");
+    NS_PRECONDITION(prev_node || gCallbacks == node, "invalid params");
+
+    NS_ASSERTION(!gCallbacksInProgress,
+        "modifying the callback list while gCallbacksInProgress is PR_TRUE");
+
+    struct CallbackNode* next_node = node->next;
+    if (prev_node)
+        prev_node->next = next_node;
+    else
+        gCallbacks = next_node;
+    PR_Free(node->domain);
+    PR_Free(node);
+    return next_node;
+}
+
+/* Deletes a node from the callback list or marks it for deletion. */
 nsresult
 PREF_UnregisterCallback(const char *pref_node,
                          PrefChangedFunc callback,
@@ -844,16 +877,21 @@ PREF_UnregisterCallback(const char *pref_node,
     {
         if ( strcmp(node->domain, pref_node) == 0 &&
              node->func == callback &&
-             node->data == instance_data )
+             node->data == instance_data)
         {
-            struct CallbackNode* next_node = node->next;
-            if (prev_node)
-                prev_node->next = next_node;
+            if (gCallbacksInProgress)
+            {
+                // postpone the node removal until after
+                // gCallbacks enumeration is finished.
+                node->func = nsnull;
+                gShouldCleanupDeadNodes = PR_TRUE;
+                prev_node = node;
+                node = node->next;
+            }
             else
-                gCallbacks = next_node;
-            PR_Free(node->domain);
-            PR_Free(node);
-            node = next_node;
+            {
+                node = pref_RemoveCallbackNode(node, prev_node);
+            }
             rv = NS_OK;
         }
         else
@@ -869,15 +907,49 @@ static nsresult pref_DoCallback(const char* changed_pref)
 {
     nsresult rv = NS_OK;
     struct CallbackNode* node;
+
+    PRBool reentered = gCallbacksInProgress;
+    gCallbacksInProgress = PR_TRUE;
+    // Nodes must not be deleted while gCallbacksInProgress is PR_TRUE.
+    // Nodes that need to be deleted are marked for deletion by nulling
+    // out the |func| pointer. We release them at the end of this function
+    // if we haven't reentered.
+
     for (node = gCallbacks; node != NULL; node = node->next)
     {
-        if ( PL_strncmp(changed_pref, node->domain, PL_strlen(node->domain)) == 0 )
+        if ( node->func &&
+             PL_strncmp(changed_pref,
+                        node->domain,
+                        PL_strlen(node->domain)) == 0 )
         {
             nsresult rv2 = (*node->func) (changed_pref, node->data);
             if (NS_FAILED(rv2))
                 rv = rv2;
         }
     }
+
+    gCallbacksInProgress = reentered;
+
+    if (gShouldCleanupDeadNodes && !gCallbacksInProgress)
+    {
+        struct CallbackNode* prev_node = NULL;
+        node = gCallbacks;
+
+        while (node != NULL)
+        {
+            if (!node->func)
+            {
+                node = pref_RemoveCallbackNode(node, prev_node);
+            }
+            else
+            {
+                prev_node = node;
+                node = node->next;
+            }
+        }
+        gShouldCleanupDeadNodes = PR_FALSE;
+    }
+
     return rv;
 }
 

modules/libpref/test/Makefile.in

@@ -0,0 +1,50 @@
+#
+# ***** BEGIN LICENSE BLOCK *****
+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
+#
+# The contents of this file are subject to the Mozilla Public License Version
+# 1.1 (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+# http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS IS" basis,
+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+# for the specific language governing rights and limitations under the
+# License.
+#
+# The Original Code is mozilla.org code.
+#
+# The Initial Developer of the Original Code is
+# Mozilla.org.
+# Portions created by the Initial Developer are Copyright (C) 2005
+# the Initial Developer. All Rights Reserved.
+#
+# Contributor(s):
+#     Boris Zbarsky <bzbarsky@mit.edu>  (Original author)
+#
+# Alternatively, the contents of this file may be used under the terms of
+# either of the GNU General Public License Version 2 or later (the "GPL"),
+# or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+# in which case the provisions of the GPL or the LGPL are applicable instead
+# of those above. If you wish to allow use of your version of this file only
+# under the terms of either the GPL or the LGPL, and not to allow others to
+# use your version of this file under the terms of the MPL, indicate your
+# decision by deleting the provisions above and replace them with the notice
+# and other provisions required by the GPL or the LGPL. If you do not delete
+# the provisions above, a recipient may use your version of this file under
+# the terms of any one of the MPL, the GPL or the LGPL.
+#
+# ***** END LICENSE BLOCK *****
+
+DEPTH		= ../../..
+topsrcdir	= @top_srcdir@
+srcdir		= @srcdir@
+VPATH		= @srcdir@
+
+include $(DEPTH)/config/autoconf.mk
+
+MODULE		= test_libpref
+
+XPCSHELL_TESTS = unit
+
+include $(topsrcdir)/config/rules.mk

modules/libpref/test/unit/test_bug345529.js

@@ -0,0 +1,34 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/  */ 
+
+// Regression test for bug 345529 - crash removing an observer during an
+// nsPref:changed notification.
+function run_test() {
+  const Cc = Components.classes;
+  const Ci = Components.interfaces;
+  const PREF_NAME = "testPref";
+
+  var prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch2);
+  var observer = {
+    QueryInterface: function QueryInterface(aIID) {
+      if (aIID.equals(Ci.nsIObserver) ||
+          aIID.equals(Ci.nsISupports))
+         return this;
+      throw Components.results.NS_NOINTERFACE;
+    },
+
+    observe: function observe(aSubject, aTopic, aState) {
+      prefs.removeObserver(PREF_NAME, observer);
+    }
+  }
+  prefs.addObserver(PREF_NAME, observer, false);
+
+  prefs.setCharPref(PREF_NAME, "test0")
+  // This second call isn't needed on a clean profile: it makes sure 
+  // the observer gets called even if the pref already had the value
+  // "test0" before this test.
+  prefs.setCharPref(PREF_NAME, "test1")
+
+  do_check_true(true);
+}