Closed Bug 956857 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPCOM, defect, P3, trivial)

defect

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: crussell, Assigned: dorsatum)

References

Details

(Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][good first bug])

Attachments

(1 file, 3 obsolete files)

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
(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).
It should clearly be aEntryName in the second case to match the first case and the code immediately below. Patches accepted.
I have no plans to take this.
See Also: → 374754
Severity: normal → trivial
Priority: -- → P3
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++]
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++] → [mentor=benjamin@smedbergs.us][lang=c++][good first bug]
Hi, I'd like to give it a shot.
Projjol, great. I hope comment 0/2 are sufficient for you to write a patch. Let me know if you run into problems.
Assignee: nobody → probaner23
Attached patch oldentry.patch (obsolete) — Splinter Review
Is this fine?
Attachment #8357607 - Flags: feedback?(benjamin)
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.
Attachment #8357607 - Flags: feedback?(benjamin) → feedback-
(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.
> 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.
(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?
Attached patch patch2.patch (obsolete) — Splinter Review
Is this correct?
Attachment #8357607 - Attachment is obsolete: true
Attachment #8358770 - Flags: feedback?(benjamin)
Comment on attachment 8358770 [details] [diff] [review]
patch2.patch

This doesn't seem like the right patch...
Attachment #8358770 - Flags: feedback?(benjamin) → feedback-
Really sorry. Where am I going wrong?
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`.
Could we go over this once on the IRC? I'll clear everything up so the next patch that I submit would be acceptable.
Attached patch oldentry.patch (obsolete) — Splinter Review
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.
Attachment #8358770 - Attachment is obsolete: true
Attachment #8362145 - Flags: feedback?(benjamin)
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.
Attachment #8362145 - Flags: feedback?(benjamin) → feedback-
Attached patch test.patchSplinter Review
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.
Attachment #8362145 - Attachment is obsolete: true
Attachment #8367080 - Flags: review?(benjamin)
Comment on attachment 8367080 [details] [diff] [review]
test.patch

Great thanks!
Attachment #8367080 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/732dba2ce115
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.