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.

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

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

Final patch as checked in.

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.

So, this was also broken in Gecko 1.7?

That's correct.

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

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