229032 - modernize 'String' usage in mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jungshik Shin]

It seems like the 'string revolution' hardly had an impact in mailnews/.  There
are a lot of places where nsString/nsCString are used as local variables and
function/method parameters. Also, there are a lot of AssignWithConversion() that
should be replaced with (Lossy)Copy(ASCII|UTF16)to(UTF16|ASCII), quite a lot of
ToNewXXX that can be avoided and so forth. 

I wanted to file this under mailnews - general or something like that, but
there's no such component in mailnews. If there's a better component, please,
feel free to move it there.

Comment 1

[Alec Flett]

yeah, especially in the area of charset conversion and charset naming...
mailnews has been hacked and patched for a long long time. (could probably cut
down on codesize, given the number of wierd conversion helper routines that are
probably never used!)

Comment 2

[Alec Flett]

and over to the default owner, but jshin, I suggest you take this :)

Comment 3

[Jungshik Shin]

OK. I'm taking (but it may be a while before I really begin to work on this)

alecf, do you think it's a good idea to embark on extending (and renaming)
'nsIUTF8ConverterService' I wrote last spring(?) to replace various
(To|From)(Unicode|UTF8)  helper functions around the tree? They're slightly
different from each other and we have to spec a new set of APIs to accomodate
them all with little disruption. That's another bug, I guess.

Here I want to deal with raw char*, PRUnichar*, nsString, nsCString, alloc/free
that can be mostly avoided if we use AutoString and XPIDLString. (see, for
instance, bug 228543). 

BTW, does anyone know why we have a lot of 'extern "C"'s in mailnews/. The only
C file in mailnews/ is movemail.c  that is kinda stand-alone but there may be a
reason I'm not aware of (linker-related?). How about 'typedef struct foo { ....}
foo'? 

Comment 4

[Jungshik Shin]

Attachment: patch (1st draft)

I just want to put this up so that I don't have to worry about losing it by
mistake.

It's very long (~ 6000 lines). About 500 lines are trimmed. Certainly, there
are more to do (and there are some inconsitency in this patch)

I didn't know there were so many charset converters in mailnews :-)

Comment 5

[Jungshik Shin]

Attachment: still a work in progress

Compiling on Windows, I found a dozen or so spots I missed (they're not built
on Linux).

Comment 6

[Jungshik Shin]

Attachment: update

Not quite ready to ask for r/sr, but getting close.

Comment 7

[[e-mail]]

I was reading attachment 138892 [details] [diff] [review] and it prompted a few questions...

It's confusing that those fields all seem to want different string types.
If the msgCompHeaderInternalCharset is always UTF-8, then you don't need
m_InternalCharSet, and you can just use the standard UTF16 to UTF8 conversion
(although you may still want a custom UTF8 to UTF16 conversion).

-  [noscript] void getLocale(in nsString result);
-  [noscript] void setLocale(in nsString locale);
+  AString getLocale();
+  void setLocale(in AString locale);
 
-  [noscript] void setMailboxName(in nsString newBoxName);
-  [noscript] void getMailboxName(in nsString boxName);
+  void setMailboxName(in AString newBoxName);
+  AString getMailboxName();

How about attribute AString locale; and attribute AString mailboxName;

-  yarn->mYarn_Buf = ToNewCString(*str);
+  yarn->mYarn_Buf = ToNewCString(str);
   yarn->mYarn_Size = PL_strlen((const char *) yarn->mYarn_Buf) + 1;
   yarn->mYarn_Fill = yarn->mYarn_Size - 1;

How about str.Length() somewhere around here?

Comment 8

[Jungshik Shin]

Thanks for the comment.

> always UTF-8, then you don't need m_InternalCharSet, 

  I meant to get rid of it and replaced its getters with 'UTF-8'. I did in a few
places, but didn't everywhere. Nor did I elminated m_InternalCharset. I'll do.
As for different string types, it's confusing to me, too. However, something
like |string getCharPtrProperty(in string propertyName)| are so widely used that
I decided to leave them alone in the first phase.

  BTW, I'll turn explicit getters/setters to attributes.

Comment 9

[Alec Flett]

yeah, multiple phases is good - we don't have to get it perfect the first time -
if we do it in phases we'll have an easier time tracking regressions...

nice work though - I see some good cleanup here..

Comment 10

[[e-mail]]

-                        (void)ConvertToUnicode(msgCompHeaderInternalCharset(),
fullAddress.get(), fullNameStr);
+                        CopyUTF8toUTF16(fullAddress, fullNameStr);
I like this. I think you should do this (and CopyUTF16toUTF8 too) more often.

Comment 11

[Jungshik Shin]

Attachment: update

addressed neil's concerns and some more.

Comment 12

[Jungshik Shin]

Comment on attachment 139351 [details] [diff] [review]
update 

asking for r/sr.

Comment 13

[[e-mail]]

