IOW, it should listen to NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID and NS_XPCOM_CATEGORY_CLEARED_OBSERVER_ID and clean it's hash table. Though, I'm not really sure what we can do with NS_XPCOM_CATEGORY_CLEARED_OBSERVER_ID.

http://mxr.mozilla.org/mozilla-central/search?string=NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID&case=1 "Found 7 matching lines in 4 files" http://mxr.mozilla.org/mozilla-central/search?string=NS_XPCOM_CATEGORY_CLEARED_OBSERVER_ID&case=1 "Found 6 matching lines in 4 files" http://mxr.mozilla.org/comm-central/source/mozilla/dom/base/nsScriptNameSpaceManager.cpp#736 { 738 nsScriptNameSpaceManager::Observe(nsISupports* aSubject, const char* aTopic, ... 756 // TODO: we could observe NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID 757 // and NS_XPCOM_CATEGORY_CLEARED_OBSERVER_ID but we are safe without it. 758 // See bug 600460. }

Comment on attachment 791769 [details] [diff] [review] Implement category entry removal for nsScriptNamespaceManager Review of attachment 791769 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsScriptNameSpaceManager.cpp @@ +174,5 @@ > return &entry->mGlobalName; > } > > +void > +nsScriptNameSpaceManager::RemoveFromHash(PLDHashTable *aTable, Please put the *s to the left.

Comment on attachment 791769 [details] [diff] [review] Implement category entry removal for nsScriptNamespaceManager Review of attachment 791769 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch! Just a few things to be sorted out. ::: dom/base/nsScriptNameSpaceManager.cpp @@ +672,5 @@ > + type == nsGlobalNameStruct::eTypeNavigatorProperty ? > + LookupNavigatorName(entry) : LookupNameInternal(entry); > + if (!s || s->mType != type) { > + return NS_OK; > + } Hm, why do we need to do this checking? Shouldn't we just be able to tell the hashtable to remove whatever it has without checking whether it exists beforehand? @@ +765,5 @@ > +nsresult > +nsScriptNameSpaceManager::RemoveCategoryEntryFromHash(const char* aCategory, > + nsISupports* aEntry) > +{ > + return OperateCategoryEntryHash(nullptr, aCategory, aEntry, true); I'd prefer to just pass the category manager in here and pass it to the helper, assert that it's non-null in OperateCategoryEntryHash, and avoid the re-ordering tricks. ::: dom/base/nsScriptNameSpaceManager.h @@ +194,5 @@ > * @aCategoryManager Instance of the category manager service. > * @aCategory Category where the entry comes from. > * @aEntry The entry that should be added. > */ > + nsresult AddCategoryEntryToHash(nsICategoryManager *aCategoryManager, Why this change? Might as well be consistent with * placement with the other two arguments. I know this file itself is horribly inconsistent, but so it goes. @@ +209,5 @@ > + */ > + nsresult RemoveCategoryEntryFromHash(const char* aCategory, > + nsISupports* aEntry); > + > + nsresult OperateCategoryEntryHash(nsICategoryManager *aCategoryManager, Same here - move the * left? Also, I think we should mark this |protected|, and comment that it's factored-out common code between {Add,Remove}CategoryEntryToHash. ::: dom/tests/mochitest/bugs/test_bug641552.html @@ +42,5 @@ > > +// Ensure the initial state > +ok(!window.randomname, "window.randomname should return undefined"); > +is(typeof(window.navigator.randomname1), 'undefined', "navigator.randomname1 should return undefined"); > +is(typeof(window.navigator.randomname2), 'undefined', "navigator.randomname1 should return undefined"); It could be nice to factor these out into helper functions defined in terms of categoryEntries rather than copy-pasting the different property names everywhere. But it's also fine as-is for test code.

(In reply to Bobby Holley (:bholley) from comment #5) > ::: dom/base/nsScriptNameSpaceManager.cpp > @@ +672,5 @@ > > + type == nsGlobalNameStruct::eTypeNavigatorProperty ? > > + LookupNavigatorName(entry) : LookupNameInternal(entry); > > + if (!s || s->mType != type) { > > + return NS_OK; > > + } > > Hm, why do we need to do this checking? Shouldn't we just be able to tell > the hashtable to remove whatever it has without checking whether it exists > beforehand? I would not like to allow removing names not registered by this API (e.g. "XMLHttpRequest"). > @@ +765,5 @@ > > +nsresult > > +nsScriptNameSpaceManager::RemoveCategoryEntryFromHash(const char* aCategory, > > + nsISupports* aEntry) > > +{ > > + return OperateCategoryEntryHash(nullptr, aCategory, aEntry, true); > > I'd prefer to just pass the category manager in here and pass it to the > helper, assert that it's non-null in OperateCategoryEntryHash, and avoid the > re-ordering tricks. I tried it first, but GetCategoryEntry failed to get the contract ID from the category entry because it was alreay unregistered before observers were notified. So the order is needed anyway. > ::: dom/base/nsScriptNameSpaceManager.h > @@ +194,5 @@ > > * @aCategoryManager Instance of the category manager service. > > * @aCategory Category where the entry comes from. > > * @aEntry The entry that should be added. > > */ > > + nsresult AddCategoryEntryToHash(nsICategoryManager *aCategoryManager, > > Why this change? Might as well be consistent with * placement with the other > two arguments. I know this file itself is horribly inconsistent, but so it > goes. I didn't meant to change it. Fixed. > Also, I think we should mark this |protected|, and comment that it's > factored-out common code between {Add,Remove}CategoryEntryToHash. Changing from private to protected? (I don't know why the private method had such a gorgeous comment.) > ::: dom/tests/mochitest/bugs/test_bug641552.html > @@ +42,5 @@ > > > > +// Ensure the initial state > > +ok(!window.randomname, "window.randomname should return undefined"); > > +is(typeof(window.navigator.randomname1), 'undefined', "navigator.randomname1 should return undefined"); > > +is(typeof(window.navigator.randomname2), 'undefined', "navigator.randomname1 should return undefined"); > > It could be nice to factor these out into helper functions defined in terms > of categoryEntries rather than copy-pasting the different property names > everywhere. But it's also fine as-is for test code. Done.

Comment on attachment 792491 [details] [diff] [review] Implement category entry removal for nsScriptNamespaceManager, v2 Review of attachment 792491 [details] [diff] [review]: ----------------------------------------------------------------- > I would not like to allow removing names not registered by this API (e.g. "XMLHttpRequest"). I still don't understand this. LookupNavigatorName and LookupNameInternal more or less just check whether the name is in the hashtables (mNavigatorNames or mGlobalNames), and RemoveFromHash just removes the entry from the hashtable if they exist, and no-ops otherwise. Given that we early-return right after the call to RemoveFromHash, I don't see what we gain from checking first. r- on account of that. If I'm confused (which I may be!) please explain, and add some comments explaining as well. :-) > Changing from private to protected? (I don't know why the private method had such a gorgeous comment.) Oh, I thought it was public. Nevermind then. > I tried it first, but GetCategoryEntry failed to get the contract ID from the category entry because > it was alreay unregistered before observers were notified. So the order is needed anyway. Ok. But please still pass the category manager in both cases, and assert that it's non-null. Also, please add a comment explaining why the ordering is important. ::: dom/base/nsScriptNameSpaceManager.cpp @@ +758,5 @@ > +nsScriptNameSpaceManager::AddCategoryEntryToHash(nsICategoryManager* aCategoryManager, > + const char* aCategory, > + nsISupports* aEntry) > +{ > + return OperateCategoryEntryHash(aCategoryManager, aCategory, aEntry, false); Please do /* aRemove = */ true here, and /* aRemove = */ false below.

(In reply to Bobby Holley (:bholley) from comment #7) > > I would not like to allow removing names not registered by this API (e.g. "XMLHttpRequest"). > > I still don't understand this. LookupNavigatorName and LookupNameInternal > more or less just check whether the name is in the hashtables > (mNavigatorNames or mGlobalNames), and RemoveFromHash just removes the entry > from the hashtable if they exist, and no-ops otherwise. Given that we > early-return right after the call to RemoveFromHash, I don't see what we > gain from checking first. The |s->mType != type| check will reject the request if the name type is, for example, eTypeNewDOMBinding. If I just call RemoveFromHash without checking the type, this API will remove any global names regardless of the name type. It looks like all name types used by this API (eTypeExternalConstructor, eTypeProperty, eTypeNavigatorProperty, eTypeStaticNameSet, and eTypeDynamicNameSet) are only used for this API. So the type check will work as expected unless I'm missing something.

Ah, I see. So this check is only relevant in the mGlobalNames case, which has multiple different types that correspond to the same hashmap, right? Please add some comments and assertions making this all more explicit.

Resolved review comments.

Comment on attachment 793505 [details] [diff] [review] Implement category entry removal for nsScriptNamespaceManager, v3 Review of attachment 793505 [details] [diff] [review]: ----------------------------------------------------------------- Looks great - thanks. :-)