58423 - mTitle.ToNewCString(); not assigned to anything

Status: NEW

(Core :: XUL, defect, P3)

Description

[Peter "jag" Annema]

313   char *title = ConvertToFileSystemCharset(mTitle.GetUnicode());
314   if (nsnull == title)
315     mTitle.ToNewCString();

Line 315 should probably be something like line 124:
122   char *title = ConvertToFileSystemCharset(mTitle.GetUnicode());
123   if (nsnull == title)
124     title = mTitle.ToNewCString();

Comment 1

[Peter "jag" Annema]

Adding keywords, attaching patch.

Comment 2

[Peter "jag" Annema]

Attachment: [patch] make mTitle.ToNewCString() assign to something

Comment 3

[Peter "jag" Annema]

This is a leak. Not a serious one, but still a leak. Making summary a bit more
clear on that.

Comment 4

[Peter Trudelle]

CVS blame says nhotta wrote that, reassigning.

Comment 5

[nhottanscp]

r=nhotta

Comment 6

[Brendan Eich [:brendan]]

I added the mlk keyword, can you remove the summary [mlk] noise-word?

It seems this code always used delete[] title (in both places) if title was
non-null, yet it wanted to set title from the ConvertToFileSystemCharset, which
returns new char[], or failing that, from the ToNewCString result, which IIRC
returns nsMemory::Alloc'd code.  So doesn't this mean that sometimes, purify
will complain about a mismatch?

I say fix ConvertToFileSystemCharset to use nsMemory too.

/be

Comment 7

[nhottanscp]

I agree with changing it to use nsMemory.
I think ToNewCString used to be freed by delete[].

Comment 8

[nhottanscp]

[e-mail], do you also want to do the memory allocation change?

Comment 9

[Peter "jag" Annema]

Attachment: [patch] Fix assignment and memory allocation

Comment 10

[Peter "jag" Annema]

Here's a first cut... I don't have access to a Windows machine to test compile
on though (and hunting someone down in #mozilla has so far proven unsuccessful),
so this is untested. I've currently only changed the (de)allocations for the
functions and variables directly affected, if so desired changing
ConvertFromFileSystemCharset and its callers shouldn't be too hard either.

Comment 11

[Peter "jag" Annema]

Attachment: [patch] While I was cleaning up ...

Comment 12

[nhottanscp]

Thanks, please also change ConvertFromFileSystemCharset.
Since I only know the code around the charset conversion, you should also get a
review from the module owner of this file.

Comment 13

[Peter "jag" Annema]

nhotta: check patch #3 ;-)

Comment 14

[nhottanscp]

r=nhotta
It compiled on windows.
As I said, the first half of the patch does not related to charset conversion,
please find a module owner of widget to get a review.

I tested with the patch, and put a break point at nsFileWidget::Show but it
didn't break when I used open dialog and save dialog. The code might not be used
anymore.
Cc to pinketon. Mike, who is a module owner of widget? I cannot find in
http://www.mozilla.org/owners.html

Comment 15

[Mike Pinkerton (not reading bugmail)]

there is no real module owner for widget, though rods claims it for win32, pav
claims it for linux, and we tagteam for mac.

Comment 16

[Brendan Eich [:brendan]]

jag: you inverted some if (NULL == converted), etc., tests and used if
(converted), which is cool -- I don't see any incompatible change there, but I
question the original code (which you didn't write) covering up out-of-memory
errors.  Worth fixing now?  Do in a later bug?  XXX comment now if the latter.

Here is a change I think that's not compatible, or a bugfix:


-         if (NS_SUCCEEDED(rv)) 
-                 rv = platformCharset->GetCharset(kPlatformCharsetSel_FileName,
aCharset);
-
     NS_ASSERTION(NS_SUCCEEDED(rv), "error getting platform charset");
-         if (NS_FAILED(rv)) 
-                 aCharset.AssignWithConversion("windows-1252");
+
+    if (NS_SUCCEEDED(rv)) 
+      rv = platformCharset->GetCharset(kPlatformCharsetSel_FileName, aCharset);
+    else
+      aCharset.AssignWithConversion("windows-1252");

The old code would fall back on "windows-1252" if anything failed, early or
late.  You fall back only in the earlier failure, and ignore a later failure in
GetCharset.  See what I mean?

/be

Comment 17

[Peter "jag" Annema]

Doh! Yes, I see what you mean. I thought I had checked for that...
Okay, so I'll undo that whole change :-)

Re: out-of-memory errors: looking into it.

cc'ing rods for review / input on this code.

Comment 18

[nhottanscp]

We can check in the first patch for the memory leak now. I'll ask brendan for an
approval.

Comment 19

[Brendan Eich [:brendan]]

Sure -- sr= on the first patch, but please finish up the related cleanups too,
they make the code significantly better, and should not be dropped on the floor.
 Thanks,

/be

Comment 20

[Brendan Eich [:brendan]]

sr=[e-mail] on the first (one-line) patch, I mean.

/be

Comment 21

[nhottanscp]

Checked in the first patch, remove mlk keyword.

Comment 22

[Peter "jag" Annema]

Cool, thanks :-)

nhotta:
Shall I attach the last patch with the correction Brendan pointed out?

I can look at his other comments (current out of memory cover-ups, converting to
String code istead of manually allocating/freeing memory) later this month, so
if you can do that sooner, that'd be cool.

Comment 23

[nhottanscp]

Yes, please attach the patch.
I reassign to you since you're actually fixing it. I can help you to check in.
Also please get a review from xptool kit group like rods since the rest of the
changes are mostly not in international code.

Comment 24

[Peter "jag" Annema]

Mass move to [e-mail]

Comment 25

[Peter Trudelle]

->future