Comment on attachment 139351 [details] [diff] [review]
update 

>+                (nsDependentString(convertedValue).FindChar((PRUnichar)' ') != -1);
PL_strchr perhaps?

>+    nsCOMPtr<nsILocalFile> localFile;
>+    nsAutoString fileName;
Why did you move these?

>+      return PL_strdup(convertedStr.get());
ToNewCString perhaps?

>+    nsCAutoString nativeString;
>+    nsresult rv = nsMsgI18NCopyUTF16ToNative(NS_ConvertUTF8toUTF16(folderURI),
>+                                             nativeString);
>+    if (NS_SUCCEEDED(rv))
>       oldPath.Assign(nativeString);
>     else
Why not
if (NS_FAILED(nsMsgI18NCopyUTF16ToNative(NS_ConvertUTF8toUTF16(folderURI),
oldPath)))

>-          nsMsgGetNativePathString(mFileSpec->GetNativePathCString(),path);
>+          NS_CopyNativeToUnicode(
>+            nsDependentCString(mFileSpec->GetNativePathCString()), path);
mFileSpec->GetUnicodePath(path) please.

>-NS_IMETHODIMP nsMsgCompFields::SetOrganization(const PRUnichar *value)
>+NS_IMETHODIMP nsMsgCompFields::SetOrganization(const nsAString &value)
> {
>   return SetUnicodeHeader(MSG_ORGANIZATION_HEADER_ID, value);
> }
I picked on this one. It has only once C++ caller, which wants to pass it a
UTF8 string, and converts it to UTF16; this function then converts it back to
UTF8. The other fields might not be so unlucky ;-)

>+			if (NS_FAILED(ConvertFromUnicode("UTF-8", nsDependentString(recipients), 
>+                                      recipientsStr)))
>+        LossyCopyUTF16toASCII(recipients, recipientsStr);
Except ConvertFromUnicode("UTF-8", nsDependentString(recipients),
recipientsStr) is CopyUTF16toUTF8(nsDependentString(recipients),
recipientsStr); return NS_OK; so this is just the same as
CopyUTF16toUTF8(nsDependentString(recipients), recipientsStr); on its own.

>+        NS_NAMED_LITERAL_STRING(emptyUnichar, " ");
I don't think that space is correct.

>-        ConvertToUnicode(val.get(), uniStr);                  \
>-        pdb->func(dbRow, NS_ConvertUCS2toUTF8(uniStr).get()); \
>+        NS_CopyNativeToUnicode(val, uniStr);                  \
>+        pdb->func(dbRow, NS_ConvertUTF16toUTF8(uniStr).get()); \
Nit: \s should line up.

>-  nsXPIDLCString escapedName;
>-  rv = NS_MsgEscapeEncodeURLPath(newsgroupName.get(), getter_Copies(escapedName));
>-  if (NS_FAILED(rv)) return rv;
>+  nsCAutoString escapedName;
>+  rv = NS_MsgEscapeEncodeURLPath(newsgroupName, escapedName);
>+  NS_ENSURE_SUCCESS(rv, rv);
You do know that NS_ENSURE_SUCCESS asserts, or warns, or something...

Comment 14

[Alec Flett]

> Why not
if (NS_FAILED(nsMsgI18NCopyUTF16ToNative(NS_ConvertUTF8toUTF16(folderURI),
oldPath)))

because that is huge and hard to read :)

having temporaries like nsresults generally don't increase codesize or bloat
because the assembly ends up the same... but using them often buys you readability.

Comment 15

[[e-mail]]

Actually it was the oldPath.Assign(nativeString); I was complaining about.

Comment 16

[Jungshik Shin]

neil, thanks for your review and comments.
alecf, do you have any other comments? I made a new patch with neil's concerns
addressed, but I want to take care of yours as well before uploading a new patch.

BTW, some of them are not valid.

1. PL_strchr doesn't work with PRUnichar*. nsCRT doesn't have strchr for
PRUnichar*, either although there are PRUnichar version of strdup and str*cmp.
2. I moved it because it crosses 'goto'
 
>>+    nsCOMPtr<nsILocalFile> localFile;
>>+    nsAutoString fileName;
>Why did you move these?

3. I intentionally used PL_strdup to match PRFREE in callers. My patch included
 a note that I would have to fix callers later to avoid string copy. 
>>+      return PL_strdup(convertedStr.get());
>ToNewCString perhaps?

4. 
>>+          NS_CopyNativeToUnicode(
>>+            nsDependentCString(mFileSpec->GetNativePathCString()), path);
> mFileSpec->GetUnicodePath(path) please.

