312000 - nsICategoryManager::AddCategoryEntry doesn't return the previous value of the entry (if any)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

AddCategoryEntry's IDL comments specify that it returns the previous value of
the entry in the specified category (if any such value existed).  The current
implementation, however, doesn't actually set this value.  (It looks like the
implementation before 2003 actually did return the value.)

Somewhat tangentially, the IDL comments don't really say whether the current
value of the entry is returned if the fifth parameter |aReplace| is set to
PR_FALSE.  The previous behavior should probably be duplicated, and ideally this
could be clarified in IDL comments.

Comment 1

[Benjamin Smedberg]

Attachment: Actually set the parameter, rev. 1

I am ashamed to have rewritten this code and flubbed this up.

Comment 2

[Darin Fisher]

Comment on attachment 199238 [details] [diff] [review]
Actually set the parameter, rev. 1

>+          // this hunk of code is "strdup" with the XPCOM allocator
>+          int toDupLen = strlen(toDup);
>+          *_retval = NS_Alloc(toDupLen + 1);
>+          if (!*_retval)
>+            return NS_ERROR_OUT_OF_MEMORY;
>+
>+          memcpy(*_retval, toDup, toDupLen + 1);

This also works:

	    *_retval = ToNewCString(nsDependentCString(toDup));

You could also just use nsCRT::strdup because that is what
we use everywhere else.  Then, we should go file a bug to
fix nsCRT::strdup to actually use NS_Alloc.

r=darin either which way

Comment 3

[Benjamin Smedberg]

Attachment: Actually set the parameter, rev. 1.1

Final patch as checked in.

Comment 4

[Benjamin Smedberg]

Fixed on trunk. Darin, I'm not sure whether I want this for branch or not: it's
a definite correctness fix for a frozen interface, but we probably haven't
audited callers in a long time.

Comment 5

[Darin Fisher]

So, this was also broken in Gecko 1.7?

Comment 6

[Benjamin Smedberg]

That's correct.

Comment 7

[Darin Fisher]

OK, then I agree that it is probably best to not take this fix for Gecko 1.8

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 240478 has been marked as a duplicate of this bug. ***