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

RESOLVED FIXED in mozilla29

Status

()

Core
XPCOM
P3
trivial
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: crussell, Assigned: dorsatum)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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).

Comment 2

4 years ago
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: → bug 374754

Updated

4 years ago
Severity: normal → trivial
Priority: -- → P3
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++]

Updated

4 years ago
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++] → [mentor=benjamin@smedbergs.us][lang=c++][good first bug]
(Assignee)

Comment 4

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

Comment 5

4 years ago
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
(Assignee)

Comment 6

4 years ago
Created attachment 8357607 [details] [diff] [review]
oldentry.patch

Is this fine?
Attachment #8357607 - Flags: feedback?(benjamin)

Comment 7

4 years ago
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-
(Assignee)

Comment 8

4 years ago
(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

4 years ago
> 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.
(Assignee)

Comment 10

4 years ago
(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?
(Assignee)

Comment 11

4 years ago
Created attachment 8358770 [details] [diff] [review]
patch2.patch

Is this correct?
Attachment #8357607 - Attachment is obsolete: true
Attachment #8358770 - Flags: feedback?(benjamin)

Comment 12

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

This doesn't seem like the right patch...
Attachment #8358770 - Flags: feedback?(benjamin) → feedback-
(Assignee)

Comment 13

4 years ago
Really sorry. Where am I going wrong?

Comment 14

4 years ago
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`.
(Assignee)

Comment 15

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

Comment 16

4 years ago
Created attachment 8362145 [details] [diff] [review]
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.
Attachment #8358770 - Attachment is obsolete: true
Attachment #8362145 - Flags: feedback?(benjamin)

Comment 17

4 years ago
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-
(Assignee)

Comment 18

4 years ago
Created attachment 8367080 [details] [diff] [review]
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.
Attachment #8362145 - Attachment is obsolete: true
Attachment #8367080 - Flags: review?(benjamin)

Comment 19

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

Great thanks!
Attachment #8367080 - Flags: review?(benjamin) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/732dba2ce115
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.