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+]
authorreed@reedloden.com
Fri, 16 Nov 2007 20:25:41 -0800
changeset 8109 ea41e08a42ed7809ac15f454a389ca46419b508d
parent 8108 9044bacb5742cba767daf0260a18ca59496bb8b7
child 8110 eaeff0a6787bdf74dc55597f60848d092c3bd8a5
push id1
push userbsmedberg@mozilla.com
push dateThu, 20 Mar 2008 16:49:24 +0000
treeherdermozilla-central@61007906a1f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdarinf, blocking1.9
bugs345529
milestone1.9b2pre
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
modules/libpref/public/nsIPrefBranch2.idl
modules/libpref/src/prefapi.cpp
modules/libpref/test/Makefile.in
modules/libpref/test/unit/test_bug345529.js
--- a/modules/libpref/Makefile.in
+++ b/modules/libpref/Makefile.in
@@ -37,13 +37,17 @@
 
 DEPTH		= ../..
 topsrcdir	= @top_srcdir@
 srcdir		= @srcdir@
 VPATH		= @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
+ifdef ENABLE_TESTS
+TOOL_DIRS += test
+endif
+
 MODULE		= pref
 DIRS		= public src
 
 include $(topsrcdir)/config/rules.mk
 
--- a/modules/libpref/public/nsIPrefBranch2.idl
+++ b/modules/libpref/public/nsIPrefBranch2.idl
@@ -88,16 +88,31 @@ interface nsIPrefBranch2 : nsIPrefBranch
    *    may hold a weak reference to it instead of a strong one.
    * 2) The nsPrefBranch object listens for xpcom-shutdown and frees all of the
    *    objects currently in its observer list. This ensures that long lived
    *    objects (services for example) will be freed correctly.
    * 3) The observer can request to be held as a weak reference when it is
    *    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
    */
   void addObserver(in string aDomain, in nsIObserver aObserver,
                    in boolean aHoldWeak);
 
   /**
    * Remove a preference change observer.
--- a/modules/libpref/src/prefapi.cpp
+++ b/modules/libpref/src/prefapi.cpp
@@ -117,16 +117,19 @@ matchPrefEntry(PLDHashTable*, const PLDH
 
 PLDHashTable        gHashTable = { nsnull };
 static PLArenaPool  gPrefNameArena;
 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 = {
     PL_DHashAllocTable,
     PL_DHashFreeTable,
     PL_DHashStringKey,
     matchPrefEntry,
     PL_DHashMoveEntryStub,
@@ -169,16 +172,19 @@ static char *ArenaStrDup(const char* str
 #define PREF_HAS_USER_VALUE(pref)       ((pref)->flags & PREF_USERSET)
 #define PREF_TYPE(pref)                 (PrefType)((pref)->flags & PREF_VALUETYPE_MASK)
 
 static PRBool pref_ValueChanged(PrefValue oldValue, PrefValue newValue, PrefType type);
 
 /* -- 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;
 };
 
 /* -- Prototypes */
 static nsresult pref_DoCallback(const char* changed_pref);
 
@@ -200,16 +206,18 @@ nsresult PREF_Init()
                            PREFNAME_ARENA_SIZE);
     }
     return NS_OK;
 }
 
 /* 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;
 
     while (node)
     {
         next_node = node->next;
         PR_Free(node->domain);
         PR_Free(node);
@@ -813,76 +821,140 @@ PREF_PrefIsLocked(const char *pref_name)
 }
 
 /* Adds a node to the beginning of the callback list. */
 void
 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)
     {
         node->domain = PL_strdup(pref_node);
         node->func = callback;
         node->data = instance_data;
         node->next = gCallbacks;
         gCallbacks = 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,
                          void * instance_data)
 {
     nsresult rv = NS_ERROR_FAILURE;
     struct CallbackNode* node = gCallbacks;
     struct CallbackNode* prev_node = NULL;
 
     while (node != NULL)
     {
         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
         {
             prev_node = node;
             node = node->next;
         }
     }
     return rv;
 }
 
 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;
 }
 
 void PREF_ReaderCallback(void       *closure,
                          const char *pref,
                          PrefValue   value,
                          PrefType    type,
                          PRBool      isDefault)
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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);
+}