1516320 - Quick search cannot find mails with quoted-printable + "ISO-2022-JP" body, finds raw text, like =1B$BF|K instead

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[YUKI "Piro" Hiroshi]

Attachment: ISO-2022-JP+Quoted Printable.eml

Bug 1427645 says that Quoted-Printable body is now supported for quick search. However, Tb doesn't list QT + ISO-2022-JP mails in the search result.

Steps to reproduce:

 1. Setup the local folder account.
 2. Drag the downloaded "ISO-2022-JP+Quoted Printable.eml" to the inbox of
    the local folder account.
 3. Activate "Quick Filter".
 4. Input "日本語" into the text field.
 5. Press the "Body" button.

Actual result:

 * The imported mail is not listed in the search result.

Expected result:

 * The imported mail is listed in the search result.

Environment:

 * Daily 66.0a1 (2018-12-23) (64-bit) on Ubuntu 16.04LTS

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: UTF-8+Quoted Printable.eml

Another testcase with QT + UTF-8. Tb lists this in the search result as expected.

Comment 2

[YUKI "Piro" Hiroshi]

Attachment: Shift_JIS+Quoted Printable.eml

One more testcase with QT + Shift_JIS. Tb lists this in the search result as expected.

Comment 3

[YUKI "Piro" Hiroshi]

I've created a workaround addon for this problem:
https://addons.thunderbird.net/thunderbird/addon/search-body-in-quoted-printabl/

Comment 4

[Jorg K (CEST = GMT+2)]

ISO-2022-JP is a 7bit encoding, so it doesn't really make much sense to use CTE QP for it. Does the body search work when it's 7bit encoded? Also, your sample message appears to have an encoding error since it's displayed as:
  日本語の本文がQuoted Printableになっているもの��

I'll look into it after Christmas.

Comment 5

[Jorg K (CEST = GMT+2)]