mFileSpec is not of nsIFileSpec but nsFileSpec (don't ask me why :-)). The
latter doesn't inherit nsIFileSpec and doesn't have GetUnicodePath. I thought it
does and changed all of them to use GetUnicodePath only to find later that it
doesn't. We have to get rid of both nsIFileSpec and nsFileSpec anyway as you
know well. 

5. As for SetOrganization and other similar setters, let me take care of them in
next round (all my patch did about them was to switch from wstring to AString).
The patch is already huge, isn't it?

Note to myself: I found that I had missed yet another 'custom charset converter'
in mailnews, SystemString(to|from)Unicode in nsIImportService.idl. I'll get rid
of them in next round. 

Comment 17

[timeless]

> 3. I intentionally used PL_strdup to match PRFREE in callers. 
that's not a match.


144  * PL_strfree
145  *
146  * Free memory allocated by PL_strdup

Comment 18

[[e-mail]]

Thanks for pointing out my errors 1, 3 and 4; but I don't still understand the
point of moving the declarations, unless it's to fix a post-gcc-2.95 compiler
warning perhaps?

I really agree with point 5 though :-)

Comment 19

[Jungshik Shin]

Re: point #2: I forgot what it was, Either it's VC++ 6 that refused to compile
with goto crossing variable declarations or it's  to avoid g++ 3.x warning as
you wrote. 

re: comment #17: thanks for pointing that out. PL_strdup was used in the current
code and hasn't caused a problem so far (well, who knows?). I can hunt down
callers an replace PRFree with PL_strfree, but I'd rather  get rid of explicit
alloc/free altogether in next round. 

Comment 20

[Alec Flett]

I'll try to look at this today.

Comment 21

[Alec Flett]

sorry, I will try to get to this again on monday, Feb 2nd.

Comment 22

[Jungshik Shin]

jst, jag, or peterv, would you take a look? alecf seems tied up. I wanna check
this in before it's too late. There are a lot of clean-ups to do after this.

Comment 23

[Jungshik Shin]

Attachment: update

To my relief, attachment 139351 [details] [diff] [review] hasn't gotten obsolete much. There were only
two rejects which were trivial to fix. I addressed Neil's concerns. 
Let's move this on !

BTW, I'll remove 'mailnews/base/util/nsMsgUtf7Utils.(cpp|h). 
The patch cuts down about 460 lines of code.

Comment 24

[Jungshik Shin]

Comment on attachment 139351 [details] [diff] [review]
update 

got rid of obsolete sr request

Comment 25

[Jungshik Shin]

Comment on attachment 141457 [details] [diff] [review]
update

asking for r/sr.

Comment 26

[[e-mail]]

Comment on attachment 141457 [details] [diff] [review]
update

>+#include "nsAutoPtr.h"
I don't see you using this...

