43221 - POP mail search by Body doesn't work if the key string contains non-ASCII chars.

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect, P3)

Description

[ji]

Build: 2000062009 commercial

Steps of reproduce:
1. Unzip the smoketest folder shown at the above URL and put it in your mail 
directory.
2. Lauch Mail.
3. Select Search | Search All Mail
4. Select the smoketest folder as the target folder
5. Select Body from the search list.
6. Enter a non-ASCII string in the key string text field which is contained in 
the mail body of a mail in the smoketest folder.
7. Click on Search button, you'll see the mail which matches the search criteria 
is not listed.

Comment 1

[Alec Flett]

is this the same as bug 43221?
did you mean to file two duplicate bugs?

Comment 2

[ji]

Did you mean 42219?

42219 is for MIME header, and this one is for body.

Comment 3

[nhottanscp]

Adding taka and jaimejr to cc.

Comment 4

[Jaime Rodriguez, Jr.]

Please nominate for NSBeta2+. Need to have search functionality in parity to 
Seamonkey US EN in the JA client, and v4.x. This is not a feature, it is a 
major bug that needs to be fixed.

Naoki, please nominate this bug as nsbeta2.

Comment 5

[nhottanscp]

Nominating for nsbeta2.

Comment 6

[Alec Flett]

I think this is alot of work for nsbeta2, and would recommend waiting on this 
one.
Nominate for nsbeta3 because I think it should be in RTM.

Comment 7

[Jay Patel [:jay]]

Putting on [nsbeta2-] radar. Not critical to beta2.  Adding "nsbeta3" keyword 
for consideration of a fix for that milestone. 

Comment 8

[Alec Flett]

I would like to say that I don't think this is an exception feature. I think 
this is just a bug in the way we do body searches.

Comment 9

[selmer (gone)]

Adding mail4 triage nomination keyword.

Comment 10

[Katsuhiko Momoi]

Per i18n/mail triage meeting, this bug is marked nsbeta3+.

Comment 11

[Katsuhiko Momoi]

ji, please keep track of this one.

Comment 12

[lchiang]

second pass: - per mail triage.

Comment 13

[Jaime Rodriguez, Jr.]

If you could please include us as part of the process when "cutting" bugs that
we have all previously agreed should be plus, it would be greatly appreciated. I
know, tough calls have to be made to hold the right glide, but we'd like to be
part of the discussion, when it comes to cutting bugs that affect I18N users.