OK, there was a state switch to ASCII missing at the end:
=1B$BF|K\8l$NK\J8$,=1B(BQuoted Printable=1B$B$K$J$C$F$$$k$b$N should be
=1B$BF|K\8l$NK\J8$,=1B(BQuoted Printable=1B$B$K$J$C$F$$$k$b$N=1B(B

If I turn the =1B back into ESC, marked as [ESC] (since BMO might strip the ESC character)
[ESC]$BF|K\8l$NK\J8$,[ESC](BQuoted Printable[ESC]$B$K$J$C$F$$$k$b$N[ESC](B
the body search actually works. So it's just the somewhat illegal QP encoding. I forgot to quote the spec in my previous comment, here it is: https://tools.ietf.org/html/rfc1468. Who creates those messages?

Comment 6

[Jorg K (CEST = GMT+2)]

=1B$BF|K is actually found in the message :-(

Comment 7

[Jorg K (CEST = GMT+2)]

The bug is clearly here:
https://searchfox.org/comm-central/rev/4c3db8aa97f726068fe1021234575b2edb445af2/mailnews/base/search/src/nsMsgSearchTerm.cpp#953
where we spin some extra processing for "stateful" charsets, well, the only one left is ISO-2022-JP.

Looking at the comments the original authors already knew that they're coding a problem :-(

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1516320-qp-iso-2022-jp.patch - v0

This documents that currently 日本 is not found, but instead =1B$BF|K

Unit test:
mach xpcshell-test comm/mailnews/base/test/unit/test_searchBody.js

I'll fix this with the next patch.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1516320-qp-iso-2022-jp.patch - v0b

Hmm, the code that breaks the QP search was added in bug 314637 but they never added a test :-( - So here we have one.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1516320-qp-iso-2022-jp.patch (v1)

Here we go, all tests still pass, but now with the correct result. First reviewer will win.

Nice exercise for Boxing Day ;-) - I fail to understand why bug 314637 added to the mess instead of implementing the dead-simple fix I have here. Yes, body search doesn't have a MIME parser, but it already does sufficient parsing that is can pick up the CTE which it's looking at headers anyway.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9033350 [details] [diff] [review]
1516320-qp-iso-2022-jp.patch (v1)

If this is winning I'd hate to not be winning…

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9211c27bcce1
Fix body search for QP-encoded messages using ISO-2022-JP. r=darktrojan

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033350 [details] [diff] [review]
1516320-qp-iso-2022-jp.patch (v1)

It was about time this got fixed properly.

Comment 14

[:aceman]

Comment on attachment 9033350 [details] [diff] [review]
1516320-qp-iso-2022-jp.patch (v1)

Review of attachment 9033350 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right conceptually. If we can see the encoding header then we should use it, why (wrongly) guess everything is QP.

::: mailnews/base/search/src/nsMsgBodyHandler.cpp
@@ +389,5 @@
>    nsCString lowerCaseLine(line);
>    ToLowerCase(lowerCaseLine);
>  
> +  if (StringBeginsWith(lowerCaseLine, NS_LITERAL_CSTRING("content-transfer-encoding:")))
> +    m_partIsQP = lowerCaseLine.Find("quoted-printable", /* ignoreCase = */ true) != -1;

kNotFound instead of -1?

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #14)
> kNotFound instead of -1?
Yes, but just below we have:
if (lowerCaseLine.Find("text/html", /* ignoreCase = */ true) != -1)
else if (lowerCaseLine.Find("multipart/", /* ignoreCase = */ true) != -1)
and five more, so I wanted to go with the flow.

No one noticed the missing initialisation, I'll attach another patch in a moment.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1516320-follow-up.patch

OK, I was so happy with the first patch that I forgot about the initialisation. In fact, it's not needed, since each message or part should provide a header that will set the variable. But for good house-keeping I've added it here.

Comment 17

[Alfred Peters [:infofrommozilla]]

Attachment: ISO-2022-JP+Base64.eml

What about 'CTE: base64'?
This is still not found.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #17)
> What about 'CTE: base64'? This is still not found.
Excuse me? What exactly isn't found? All should be (and is) found in base64 since I fixed bug 1259534. I searched for 日本 and it was found. That's already working in TB 60. If not, please file another bug, here we're doing QP.

Comment 19

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #18)
> (In reply to Alfred Peters from comment #17)
> > What about 'CTE: base64'? This is still not found.
> Excuse me? What exactly isn't found? All should be (and is) found in base64
> since I fixed bug 1259534. I searched for 日本 and it was found. That's
> already working in TB 60. 

So you tried the search for 日本 with attachment 9033372 [details]?
With me the mail is not found. UTF-8 mails are found.

>  If not, please file another bug, here we're doing
> QP.

I'll do

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #19)
> So you tried the search for 日本 with attachment 9033372 [details]?
> With me the mail is not found. UTF-8 mails are found.
Yep. In TB 60.4. There shouldn't be a difference between UTF-8 and ISO-2022-JP after the base64 part is decoded.

Comment 21

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #19)

> So you tried the search for 日本 with attachment 9033372 [details]?
> With me the mail is not found. UTF-8 mails are found.

After I repaired the folder that mail is now found.
Really curious. Sorry for the noise - Case closed. O:-)

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033372 [details]
ISO-2022-JP+Base64.eml

Well, it goes through the folder line by line. If the MSF isn't right, then it won't work :-(

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b3f44f8462f9
Follow-up: Initialise IsQP member variable where appropriate. r=darktrojan DONTBUILD

Comment 24

[Jorg K (CEST = GMT+2)]

TB 60.5 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/d24d2e63ab52d3ddfb0e1ad7cdd14b71ed87e060
https://hg.mozilla.org/releases/comm-esr60/rev/b71f94838b386b650b759e9aa5f310f50ae0adbb

Comment 25

[Jorg K (CEST = GMT+2)]

TB 65 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/e61569181ade20e11058207d79b10ee4647f45de
https://hg.mozilla.org/releases/comm-beta/rev/81e801fbbdbd9df6bc9c8a52db95e697179263b8