>+    nsAutoString leafName;
>     if (mShowFullName) {
>-	rv = NS_MsgDecodeUnescapeURLPath(path, aLeafName);
>+       rv = NS_MsgDecodeUnescapeURLPath(nsDependentCString(path), leafName);
>     }
>     else {
>-        rv = CreateUnicodeStringFromUtf7(node->name, aLeafName);
>+        rv = CopyMUTF7toUTF16(nsDependentCString(node->name), leafName);
>+    }
>+    if (NS_SUCCEEDED(rv)) {
>+        *aLeafName = ToNewUnicode(leafName);
>+        NS_ENSURE_TRUE(*aLeafName, NS_ERROR_OUT_OF_MEMORY);
I don't suppose we can change aLeafName to an nsAString&
or better still, remove it - who calls it anyway?

>Index: mailnews/base/util/nsMsgI18N.cpp
My eyes glazed over this file... does it look better with -w?

>+  nsCAutoString cString;
>+  CopyUTF16toUTF8(value, cString);
>+  nsresult rv = SetAsciiHeader(header, cString.get());
How about
return SetAsciiHeader(header, NS_ConvertUTF16toUTF8(value).get());

>+  CopyUTF8toUTF16(GetBody(), _retval);
Should use m_body to save the dependent c string.

>+          if (!emailAddressOnly)
>+            rv = parser->MakeFullAddress("UTF-8", pNames,
>+                                         pAddresses, getter_Copies(fullAddress));
>+          if (NS_SUCCEEDED(rv) && !emailAddressOnly)
>+          {
>+            rv = ConvertToUnicode("UTF-8", fullAddress, recipient);
>+          }
>+          else
>+            rv = ConvertToUnicode("UTF-8", nsDependentCString(pAddresses), recipient);
>+          if (NS_FAILED(rv))
>+            break;
I would have suggested this, but you need to check rv, don't you...
if (!emailAddressOnly && NS_SUCCEEDED(parser->MakeFullAddress(...)))
  CopyUTF8toUTF16(fullAddress, recipient);
else
  CopyUTF8toUTF16(pAddresses, recipient);

>+			if (NS_FAILED(ConvertFromUnicode("UTF-8", nsDependentString(recipients), 
>+                                       recipientsStr)))
>+        LossyCopyUTF16toASCII(recipients, recipientsStr);
Surely CopyUTF16toUTF8(recipients, recipientStr); can never fail?

>-        PRUnichar emptyUnichar = 0;
>+        NS_NAMED_LITERAL_STRING(emptyUnichar, "");
We have EmptyString(); now.

>-  if (NS_FAILED(rv)) return rv;
>+  NS_ENSURE_SUCCESS(rv, rv);
Don't change this, NS_ENSURE_SUCCESS asserts.

Comment 27

[Jungshik Shin]

Thanks for review. 

1. nsAutoPtr.h is necessary for nsAutoArrayPtr.
2.  
> I don't suppose we can change aLeafName to an nsAString&
> or better still, remove it - who calls it anyway?

  Needless to say, I don't like having to use ToNewUnicode at all. Let me take
care of  it (and a few callers I've just found) in next phase.

3. '-w diff' for nsMsgI18N.cpp wouldn't improve the readability. It's not white
spaces but a bulk of lines taken out that make it hard to read that part of the
patch. (those lines with tabs would have shown up anyway in the patch because
they're eliminated). If you want, I'll just upload a patched version of
nsMsgI18N.cpp

4.
> I would have suggested this, but you need to check rv, don't you..

 Yeah. I kept 'rv-checking' (in UTF8 -> UTF-16 conversion) unless it's very
clear that I don't have to. 

 I've just taken care of other problems in my tree.

Comment 28

[[e-mail]]

Comment on attachment 141457 [details] [diff] [review]
update

OK, I unreplaced rv with res and ran diff -w which cleaned things up.

>-nsresult nsMsgI18NConvertFromUnicode(const nsAFlatCString& aCharset, 
>-                                     const nsAFlatString& inString,
>+nsresult nsMsgI18NConvertFromUnicode(const char* aCharset,
>+                                     const nsAString& inString,
Your new callers can't guarantee flat strings? (As it happens, darin's proposed
string changes will flatten all strings, so it doesn't matter).

>-  nsresult res;
>-  nsCOMPtr <nsICharsetConverterManager> ccm = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &res);
>-  if(NS_SUCCEEDED(res)) {
>-    nsCOMPtr <nsIUnicodeEncoder> encoder;
>+  nsresult rv;
>+  nsCOMPtr <nsICharsetConverterManager> ccm = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
I assume you fixed this from my previous comment about NS_ENSURE_SUCCESS.

>+  const nsAFlatString &inStr = PromiseFlatString(inString);
Ugh! (But see above).

>+  if (NS_FAILED(rv)) {
This looks wrong, I can't see what it relates to in the old code.

Comment 29

[Jungshik Shin]

(In reply to comment #28)

> Your new callers can't guarantee flat strings? 

 No. 

>>+  nsCOMPtr <nsICharsetConverterManager> ccm =
do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> I assume you fixed this from my previous comment about NS_ENSURE_SUCCESS.

  Not all assertions are bad. We need to assert here because the failure here
means something has gone terribly wrong.
 

> >+  if (NS_FAILED(rv)) {
> This looks wrong, I can't see what it relates to in the old code.

  That's added. Anyway, you're right. I shouldn't have added it although in
practice, it wouldn't matter most of time. 

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 141457 [details] [diff] [review]
update

I started looking at this, and I did like what I saw, but this is a rather
large change with some potentially rather scary implications if any of this
goes wrong. Given that I know little about mailnews internals, I talked to
mscott about this and he agreed to sr this change. Sorry to push this off to a
different sr yet again, but I feel it's for the better.

Comment 31

[[e-mail]]

(In reply to comment #29)
>(In reply to comment #28)
>>Your new callers can't guarantee flat strings? 
>No. 
Well, I can't see one that can't.
What I did notice was that in some cases you used an nsXPIDLCString as the
destination, while in other cases you used an nsCAutoString... surely you should
use nsCAutoString all the time?

>+  nsXPIDLCString folderCName;
>+  LossyAppendUTF16toASCII(aName, folderCName);
NS_LossyConvertUTF16toASCII folderCName(aName); ?
Mind you, that method is really ugly, why are we passing zero-extended ascii
around anyway... more for your next patch ;-)

Comment 32

[[e-mail]]

Comment on attachment 141457 [details] [diff] [review]
update

It might not have been true before, but apparently nsACString is now
single-fragment, so your en/decoder doesn't need to PromiseFlatString.

Comment 33

[Jungshik Shin]

(In reply to comment #31)
> (In reply to comment #29)
> >(In reply to comment #28)
> >>Your new callers can't guarantee flat strings? 
> >No. 
> Well, I can't see one that can't.

  You're right. I stand corrected. I thought there were a couple of cases that
need to use non-flat strings.

> What I did notice was that in some cases you used an nsXPIDLCString as the
> destination, while in other cases you used an nsCAutoString... surely you 
> should use nsCAutoString all the time?

  I changed all those cases to nsCAutoString, but affected files didn't get
compiled because back I was using nsXPIDLCString to use 'string' instead of
'staring.get()'  when calling |nsEscape(const char*,...)|. Perhaps, that's a
pretty lame reason to prefer nsXPIDLCString to nsCAutoString, isn't it? 

> >+  nsXPIDLCString folderCName;
> >+  LossyAppendUTF16toASCII(aName, folderCName);
> NS_LossyConvertUTF16toASCII folderCName(aName); ?

  thanks for the catch.

> Mind you, that method is really ugly, why are we passing zero-extended ascii
> around anyway... more for your next patch ;-)

  Yeah, that's ugly. A part of the reason may be that it's an implementation of
an interface with 'wstring' parameter. Other implementations of the interface
need 'wstring' (genuine).
  
> so your en/decoder doesn't need to PromiseFlatString.

   That's what I thought, but I can't call 'get()' on nsA(C)String. Neither can
I call 'get()' on ns(C)Substring. I ended up replacing nsA(C)String with
nsAFlat(C)String, which is just typedef'd ns(C)String in the new world.

I'll upload a new patch after building TB with my tree updated.

Comment 34

[Jungshik Shin]

Attachment: update

I took care of issues caught/raised by Neil. Recent string changes had little
effect, but some parts of my patch may have to be reconsidered.

Comment 35

[Jungshik Shin]

Comment on attachment 141811 [details] [diff] [review]
update

asking for r/sr

Comment 36

[Johnny Stenback (:jst)]

(In reply to comment #28)
> (From update of attachment 141457 [details] [diff] [review])
[...]
> Your new callers can't guarantee flat strings? (As it happens, darin's proposed
> string changes will flatten all strings, so it doesn't matter).

Well, darin's changes makes all internal strings be flat, but that doesn't mean
that all nsA[C]Strings will be flat. If they're coming from outside callers
(i.e. embedders) they may not be flat. So you can't assume that all nsA[C]String
objects are flat string. And besides, there's no .get() on nsA[C]String anyways,
so PromiseFlatString() is still needed.

Comment 37

[Jungshik Shin]

jst, which is better with darin's change?

a. 
  foo(const nsAString& s)
  {
     const nsPromiseFlatString& fs = PromiseFlatString(s);
     PRUnichar *buffer = fs.get();
     ......
   }

b.
   foo(const nsAFlatString& s)
   {
     PRUnichar *buffer = s.get();
     ......
   }

Because all my callers call foo with flat strings in mailnews, I went with 'b'
as suggested by neil. In the past, I was sure 'b' is better as long as my
callers always call with flat strings, but now I'm a bit confused.

Comment 38

[Jungshik Shin]

(In reply to comment #37)

oops.
>      const nsPromiseFlatString& fs = PromiseFlatString(s);

   s/nsPromiseFlatString/nsAFlatString/

Comment 39

[Johnny Stenback (:jst)]

If you can use flat strings, use them. PromiseFlatString() isn't free, so if you
can avoid it, avoid it.

Comment 40

[Jungshik Shin]

thanks, jst. That's what I did in the latest patch.

BTW, with the patch, I got a lot of assertions in nsCharsetAliasImpl.cpp :

###!!! ASSERTION: nsCharsetAlias2 not thread-safe: '_mOwningThread.GetThread() =
= PR_GetCurrentThread()', mozilla/intl/uconv/src/nsCharsetAliasImp.cpp, line 54

Using NS_IMPL_THREADSAFE_SUPPORT1 in place of NS_IMPL_SUPPORT1 (in
nsCharsetAliasImp.cpp) solved the problem, but I'm wondering if there's a better
way and I'm doing something wrong in my patch.

Although not as often as in nsCharsetAliasImp.cpp, I got the same assertion at
netwerk/cache/src/nsCacheSession.cpp, line 30

Comment 41

[Boris Zbarsky [:bzbarsky]]

> Using NS_IMPL_THREADSAFE_SUPPORT1 in place of NS_IMPL_SUPPORT1

That'll shut up the assertion, but you need to make sure it actually _is_
threadsafe too.

Comment 42

[[e-mail]]

Comment on attachment 141811 [details] [diff] [review]
update

>-NS_MSG_BASE nsresult NS_MsgEscapeEncodeURLPath(const PRUnichar *str, char **result);
>+NS_MSG_BASE nsresult NS_MsgEscapeEncodeURLPath(const nsAString& str, nsACString& result);

>-NS_MSG_BASE nsresult NS_MsgDecodeUnescapeURLPath(const char *path, PRUnichar **result);
>+NS_MSG_BASE nsresult NS_MsgDecodeUnescapeURLPath(const nsACString& path, nsAString& result);
Unfortunately these declarations don't match up with the definitions...

>-  nsAutoString empty;
>-  nsAutoString bodStr(bod);
>   nsAutoString tSignature;
> 
>   if (addSignature)
>     ProcessSignature(m_identity, PR_FALSE, &tSignature);
> 
>-  rv = ConvertAndLoadComposeWindow(empty, bodStr, tSignature,
>+  rv = ConvertAndLoadComposeWindow(NS_CONST_CAST(nsString&, EmptyString()),
>+                                   body, tSignature,
>                                    PR_FALSE, m_composeHTML);
I don't like that const cast, please can you use new nsString() or nsString
empty; or something like that?

Comment 43

[Jungshik Shin]

(In reply to comment #42)
> (From update of attachment 141811 [details] [diff] [review])

> >+NS_MSG_BASE nsresult NS_MsgEscapeEncodeURLPath(const nsAString& str,
nsACString& result);

This does match the definition.  

> >+NS_MSG_BASE nsresult NS_MsgDecodeUnescapeURLPath(const nsACString& path,
nsAString& result);
> Unfortunately these declarations don't match up with the definitions...

I've already fixed this mismatch in my tree. Otherwise, how could I have
compiled and tested my patch? ;-)

> >-  nsAutoString empty;

> >+  rv = ConvertAndLoadComposeWindow(NS_CONST_CAST(nsString&, EmptyString()),
> >+                                   body, tSignature,
> >                                    PR_FALSE, m_composeHTML);
> I don't like that const cast, please can you use new nsString() or nsString
> empty; or something like that?

  I did check |Conver...Window| doesn't touch it if it's empty. Nonetheless, I
agree that it's better and safer to do what you suggested than to cast away
const. Did you find any other issues (the latest patch is almost identical to
the second latest and the third latest)? 

re: comment #41

  bz, thanks. I guess I can make it thread-safe, but wouldn't that slow it down?
I'm wondering what in my patch introduced 'cross-threading' issuses (my use of
nsXPIDLString?). What are 'usual suspects' in similar(?) cases? 

Comment 44

[Boris Zbarsky [:bzbarsky]]

jshin, no idea what in your patch triggered it.  Usually, I would assume that
it's in fact being addrefed/released on a thread different from the thread it's
created on.  I'd set a breakpoint on the assert and see what the stack looks like.

Comment 45

[Jungshik Shin]

Attachment: patch to avoid cross-thread issues (nsMsgI18N.cpp/h)

Here, I'm special-casing 'modified UTF-7' used in the IMAP protocol so that
nsCharsetAlias is not invoked for 'modified UTF-7'. 

In the current trunk code,  GetUnicode(En|De)coderRaw is used with
'x-imap4-modified-utf7' because 'x-imap4-modified-utf7' is the canonical name
for 'modified UTF-7'. My patch got rid of separate functions for 'modified
UTF-7' and consolidated them with nsMsgConvert(From|To)Unicode which call
GetUnicode(En|De)coder. The 'raw' version bypasses the alias resolution while
the ordinary version (used for 'modified UTF-7' as well as other charset names
with my patch) goes through the alias resolution, which triggers the assertion
because the IMAP access is multi-threaded.

Comment 46

[Jungshik Shin]

Attachment: make nsCharsetAliasImp.cpp thread-safe

This is not necessary to fix this bug but we might have to do it anyway.

Comment 47

[Jungshik Shin]

Comment on attachment 141948 [details] [diff] [review]
patch to avoid cross-thread issues (nsMsgI18N.cpp/h)

nsMsgI18N.cpp and nsMsgI18N.h are identical to those in attachment 141949 [details] [diff] [review]
except that nsMsgI18NConvert(to|from)Unicode have the optional 4th argument
(aIsCharsetCaononical). When it's set, GetUnicode(En|De)coderRaw is called
instead of GetUnicode(En|De)Coder to bypass the charset alias resolution. It's
only used for 'modified UTF-7' for IMAP.

Neil, if you don't have any other issues with attachment 141949 [details] [diff] [review] (than  two you
already mentioned that I fixed in my tree), can you give r to this and
attachment 141949 [details] [diff] [review]?

Comment 48

[[e-mail]]

Comment on attachment 141948 [details] [diff] [review]
patch to avoid cross-thread issues (nsMsgI18N.cpp/h)

It would have been quicker if I had had a working patch that applied first
time...

Comment 49

[Jungshik Shin]

(In reply to comment #48)
> It would have been quicker if I had had a working patch that applied first

 Sorry I wasted your time and thank you for your review. I didn't expect you to
apply the patch and the mismatch between the declaration and the definition was
so obvious that I forgot to tell you that it's fixed locally after uploading it. 

 Anyway, in comment #47, I meant attachment 141811 [details] [diff] [review] (not attachment 141949 [details] [diff] [review]).
Unless you have other issues, can you grant me 'r' on attachment 141811 [details] [diff] [review]? 

Comment 50

[[e-mail]]

Reviewers are supposed to test patches... as for attachment 141811 [details] [diff] [review], I have no
more issues.

Comment 51

[timeless]

does this help
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsDirectoryIndexStream.cpp#80

Comment 52

[Jungshik Shin]

Thanks. bug 99382 helped me better understand the nature of the problem. 

Comment 53

[Jungshik Shin]

Comment on attachment 141949 [details] [diff] [review]
make nsCharsetAliasImp.cpp thread-safe

I'm wondering if I'm doing the right thing here. Perhaps, I can  'lock around'
a smaller section.

Comment 54

[Jungshik Shin]

Comment on attachment 141811 [details] [diff] [review]
update

neil, can you turn r- to r+ here? I've locally replaced |const nsACString| with
|const nsASingleFragmentString| in both definition and the declaration of
|NS_MsgDecodeUnescapeURLPath|
I also fixed  'EmptyString()' issue.

Comment 55

[[e-mail]]

Comment on attachment 141811 [details] [diff] [review]
update

Yeah, r=me with the various provisos noted.

Comment 56

[timeless]

Comment on attachment 141949 [details] [diff] [review]
make nsCharsetAliasImp.cpp thread-safe

>Index: intl/uconv/src/nsCharsetAliasImp.cpp

> nsCharsetAlias2::nsCharsetAlias2()
> {
>   mDelegate = nsnull; // delay the load of mDelegate untill we need it.
>+  mLock = PR_NewLock();
Not a very good idea, an init method and making the
ns_generic_factory_constructor() thing call it, would be better

>@@ -93,23 +96,28 @@ NS_IMETHODIMP nsCharsetAlias2::GetPrefer
>+   if(!mLock)
>+     return NS_ERROR_UNEXPECTED;
not a very friendly return code. you're expecting this. either avoid this
problem by addressing my earlier comment or pick something more interesting.
(i'd suggest the former)

Lock:
>+   PR_Lock(mLock);
Locked:
>    if(!mDelegate) {
>      //load charsetalias.properties string bundle with all remaining aliases
>      mDelegate = new nsURLProperties( NS_LITERAL_CSTRING("resource://gre/res/charsetalias.properties") );
>      NS_ASSERTION(mDelegate, "cannot create nsURLProperties");
StillLocked:
>      if(nsnull == mDelegate)
>        return NS_ERROR_OUT_OF_MEMORY;
you can't do this anymore. you're holding a lock.
>    }
>+
>+   PR_Unlock(mLock);
Unlocked:

One approach would be to use a scoped nsAutoLock. (there are others...)

Comment 57

[timeless]

dumb question time. why can't the init/constructor for nsCharsetAlias do:
      mDelegate = new nsURLProperties(
NS_LITERAL_CSTRING("resource://gre/res/charsetalias.properties") );

Comment 58

[Alec Flett]

Comment on attachment 141949 [details] [diff] [review]
make nsCharsetAliasImp.cpp thread-safe

I'm with timeless - why not create mDelegate in the constructor?

In fact, why even make mDelegate a nsURLProperties* - just declare it

nsURLProperties mDelegate;

and then initialize it in the constructor:
nsCharsetAlias2::nsCharsetAlias2() : mDelegate(NS_LITERAL_CSTRING(...))  {.. }

Comment 59

[Jungshik Shin]

Thanks for your review and comment.

(In reply to comment #57)
> dumb question time. why can't the init/constructor for nsCharsetAlias do:
>       mDelegate = new nsURLProperties(
> NS_LITERAL_CSTRING("resource://gre/res/charsetalias.properties") );

Because it's lazy :-) It doesn't want to load it until it has to. I don't know
how much we're saving that way. For English and West European users, it could
never be loaded.

Comment 60

[Jungshik Shin]

(In reply to comment #58)
> (From update of attachment 141949 [details] [diff] [review])

> In fact, why even make mDelegate a nsURLProperties* - just declare it
> nsURLProperties mDelegate;

How about declaring it and then ...
 
> and then initialize it in the constructor:
> nsCharsetAlias2::nsCharsetAlias2() : mDelegate(NS_LITERAL_CSTRING(...))  {.. }

  lazyily initing it when it becomes necessary (within scoped-nsAutolock)? 

BTW, I vaguely remember seeing nsAutoLock, but lxr (identifier search) came up
empty... I should have bookmarked timeless' alternate lxr or tried text/file search.

Comment 61

[Johnny Stenback (:jst)]

xpcom/threads/nsAutoLock.h?

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #59)
> Because it's lazy :-) It doesn't want to load it until it has to. I don't know

Not only that, but if it weren't lazy, we'd probably recur into GetService and
create two instances.  See bug 190951.

Comment 63

[timeless]

actually, bz pointed out that we eventually added .properties to the list of
types. anyway, i tried building a mozilla which created nsURLProperties in the
charsetalias constructor and nothing bad happened.

bz and i did a bit more research and decided that the construction of the
urlproperties isn't dangerous, in fact stringbundles are lazyloaded (thanks
alecf - bug 76944), so they aren't much of a problem.

the problem that we found was that stringbundles aren't threadsafe. (Ok, they
claim to be threadsafe, but they aren't.) - I'll cover this in bug 31790.

Comment 64

[Boris Zbarsky [:bzbarsky]]

> actually, bz pointed out that we eventually added .properties to the list of
> types.

Er, no.  I pointed out that the code that loads .properties files tells necko
the type is text/plain and not to bother looking.

Comment 65

[Jungshik Shin]

Attachment: patch (attachment 141811 + 141948 + update to the trunk)

This patch is basically attachment 141811 [details] [diff] [review] + attachment 141948 [details] [diff] [review] + two fixes
suggested in comment #42 (directly using nsString() didn't work with gcc 3.x so
that I used 'nsString emtpy'). In addition, there are a couple of changes in
nsMsgI18N.cpp (because dbaron landed recently to get rid of casting away const
in |StringClass.get()|).

Comment 66

[Jungshik Shin]

Comment on attachment 142682 [details] [diff] [review]
patch (attachment 141811 [details] [diff] [review] + 141948 + update to the trunk)

neil, can you carry over your r's to previous patches here? 
mscott, can you take some time to review this? 

thanks.

Comment 67

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

(In reply to comment #65)
> (directly using nsString() didn't work with gcc 3.x so
> that I used 'nsString emtpy'). 

use EmptyString()

Comment 68

[Jungshik Shin]

No, I can't. see comment #42.

Comment 69

[Darin Fisher]

>+  rv = ConvertAndLoadComposeWindow(NS_CONST_CAST(nsString&, EmptyString()),

EmptyString returns |const nsAFlatString&|, which is actually equivalent to
|const nsString&|.  you cannot cast away the const since the function may try to
modify the string object.  creating a temporary nsString object should do the
trick.  

it's a little bit odd that that function may, as a side effect, modify its input
arguments.

Comment 70

[[e-mail]]

Comment on attachment 142682 [details] [diff] [review]
patch (attachment 141811 [details] [diff] [review] + 141948 + update to the trunk)

(In reply to comment #65)
>(directly using nsString() didn't work with gcc 3.x so
>that I used 'nsString emtpy').
I don't see that in the patch... r=me with that in.

Comment 71

[Scott MacGregor]

Comment on attachment 142682 [details] [diff] [review]
patch (attachment 141811 [details] [diff] [review] + 141948 + update to the trunk)

Sorry it took so long. This was a big patch to go through!

Just some minor nits, in nsOutlookCompose.cpp you turned some nsStrings into
nsAutoStrings for the message body. Most message bodies are going to be larger
than the buffer allocated for the auto string so they should really remain
nsString to avoid extra work:

nsAutoString	uniBody;
nsCAutoString body;

Nice work jshin

Comment 72

[Jungshik Shin]

Comment on attachment 142682 [details] [diff] [review]
patch (attachment 141811 [details] [diff] [review] + 141948 + update to the trunk)

asking for a1.7 (I addressed mscott's concern locally. I also made the
corresponding change in importing code for Outlook(Express)

risk : a bit high.
affected platforms : all
affected users : all mailnews users

Ideally, it'd have been better if this had gotten in for 1.7beta.
Unfortunately, it missed it.My test-run so far has been smooth (I've been 
using Mozilla trunk + this patch for two days.) Needless to say, that doesn't
guarantee that there's no problem. 

I haven't measured the size, but this patch will cut down the code size rather
much. We can boast of higher code size reduction in 1.7 release notes :-)

If this gets approved, the earlier the better. If it's deemed too risky, well,
I have to wait for 1.8alpha cycle to begin.

Comment 73

[chris hofmann]

Comment on attachment 142682 [details] [diff] [review]
patch (attachment 141811 [details] [diff] [review] + 141948 + update to the trunk)

let's do this early in 1.8.  it would also be good to post specfics to the bug
that tell about the amount of the improvment in code size.  great contribution!
 thanks for working on this.

Comment 74

[WADA:World Anti-bad-Duping Agency]

Bug 240558 (regression of fix for bug 229345) seems to be caused by patch
checked-in on 4/12 by this bug.

(1) nsMsgUtils.cpp is the fixed source by bug 229345.
(2) Change histry of nsMsgUtils.cpp by Bonsai ;
    2004-01-07 08:04 by bug 229345
    2004-04-12 21:07 by bug 229032	 
(3) Problem occurs on 2004-04-14 build but no prblem on 2004-04-06 build.
 
Jungshik Shin, have you checked in some changes on nsMsgUtils.cpp?

Comment 75

[Simon Montagu :smontagu]

This was checked in, so marking FIXED.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fmailnews&file=&filetype=match&who=jshin%25mailaps.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-04-12&maxdate=2004-04-13&cvsroot=%2Fcvsroot