Actions like this are deflating to our team, when we spent time to traige,
nominate, and gain consensus with MailNews on the "bare minimum" ( i think it
was 6 bugs) that needed to be fixed. <]8^(

Comment 14

[scottputterman]

Sorry for the extra mail. Removing mail4 keyword.

Comment 15

[nhottanscp]

Alec, can taka take this bug?

Comment 16

[Alec Flett]

search backend->bienvenu

Comment 17

[nhottanscp]

As I mentioned, taka can work on this (he fixed local header search before).
David, is that okay to assign this to taka?

Comment 18

[David :Bienvenu]

yes, no problem. I'll reassign it.

Comment 19

[Takayuki Tei]

Index: nsMsgSearchTerm.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v
retrieving revision 1.55
diff -u -r1.55 nsMsgSearchTerm.cpp
--- nsMsgSearchTerm.cpp	2000/08/16 23:19:51	1.55
+++ nsMsgSearchTerm.cpp	2001/01/05 07:18:26
@@ -701,14 +701,6 @@
 		PRBool endOfFile = PR_FALSE;  // if retValue == 0, we've hit 
the end of the file
 		uint32 lines = 0;
 
-		PRBool getConverter = PR_FALSE;
-#ifdef DO_I18N
-		CCCDataObject conv = INTL_CreateCharCodeConverter();
-		PRInt16 win_csid = INTL_DocToWinCharSetID(foldcsid);
-		PRInt16 mail_csid = INTL_DefaultMailCharSetID(win_csid);    // 
to default mail_csid (e.g. JIS for Japanese)
-		if ((nsnull != conv) && INTL_GetCharCodeConverter(mail_csid, 
win_csid, conv)) 
-			getConverter = PR_TRUE;
-#endif // DO_I18N
 		// Change the sense of the loop so we don't bail out 
prematurely
 		// on negative terms. i.e. opDoesntContain must look at all 
lines
 		PRBool boolContinueLoop;
@@ -733,27 +725,13 @@
 				// Do in-place decoding of quoted printable
 				if (isQuotedPrintable)
 					StripQuotedPrintable ((unsigned 
char*)buf);
-
-				nsCString compare(buf);
-				if (getConverter) 
-				{	
-#ifdef DO_I18N
-					// In here we do I18N conversion if we 
get the converter
-					char *newBody = nsnull;
-					newBody = (char 
*)INTL_CallCharCodeConverter(conv, (unsigned char *) buf, (int32) 
PL_strlen(buf));
-					if (newBody && (newBody != buf))
-					{
-						// CharCodeConverter return the 
char* to the orginal string
-						// we don't want to free body 
in that case 
-						compare = newBody;
-					}
-#endif
-				}
+			    nsCString  compare(buf);
+//				ConvertToUnicode(charset, buf, compare);
 				if (compare.Length() > 0) {
 					char startChar = (char) 
compare.CharAt(0);
 					if (startChar != CR && startChar != LF)
 					{
-						err = MatchString (compare, 
nsnull, &result);
+						err = MatchString (compare, 
folderCharset, &result);
 						lines++; 
 					}
 				}
@@ -816,75 +794,80 @@
 
 	nsresult err = NS_OK;
 	nsCAutoString n_str;
-	const char* n_header = nsnull;
+	const char *utf8 = stringToMatch;
 	if(nsMsgSearchOp::IsEmpty != m_operator)	// Save some 
performance for opIsEmpty
 	{
-		n_header = stringToMatch;
 		n_str = m_value.string;
+		nsAutoString  srcCharset;
+		srcCharset.AssignWithConversion(charset);
+		nsString out;
+		ConvertToUnicode(srcCharset, stringToMatch ? stringToMatch : 
"", out);
+		utf8 = out.ToNewUTF8String();
 	}
+
 	switch (m_operator)
 	{
 	case nsMsgSearchOp::Contains:
-		if ((nsnull != n_header) && ((n_str.GetBuffer())[0]) && /* 
INTL_StrContains(csid, n_header, n_str) */
-			PL_strcasestr(stringToMatch, n_str))
+		if ((nsnull != utf8) && ((n_str.GetBuffer())[0]) && /* 
INTL_StrContains(csid, n_header, n_str) */
+			PL_strcasestr(utf8, n_str))
 			result = PR_TRUE;
 		break;
 	case nsMsgSearchOp::DoesntContain:
-		if ((nsnull != n_header) && ((n_str.GetBuffer())[0]) &&  /* 
!INTL_StrContains(csid, n_header, n_str) */
-			!PL_strcasestr(stringToMatch, n_str))
+		if ((nsnull != utf8) && ((n_str.GetBuffer())[0]) &&  /* 
!INTL_StrContains(csid, n_header, n_str) */
+			!PL_strcasestr(utf8, n_str))
 			result = PR_TRUE;
 		break;
 	case nsMsgSearchOp::Is:
-		if(n_header)
+		if(utf8)
 		{
 			if ((n_str.GetBuffer())[0])
 			{
-				if (n_str.EqualsWithConversion(stringToMatch, 
PR_TRUE /*ignore case*/) /* INTL_StrIs(csid, n_header, n_str)*/ )
+				if (n_str.EqualsWithConversion(utf8, PR_TRUE 
/*ignore case*/) /* INTL_StrIs(csid, n_header, n_str)*/ )
 					result = PR_TRUE;
 			}
-			else if (n_header[0] == '\0') // Special case for "is 
<the empty string>"
+			else if (utf8[0] == '\0') // Special case for "is <the 
empty string>"
 				result = PR_TRUE;
 		}
 		break;
 	case nsMsgSearchOp::Isnt:
-		if(n_header)
+		if(utf8)
 		{
 			if ((n_str.GetBuffer())[0])
 			{
-				if (!n_str.EqualsWithConversion(stringToMatch, 
PR_TRUE)/* INTL_StrIs(csid, n_header, n_str)*/ )
+				if (!n_str.EqualsWithConversion(utf8, 
PR_TRUE)/* INTL_StrIs(csid, n_header, n_str)*/ )
 					result = PR_TRUE;
 			}
-			else if (n_header[0] != '\0') // Special case for 
"isn't <the empty string>"
+			else if (utf8[0] != '\0') // Special case for "isn't 
<the empty string>"
 				result = PR_TRUE;
 		}
 		break;
 	case nsMsgSearchOp::IsEmpty:
-		if (!PL_strlen(stringToMatch))
+		if (!PL_strlen(utf8))
 			result = PR_TRUE;
 		break;
 	case nsMsgSearchOp::BeginsWith:
 #ifdef DO_I18N_YET
-		if((nsnull != n_str) && (nsnull != n_header) && 
INTL_StrBeginWith(csid, n_header, n_str))
+		if((nsnull != n_str) && (nsnull != utf8) && 
INTL_StrBeginWith(csid, utf8, n_str))
 			result = PR_TRUE;
 #else
 		// ### DMB - not the  most efficient way to do this.
-		if (PL_strncmp(stringToMatch, n_str, PL_strlen(n_str)) == 0)
+		if (PL_strncmp(utf8, n_str, PL_strlen(n_str)) == 0)
 			result = PR_TRUE;
 #endif
 		break;
 	case nsMsgSearchOp::EndsWith: 
     {
-      PRUint32 searchStrLen = (PRUint32) PL_strlen(stringToMatch);
+      PRUint32 searchStrLen = (PRUint32) PL_strlen(utf8);
       if (n_str.Length() <= searchStrLen)
       {
         PRInt32 sourceStrOffset = searchStrLen - n_str.Length();
-        if (PL_strcmp(stringToMatch + sourceStrOffset, n_str) == 0)
+        if (PL_strcmp(utf8 + sourceStrOffset, n_str) == 0)
           result = PR_TRUE;
       }
     }
 #ifdef DO_I18N_YET
 		{
-		if((nsnull != n_str) && (nsnull != n_header) && 
INTL_StrEndWith(csid, n_header, n_str))
+		if((nsnull != n_str) && (nsnull != utf8) && 
INTL_StrEndWith(csid, utf8, n_str))
 			result = PR_TRUE;
 		}
 #else
@@ -894,6 +877,9 @@
 	default:
 		NS_ASSERTION(PR_FALSE, "invalid operator matching search 
results");
 	}
+
+	if (utf8 != nsnull && utf8 != stringToMatch)
+	  free((void *)utf8);
 
 	*pResult = result;
 	return err;
Index: nsMsgSearchAdapter.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchAdapter.cpp,v
retrieving revision 1.27
diff -u -r1.27 nsMsgSearchAdapter.cpp
--- nsMsgSearchAdapter.cpp	2000/09/22 02:52:11	1.27
+++ nsMsgSearchAdapter.cpp	2001/01/05 07:18:28
@@ -137,73 +137,6 @@
 
 
 char *
-nsMsgSearchAdapter::TryToConvertCharset(const PRUnichar *sourceStr,
-                                        const PRUnichar * destCharset,
-                                        PRBool useMime2)
-{
-	char *result = nsnull;
-
-	if (sourceStr == nsnull) 
-		return nsnull;
-
-  // Convert from unicode to a destination charset.
-  if (NS_FAILED(ConvertFromUnicode(nsAutoString(destCharset), 
nsAutoString(sourceStr), &result))) {
-    PR_FREEIF(result);
-    result = nsnull;
-  }
-
-#ifdef DO_I18N  // I have no idea what we should do here.
-	if ((src_csid != dest_csid) || (useMime2))
-	{
-		// Need to convert. See if we can.
-
-		// ### mwelch Much of this code is lifted from 
-		//     lib/libi18n/mime2fun.c (in particular,
-		//     intl_EncodeMimePartIIStr).
-		CCCDataObject obj = nsnull;
-		CCCFunc cvtfunc = nsnull;
-		int srcLen = XP_STRLEN(sourceStr);
-
-		obj = INTL_CreateCharCodeConverter();
-		if (obj == nsnull)
-			return 0;
-
-		/* setup converter from src_csid --> dest_csid */
-	

Comment 20

[Takayuki Tei]

Do I have to ask somebody to review the code above?

Comment 21

[David :Bienvenu]

you need to get two people to review this (one of them is me). You should also
indicate what kind of testing you've done (e.g., have you tested pop mail
filters that work on message bodies)

Comment 22

[Takayuki Tei]

I've done local search for message body using Japanese string.
No filter testing is done since this was search bug.

Comment 23

[David :Bienvenu]

filters use the search backend to evaluate filter conditions. 

Comment 24

[nhottanscp]

I do not see UI for filtering by message body. Is there a way to test it?

Comment 25

[David :Bienvenu]

Were you looking at an imap server or a pop3 server? As I indicated before, but
did not emphasize(sorry), body applies to a pop3 server message filter.

Comment 26

[nhottanscp]

Sorry, I was looking at imap filters.
Taka, please test your patch for pop fileter.

Comment 27

[Takayuki Tei]

Attachment: proposed fix for local body search

Comment 28

[Takayuki Tei]

the attached patch above is a better version of previous one.
tested both body search and POP3 filter for several Japanese string.

the current code doesn't recognize MIME multi-part body.
I use nsMsgI18NGetDefaultMailCharset() if msgToMatch->GetCharset() in 
nsMsgSearchOfflineMail::MatchTerms() doesn't give any, so that
text/* in multi-part messages can be searched as well.

Naoki-san,

I believe, nsMsgI18NGetDefaultMailCharset() should read "mailnews.view_default_charset" instead of "mailnews.send_default_charset".

Comment 29

[nhottanscp]

I filed the default charset issue separately as bug 64781.

Comment 30

[nhottanscp]

Taka, please get reviewes from bienvenu and mscott to checkin.

Comment 31

[David :Bienvenu]

sr=bienvenu. While you're at it, do you think you could clean out the obsolete,
iffdeffed out i18n code, and remove the comments of the form /* INTL... */ that
reference the old routines. Now that you've fixed this (thanks a lot!, by the
way) we don't need those comments.

Comment 32

[Takayuki Tei]

Attachment: as a result of changes related #64781, I need to refresh proposed patch

Comment 33

[Takayuki Tei]

Attachment: new proposed patch using folder charset

Comment 34

[David :Bienvenu]

I'm really not happy with the Recycle((char *)) ... part of
nsMsgSearchOfflineMail::MatchTerm(). Is that really the correct useage? I've
never seen it used on nsXPIDLCStrings before, and I don't like the cast.

Other than that, it looks OK.

Comment 35

[Scott MacGregor]

Couple comments
1) why do we define const char * charset twice in ::MatchTerms?
2) the declaration of charset looks fishy here...if charset is declared as a
const char * why would it's value ever be freed? That seems very strange. It
shouldn't be const if it's something that needs freed. 
3) I'm worried about the deletion of memory for the charset string. 
It seems to be pointing inside the buffer for the XPIDLString: msgCharset. If
that's the case, then it's going to get deleted when the idl string gets
destroyed. Seems like we'd be double deleting it if you call Recycle on that
buffer as well. Or am I just confused because it's declared twice?

