956857 - "xpcom-category-entry-removed" notification has busted semantics for subject parameter

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

The category manager notifies observers with the topic "xpcom-category-entry-removed" (among others).  The notification will occur when an entry is replaced with |addCategoryEntry|.

Problem is, when observers get notified of the -removed topic, in the former case the subject parameter will be the name of the entry removed, and in the latter case it will be the entry's old value.  There's no way for observers to distinguish which one is occurring (unless they were to keep their own copy of all the entries, build another copy of the entries at the time of notification, and compare the two.)

This is where the category manager notifies observers of a deletion:
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsCategoryManager.cpp#698

This is where it notifies observers in the case of a replacement:
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsCategoryManager.cpp#661

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

(Colby Russell :crussell wrote in comment #0)
> The notification will occur when an entry is replaced

That should say "when an entry is removed with |deleteCategoryEntry| and when an entry is replaced with |addCategoryEntry|".

nsScriptNamespaceManager and nsCategoryCache both treat the subject as the name of the entry removed:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptNameSpaceManager.cpp#781
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCategoryCache.cpp#131

There are no in-tree observers for this topic that expect the subject to be the old value in the entry-value-was-replaced case (which only makes sense; it's hard for the old value to be of any use when you don't have the name of the entry it corresponds to).

Comment 2

[Benjamin Smedberg]

It should clearly be aEntryName in the second case to match the first case and the code immediately below. Patches accepted.

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

I have no plans to take this.

Comment 4

[Projjol [:dorsatum]]

Hi, I'd like to give it a shot.

Comment 5

[Benjamin Smedberg]

Projjol, great. I hope comment 0/2 are sufficient for you to write a patch. Let me know if you run into problems.

Comment 6

[Projjol [:dorsatum]]

Attachment: oldentry.patch

Is this fine?

Comment 7

[Benjamin Smedberg]

Comment on attachment 8357607 [details] [diff] [review]
oldentry.patch

Does this patch compile? It looks like you removed `oldEntry` completely, but it is still used.

This also appears to be mixing up aOldValue (which should give you the old value) with the NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID notification, which is supposed to give you the name.

I suspect that the only part of this patch you needed was:

-                      aCategoryName, oldEntry);
+                      aCategoryName, aEntryName);

Also, there are already some tests for this functionality at /xpcom/tests/unit/test_bug374754.js. Could you please make this test also cover the case you're fixing here? Currently it uses `testEntry` for *both* the key name and the value, so you'll have to change it to use a different string for the key and the value so you can test that you're getting the right one.

Comment 8

[Projjol [:dorsatum]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)

> Does this patch compile? It looks like you removed `oldEntry` completely,
> but it is still used.
> 
I'm sorry, this going to sound like a really stupid question on my part but is there another instance of oldEntry in this file? Because I had checked before I put up the patch. If I made a mistake there, I apologize.

> I suspect that the only part of this patch you needed was:
> 
> -                      aCategoryName, oldEntry);
> +                      aCategoryName, aEntryName);
> 
Since, this part is okay, should I simply assign oldEntry to aEntryName for this one line and let everything else remain as it was earlier? Just a change in this line.

> Also, there are already some tests for this functionality at
> /xpcom/tests/unit/test_bug374754.js. Could you please make this test also
> cover the case you're fixing here? Currently it uses `testEntry` for *both*
> the key name and the value, so you'll have to change it to use a different
> string for the key and the value so you can test that you're getting the
> right one.

I'll make the required changes there.

Comment 9

[Benjamin Smedberg]

> I'm sorry, this going to sound like a really stupid question on my part but
> is there another instance of oldEntry in this file? Because I had checked
> before I put up the patch. If I made a mistake there, I apologize.

   nsresult rv = category->AddLeaf(aEntryName,
                                   aValue,
                                   aReplace,
-                                  &oldEntry,
                                   &mArena);

You removed oldEntry here, but the signature of AddLeaf would still require it (and we need it to pass back to the caller!)

> Since, this part is okay, should I simply assign oldEntry to aEntryName for

replace oldEntry with aEntryname, yes.

Comment 10

[Projjol [:dorsatum]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> > I'm sorry, this going to sound like a really stupid question on my part but
> > is there another instance of oldEntry in this file? Because I had checked
> > before I put up the patch. If I made a mistake there, I apologize.
> 
>    nsresult rv = category->AddLeaf(aEntryName,
>                                    aValue,
>                                    aReplace,
> -                                  &oldEntry,
>                                    &mArena);
> 
> You removed oldEntry here, but the signature of AddLeaf would still require
> it (and we need it to pass back to the caller!)
Ah, yes absolutely. Removing it totally messes up the parameters for the function. I'm sorry, I should've been more attentive.

And, I'll undo all the changes I made? They are not required then, right?

Comment 11

[Projjol [:dorsatum]]

Attachment: patch2.patch

Is this correct?

Comment 12

[Benjamin Smedberg]

Comment on attachment 8358770 [details] [diff] [review]
patch2.patch

This doesn't seem like the right patch...

Comment 13

[Projjol [:dorsatum]]

Really sorry. Where am I going wrong?

Comment 14

[Benjamin Smedberg]

Well, it looks like you wrote one patch, and then wrote another patch which undoes most of the work of the first patch. Although I'm not sure. If you're using Mercurial queues, you need to fold your patches together into one patch using `qfold`.

Comment 15

[Projjol [:dorsatum]]

Could we go over this once on the IRC? I'll clear everything up so the next patch that I submit would be acceptable.

Comment 16

[Projjol [:dorsatum]]

Attachment: oldentry.patch

Hi, I'm sorry for taking time to submit this patch. Does this version make the cut? 
Also, I built with the patch in and it was successful.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8362145 [details] [diff] [review]
oldentry.patch

The test is incorrect, because you're only changing the value in the second line, not the first line. We only check the value of the category-removed observer notification when it goes from the first to the second value. If you added that test to the current tree, it wouldn't fail as it should.

Comment 18

[Projjol [:dorsatum]]

Attachment: test.patch

It went along exactly how you'd said it would. I ran it with the changes in the test causing it to fail, followed by changes in the code file after which it passed.

Comment 19

[Benjamin Smedberg]

Comment on attachment 8367080 [details] [diff] [review]
test.patch

Great thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/732dba2ce115

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/732dba2ce115