267073 - Fix code errors in nsStringBundle

Status: RESOLVED INACTIVE

(Core :: Internationalization, defect)

Description

[timeless]

The url contains a code audit. Most of these are unchecked allocs, some trigger
FMM, one references an undeclared variable, one isn't released at all, and
there's an else after return for good measure.

if you are interested in picking up this bug and need help figuring out the
right allocator/release pair, note that the answer can be found by checking the
header that declares the allocator.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

i can try to play a little bit.

Comment 2

[Daniel Kraft]

Attachment: Some fixes

This should fix some of the problems. When I find time again, I'll look at the
others (where I'll have to look a bit through lxr to see what's wrong).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Calling delete on a pointer allocated via ToNewCString is wrong.  It should use
nsMemory::Free.

Comment 4

[Daniel Kraft]

Attachment: The same corrected

Thanks!

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think asking timeless for review is the right idea... I won't have time for a
real review of this patch for probably a week at least...

Comment 6

[timeless]

Comment on attachment 171652 [details] [diff] [review]
The same corrected

thanks for working on this.

>Index: intl/strres/src/nsStringBundle.cpp
>@@ -248,17 +249,20 @@ nsStringBundle::GetStringFromID(PRInt32 
>   rv = LoadProperties();
>   if (NS_FAILED(rv)) return rv;
>   
>   *aResult = nsnull;
>   nsAutoString tmpstr;

it looks like rv isn't used past this point.
could we replace ret with rv?
>   nsresult ret = GetStringFromID(aID, tmpstr);

I think i'd rather an early return:
  if (NS_FAILED(ret)) return ret; /* i'd rather rv as mentioned above */
instead of this indented if block:
>   if (NS_SUCCEEDED(ret))
>+  {
>     *aResult = ToNewUnicode(tmpstr);
>+    NS_ENSURE_TRUE(*aResult,NS_ERROR_OUT_OF_MEMORY);
please include spaces after commas. (occurs a couple of times)
>+  }
>   return ret;
> }

>@@ -268,17 +272,20 @@ nsStringBundle::GetStringFromName(const 
same comment

>@@ -698,21 +705,17 @@ nsStringBundleService::recycleEntry(bund
> }
> 
> NS_IMETHODIMP
> nsStringBundleService::CreateBundle(const char* aURLSpec, 
>                                     nsIStringBundle** aResult)
> {
> #ifdef DEBUG_tao_
>   printf("\n++ nsStringBundleService::CreateBundle ++\n");
>-  {
>-    printf("\n** nsStringBundleService::CreateBundle: %s\n",
>-           aURLSpec ? aURLSpec : "null");
>-    delete s;
>-  }
>+  printf("\n** nsStringBundleService::CreateBundle: %s\n",

this won't compile.

> #endif
> 
>   return getStringBundle(aURLSpec,aResult);
> }

Comment 7

[Daniel Kraft]

Attachment: Corrected version

How about that?

Comment 8

[Daniel Kraft]

Comment on attachment 171747 [details] [diff] [review]
Corrected version

Sorry timeless, I found some mistakes myself...

Comment 9

[Daniel Kraft]

Attachment: More fixes

Now, this should be better. I'm afraid it took some time, but I completely
forgot about uploading that patch...

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #9)
> Created an attachment (id=172379) [edit]
> More fixes
> 
> Now, this should be better. I'm afraid it took some time, but I completely
> forgot about uploading that patch...

timeless asked me to look at the patch - nsStringBundle::GetCombinedEnumeration
has some marked lines that you don't seem to touch.  Do those not need to be
fixed?  291, 292, and 299 look ok to me, but I think the return value from line
284 needs to be checked.

Comment 11

[Daniel Kraft]

I don't see what needs to be checked in 284 (ENSURE_TRUE(*aResult...) is already
present), please tell me what you mean (the same would be for 261).
I know I haven't yet corrected all errors, as I've had not much time and it was
not completely clear to me what to do there. But I'll look at them as soon as
possible.

Comment 12

[Daniel Kraft]

Found what to do therefore, it wasn't really that hard...

Comment 13

[Daniel Kraft]

Attachment: patch

Does lines are fixed now, please let me know what additional checks you want!

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 172572 [details] [diff] [review]
patch

Sorry, I'm not a peer for the relevant module, so I can't actually r+ the patch
(it looks good to me, though).

Comment 15

[timeless]

Comment on attachment 172572 [details] [diff] [review]
patch

sorry, i'm not an sr. but thanks for working on this :).

+  rv=GetStringFromID(aID, tmpstr);
+  rv=NS_NewArray(getter_AddRefs(resultArray));
+    rv=propEnumerator->HasMoreElements(&hasMore);

to match the style for this file, you should have spaces around the = .

Comment 16

[Daniel Kraft]

Is it important to follow the style of a particular file up to those details?

Comment 17

[Jungshik Shin]

Do you have any reason not to? Mozilla.org doesn't impose any particular coding
style, but within a single file (or depending on the situation, within a single
directory or a single module), the consistency is a good thing (TM). 
 

Comment 18

[timeless]

yes: http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual

Comment 19

[Daniel Kraft]

Attachment: with spaces changed

OK, you persuaded me...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172667 [details] [diff] [review]
with spaces changed

sr=bzbarsky.  Let me know if this needs landing (though I assume timeless will
check this in).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk for 1.8b.  Not resolving, since I'm not sure whether there
is more to be done here.

Comment 22

[[e-mail]]

This patch introduced a bunch of meaningless assertions. It is not illegal for
GetStringFromName to fail. I'm not even sure that it is illegal for
LoadProperties to fail, but I do think that NS_ENSURE_SUCCESS is overkill.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.