nsCategoryManager::AddCategoryEntry should notify about removal of the previous value

RESOLVED FIXED

Status

()

Product:
Core
Component:
XPCOM
Status:
RESOLVED FIXED
Reported:
12 years ago
Modified:
5 years ago

People

(Reporter: gaubugzilla, Assigned: gaubugzilla)

Tracking

Version:
Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Slightly cleaner patch
3.79 KB, patch
gaubugzilla
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

•
12 years ago 
Right now if you use nsICategoryManager::AddCategoryEntry with aReplace parameter set it will only send notifications about the category entry added but not about the one removed/replaced. So for example, if you have a content policy registered and then autoregistration is re-run you end up with two instances of your content policy registered: one instance was already registered on startup and when autoregistration called nsICategoryManager::AddCategoryEntry another one has been added. I think we need this change:

   if (NS_SUCCEEDED(rv)) {
+    if (*retval) {
+      NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID,
+                      aCategoryName, *retval);
+    }
     NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID,
                     aCategoryName, aEntryName);
   }

Will test it later.
(Assignee)

Comment 1

•
12 years ago 
Created attachment 264630 [details] [diff] [review]
Proposed patch and testcase
Assignee: nobody → trev.moz
Status: NEW → ASSIGNED
Attachment #264630 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

•
12 years ago 
Typo in the comment - "if the called" should be "if the caller" of course.
 (Assignee)

Updated

•
12 years ago
Attachment #264630 - Flags: review?(cbiesinger) → review?(darin.moz)

Comment 3

•
12 years ago 
Comment on attachment 264630 [details] [diff] [review]
Proposed patch and testcase

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: xpcom/components/nsCategoryManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/components/nsCategoryManager.cpp,v
>retrieving revision 1.51
>diff -p -u -8 -r1.51 nsCategoryManager.cpp
>--- xpcom/components/nsCategoryManager.cpp	10 May 2006 17:30:08 -0000	1.51
>+++ xpcom/components/nsCategoryManager.cpp	13 May 2007 00:51:04 -0000
>@@ -619,24 +619,35 @@ nsCategoryManager::AddCategoryEntry( con
>     char* categoryName = ArenaStrdup(aCategoryName, &mArena);
>     mTable.Put(categoryName, category);
>   }
>   PR_Unlock(mLock);
> 
>   if (!category)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>+  // We will need the return value of AddLeaf even if the called doesn't want it
>+  char *oldEntry = nsnull;
>+  if (!_retval)
>+    _retval = &oldEntry;
>+

might be slightly cleaner to just use oldEntry directory, and then copy oldEntry to *_retval if _retval is non-null.

otherwise, this change looks fine to me.


>   nsresult rv = category->AddLeaf(aEntryName,
>                                   aValue,
>                                   aPersist,
>                                   aReplace,
>                                   _retval,
>                                   &mArena);
> 
>   if (NS_SUCCEEDED(rv)) {
>+    if (*_retval) {
>+      NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID,
>+                      aCategoryName, *_retval);
>+      if (oldEntry)
>+        nsMemory::Free(oldEntry);
>+    }
>     NotifyObservers(NS_XPCOM_CATEGORY_ENTRY_ADDED_OBSERVER_ID,
>                     aCategoryName, aEntryName);
>   }
> 
>   return rv;
> }
> 
> NS_IMETHODIMP
>Index: xpcom/tests/unit/test_bug374754.js
>===================================================================
>RCS file: xpcom/tests/unit/test_bug374754.js
>diff -N xpcom/tests/unit/test_bug374754.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ xpcom/tests/unit/test_bug374754.js	13 May 2007 00:44:44 -0000
>@@ -0,0 +1,58 @@
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+var addedTopic = "xpcom-category-entry-added";
>+var removedTopic = "xpcom-category-entry-removed";
>+var testCategory = "bug-test-category";
>+var testEntry = "@mozilla.org/bug-test-entry;1";
>+
>+var result = "";
>+var expected = "add remove add remove ";
>+var timer;
>+
>+var observer = {
>+  QueryInterface: function(iid) {
>+    if (iid.equals(Ci.nsISupports) || iid.equals(Ci.nsIObserver))
>+      return this;
>+
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },
>+
>+  observe: function(subject, topic, data) {
>+    if (topic == "timer-callback") {
>+      do_check_eq(result, expected);
>+
>+      var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+      observerService.removeObserver(this, addedTopic);
>+      observerService.removeObserver(this, removedTopic);
>+
>+      do_test_finished();
>+
>+      timer = null;
>+    }
>+
>+    if (subject.QueryInterface(Ci.nsISupportsCString).data != testEntry || data != testCategory)
>+      return;
>+
>+    if (topic == addedTopic)
>+      result += "add ";
>+    else if (topic == removedTopic)
>+      result += "remove ";
>+  }
>+};
>+
>+function run_test() {
>+  do_test_pending();
>+
>+  var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+  observerService.addObserver(observer, addedTopic, false);
>+  observerService.addObserver(observer, removedTopic, false);
>+
>+  var categoryManager = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
>+  categoryManager.addCategoryEntry(testCategory, testEntry, testEntry, false, true);
>+  categoryManager.addCategoryEntry(testCategory, testEntry, testEntry, false, true);
>+  categoryManager.deleteCategoryEntry(testCategory, testEntry, false);
>+
>+  timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+  timer.init(observer, 0, timer.TYPE_ONE_SHOT);
>+}
Attachment #264630 - Flags: review?(darin.moz) → review+
 (Assignee)

Comment 4

•
12 years ago 
Created attachment 266247 [details] [diff] [review]
Slightly cleaner patch

Darin, can you sr this as well?
Attachment #264630 - Attachment is obsolete: true
Attachment #266247 - Flags: superreview?(darin.moz)
Attachment #266247 - Flags: review+

Comment 5

•
12 years ago 
Comment on attachment 266247 [details] [diff] [review]
Slightly cleaner patch

OK, sr=darin
Attachment #266247 - Flags: superreview?(darin.moz) → superreview+
 (Assignee)

Updated

•
12 years ago
Whiteboard: [checkin needed]

Comment 6

•
12 years ago 
xpcom/components/nsCategoryManager.cpp 1.52
xpcom/tests/unit/test_bug374754.js 1.1
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.