Comment 36

[Takayuki Tei]

Attachment: got rid of Recycle()

Comment 37

[Takayuki Tei]

attached whole nsMsgLocalSearch.cpp since diff didn't seem to work correctly.

Comment 38

[David :Bienvenu]

OK, Scott and I misread the diffs, and thought the Recycle line was in
MatchTerms, not Search. My apologies for that. However, doing that cast and
Recycle still seems wrong. again, sorry, I misread the diffs - it's impossible
to read all diffs correctly without a visual tool like pmdiff - perhaps I should
go back to using that.) sr=bienvenu.
 

Comment 39

[David :Bienvenu]

I guess this didn't get through before : sr=bienvenu

Comment 40

[nhottanscp]

> 2) the declaration of charset looks fishy here...if charset is declared as a
> const char * why would it's value ever be freed? That seems very strange. It
> shouldn't be const if it's something that needs freed.

* I have the same question as mscott, declared and freed in
nsMsgSearchOfflineMail::Search.

* Please check return value of charset conversion. Please change them like below.

+
		ConvertToUnicode(srcCharset, stringToMatch ? stringToMatch : "", out);
+
		utf8 = out.ToNewUTF8String();

err = ConvertToUnicode(srcCharset, stringToMatch ? stringToMatch : "", out);
if (NS_SUCCEEDED(err))
  utf8 = out.ToNewUTF8String();


+
		ConvertFromUnicode(nsAutoString(convertedValue), nsAutoString(destCharset), &value);

		if (value)

err = ConvertFromUnicode(nsAutoString(convertedValue),
nsAutoString(destCharset), &value);
if (NS_SUCCEEDED(err))

Comment 41

[nhottanscp]

Sorry, I did not realized that 01/18/01 20:28 attachment changed
nsMsgSearchOfflineMail::Search.
r=nhotta, please change to check return value of conversions, there are possible
conversion errors since the code reagards all the messages use folder charset,
in case of conversion error, we at least want to search ASCII strings.

Comment 42

[Takayuki Tei]

checked in.  r=nhotta, a=sr=bienvenu
mark as fixed.

Comment 43

[ji]

Verified with win32 and linux 2001-02-12-06-mtrunk builds. It is fixed. 
Problem about mail body search in a subfolder is described in bug 68630.