The default bug view has changed. See this FAQ.

Behaviour of malformed rfc2047 encoded Subject message header inconsistent

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 50.0
x86_64
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr38 wontfix, thunderbird_esr45- wontfix)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8581251 [details]
Showing three different representations of the subject

A mal-formed RFC2047 subject header is decoded in the message list, the message summary, the tab and when saving as EML file, but not the message header, see enclosed image.

It's mal-formed since it contains a "closing single quote" (’) (U+2019).
TB displays this in three different ways.
(Assignee)

Comment 1

2 years ago
Created attachment 8581255 [details]
Two messages showing the problem.

Enclosed two messages with mal-formed subjects.

The second one, the "Schnapp-broken" is from bug 506927. It has an illegal additional "= " in the header.

TB copes with the malformed headers as well as possible, only in the message header display in the message pane they look bad.
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 2

2 years ago
I was surprised to see that there are two RFC2047 decoders at work here. The new JSMime implementation and an old C++ implementation in comm-central/mozilla/netwerk/mime.

This latter one doesn't perform as well as the new implementation, for "bad" headers, it passes the header back, as the following debug print shows:

Bad header:
=== (nsMIMEHeaderParamImpl) DecodeRFC2047Str: Header >>>=?windows-1252?Q?Announcing_India’s_First_Regional_Hospitality_Awards?=<<<
=== (nsMIMEHeaderParamImpl) DecodeRFC2047Str: Result >>>=?windows-1252?Q?Announcing_India’s_First_Regional_Hospitality_Awards?=<<<

Good header (replaced the ’ with a '):
=== (nsMIMEHeaderParamImpl) DecodeRFC2047Str: Header >>>=?windows-1252?Q?Announcing_India's_First_Regional_Hospitality_Awards?=<<<
=== (nsMIMEHeaderParamImpl) DecodeRFC2047Str: Result >>>Announcing India's First Regional Hospitality Awards<<<
(Assignee)

Comment 3

2 years ago
OK, I cheated a bit on the debug output. What is returned from DecodeRFC2047Str for the ’ is
xE2, x80, x99. This the UTF-8 encoding for U+2019:
[1110] 0010 - [10]00 0000 - [10]01 1001, so stripping the part in [] away we have:
0010 0000 0001 1001 (decimal: 2019).

The big question is: When will JSMime replace the C++ code in comm-central/mozilla/netwerk/mime?
(Assignee)

Comment 4

2 years ago
I noticed that in
https://dxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#78
we don't provide an output function for the subject header.
Is this the place to ensure a "decent" display?

I still haven't figured out where <mail-headerfield id="expandedsubjectBox" ... receives its content.
(Assignee)

Updated

2 years ago
Summary: Behaviour of mal-formed rfc2047 encoded Subject message header inconsistent → Behaviour of malformed rfc2047 encoded Subject message header inconsistent
(Assignee)

Updated

2 years ago
Blocks: 1132405
(Assignee)

Comment 5

2 years ago
Kent: Sorry to trouble you with this one. Maybe you can take a look. Perhaps it's just another omission (like in bug 1166206 and bug 1130248) and an easy fix. Maybe not all places where "glue" to the new JSMIME was necessary were covered. If it's easy, we should consider inclusion in TB38. A quick look at attachment 8581251 [details] demonstrates the problem.
Flags: needinfo?(rkent)
Blocks: 673092
(Assignee)

Updated

a year ago
See Also: → bug 1224426, bug 1204499
(Assignee)

Updated

a year ago
Flags: needinfo?(rkent)
Flags: needinfo?(Pidgeot18)
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1279804

Comment 7

10 months ago
Related/dupe: bug 1023285
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1285481
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

9 months ago
See Also: → bug 1023285
(Assignee)

Comment 12

9 months ago
(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #11)
> So this bug here continues to be about the different display in tread pane
> (using jsmime) and message header in message pane.
More precisely:
This bug here continues to be about the different display of a malformed *subject* in tread pane (using jsmime) and message header in message pane (apparently not yet using jsmime, see comment #2).

Bug 1023285 fixed the different display of the *from/to* headers using raw UTF-8 which was a different problem. For the subject, raw UTF-8 always worked. Sorry about the confusion in comment #9 (marked obsolete).
(Assignee)

Comment 13

9 months ago
OK, since I'm revisiting this bug, here is a stack trace of how we get into the M-C netwerk code:

DecodeRFC2047Str(...) Line 1173	C++
internalDecodeRFC2047Header(...) Line 751	C++
nsMIMEHeaderParamImpl::DecodeRFC2047Header(...) Line 778	C++
MIME_DecodeMimeHeader(...) Line 34	C++  <==== Mailnews.
MimeHeaders_convert_header_value(...) Line 50	C++
MimeHeaders_write_all_headers(...) Line 603	C++
MimeMessage_write_headers_html(...) Line 776	C++
MimeMessage_close_headers(...) Line 433	C++
MimeMessage_parse_line(...) Line 251	C++
convert_and_send_buffer(...) Line 152	C++
mime_LineBuffer(...) Line 238	C++
MimeObject_parse_buffer(...) Line 240	C++
mime_display_stream_write(...) Line 990	C++
nsStreamConverter::OnDataAvailable(...) Line 938	C++
nsDocumentOpenInfo::OnDataAvailable(...) Line 303	C++
nsMailboxProtocol::ReadMessageResponse(...) Line 589	C++
nsMailboxProtocol::ProcessProtocolState(...) Line 675	C++
nsMsgProtocol::OnDataAvailable(...) Line 293	C++

It's called from mailnews/mime/src/comi18n.cpp

I'm not sure why mimehdrpar->DecodeRFC2047Header()
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/comi18n.cpp#32

isn't replaced with a call to decodeMimeHeader()
https://dxr.mozilla.org/comm-central/source/mailnews/mime/public/nsIMimeConverter.idl#70
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeJSComponents.js#482
(Assignee)

Comment 14

9 months ago
Created attachment 8769612 [details] [diff] [review]
Proposed solution (v1).

Sorry Kent, more work for you.

Inspired by bug 1023285, I have returned to this bug here, my old pet hate, and finally got to the bottom of it.

The fix is to replace the use of the M-C netwerk stuff with jsmime. This has happened in many other locations, but not here.

Since this is used for the display of the message header in the message pane, there is no performance problem. The tread pane already uses jsmime.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8769612 - Flags: review?(rkent)
(Assignee)

Comment 15

9 months ago
Created attachment 8769779 [details] [diff] [review]
Proposed solution (v1a).

Removed unnecessary include.
Attachment #8769612 - Attachment is obsolete: true
Attachment #8769612 - Flags: review?(rkent)
Attachment #8769779 - Flags: review?(rkent)
(Assignee)

Comment 16

9 months ago
Created attachment 8769866 [details]
Another test

Here's another test. To/From/Subject look bad in message pane, but good with the patch here and from bug 1023285.
(Assignee)

Comment 17

9 months ago
Created attachment 8769867 [details]
Without patches (above), with patches (also from bug 1023285) below.

A picture says more than 1000 words ;-)

Comment 18

9 months ago
How about these:
1. From: Homer Simpson
2. From: <Homer Simpson>
3. From: Homer Simpson:;

Only #3 is spec compliant, and if the first 2 are issued by an MTA, it's just wrong.  But in fact, #1 and #2 are/were used in feeds..  A separate bug, sure ;)

Comment 19

9 months ago
Comment on attachment 8769779 [details] [diff] [review]
Proposed solution (v1a).

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

I made one comment about what to me is premature optimization.

But the real issue is that I am very uncomfortable reviewing this, as I don't really know the differences between the two code variants, and what the issues might be in doing this change. You are proposing a change in underlying method in a really commonly used code path. My fear is that you are solving one regression at the risk of introducing many others.

I could review this as a reviewer of last resort, but is there anyone else who might have more knowledge of this?

::: mailnews/mime/src/comi18n.cpp
@@ +23,5 @@
>                             bool override_charset, bool eatContinuations,
>                             nsACString &result)
>  {
> +  // Since we are in C code here, do some basic caching with a static.
> +  static bool triedToGetConverter = false;

We do this sort of getting of XPCOM components all over the place without caching it. The pattern that you are introducing is very rare in other Gecko code, and I am reluctant to introduce a new pattern, with possible unforeseen consequences, unless there is a clear indication that this is a performance bottleneck, and the proposed fix has a clear benefit.
(Assignee)

Comment 20

9 months ago
(In reply to Kent James (:rkent) from comment #19)
> But the real issue is that I am very uncomfortable reviewing this, as I
> don't really know the differences between the two code variants, and what
> the issues might be in doing this change. You are proposing a change in
> underlying method in a really commonly used code path. My fear is that you
> are solving one regression at the risk of introducing many others.
Not really true. That jsmime method is used all over the place now:
https://dxr.mozilla.org/comm-central/search?q=mimeConverter-%3EDecodeMimeHeader&redirect=false

If you look at
https://dxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#3644

you can see that there is caching going on, but in a member of the class.

> I could review this as a reviewer of last resort, but is there anyone else
> who might have more knowledge of this?
Me ;-) ?

> We do this sort of getting of XPCOM components all over the place without
> caching it.
I can remove the caching. No problem, although other code does caching, see above.
(Assignee)

Comment 21

9 months ago
Created attachment 8769903 [details] [diff] [review]
Proposed solution (v2).

No caching.
Attachment #8769779 - Attachment is obsolete: true
Attachment #8769779 - Flags: review?(rkent)
Attachment #8769903 - Flags: review?(rkent)
(Assignee)

Comment 22

9 months ago
(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #20)
> (In reply to Kent James (:rkent) from comment #19)
> You are proposing a change in
> > underlying method in a really commonly used code path. My fear is that you
> > are solving one regression at the risk of introducing many others.
> Not really true. That jsmime method is used all over the place now:
> https://dxr.mozilla.org/comm-central/search?q=mimeConverter-
> %3EDecodeMimeHeader&redirect=false
The point is this: There are two decoders, the jsmime one and the M-C::Network one.

The jsmime one is used here:
https://dxr.mozilla.org/comm-central/search?q=mimeConverter-%3EDecodeMimeHeader&redirect=false
(five times).

The M-C:Network one is used here:
https://dxr.mozilla.org/comm-central/search?q=DecodeRFC2047Header&redirect=false
(once).

That means that the entire system has been changed over to jsmime, minus the one call site in
mailnews/mime/src/comi18n.cpp 

The two decoders give different results in some cases and therefore cause inconsistencies in the UI. Additionally, the jsmime decoder works better, that means it is more error tolerant to malformed headers. That's what this bug is about.

I don't know why the last call site wasn't changed over. Maybe an oversight, or maybe is was left out deliberately. The only reason I could think of would be a threading issue. Apparently XPCOM written in JavaScript must run on Gecko's main thread; that was the issue why Outlook import doesn't work any more since jsmime was introduced.

Maybe that's why my try run
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=561cc6161b81&selectedJob=20106
came put totally red with heaps of identical errors (only quoting a few here):
TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_hidden_attachments.js | xpcshell return code: -11
TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_mimeStreaming.js | xpcshell return code: -11
TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_rfc822_body.js | xpcshell return code: -11
TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_text_attachment.js | xpcshell return code: -11

So maybe "doing the right thing" and using the same decoder is technically not possible.

Magnus, do you know more about this (since Joshua won't answer)?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 23

9 months ago
Comment on attachment 8769903 [details] [diff] [review]
Proposed solution (v2).

Clearing the r? for now since the solution seems to cause massive Xpcshell failures.
Attachment #8769903 - Flags: review?(rkent)
(Assignee)

Comment 24

9 months ago
OK, I've asked here:
https://groups.google.com/d/msg/mozilla.dev.apps.thunderbird/EB6fqF3Hngk/ZE4aHBKBBQAJ

Comment 25

9 months ago
No idea, but it's possible it wasn't moved over because those test failures would have to be figure out first.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 26

9 months ago
Since the review was cancelled, I'm adding NI for Kent as a reminder.

Weird, I ran some of the failing tests locally on Windows 10 and they all passed:
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_rfc822_body.js
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_hidden_attachments.js
mozilla/mach xpcshell-test mailnews/base/test/unit/test_bug366491.js
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_query_messages_imap_online_to_offline.js

The one I could get to fail locally was this one, the only one that didn't fail with error -11:
mozilla/mach xpcshell-test mailnews/mime/test/unit/test_message_attachment.js

Looks like I need help ;-(

I'm shipping this to try again, this time on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7b68a873a081
Flags: needinfo?(rkent)
(Assignee)

Comment 27

9 months ago
On Windows, I get exactly one test failure, the one that I can reproduce locally, the one that isn't error -11:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=20174

TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_message_attachment.js | gStreamListener.onStopRequest - [gStreamListener.onStopRequest : 81] "testSubje.eml" == "testSubject.eml"

I can look into those, but I have no idea what the error -11 are about.
(Assignee)

Comment 28

9 months ago
Created attachment 8770431 [details] [diff] [review]
Proposed solution (v2) + test fixed.

Sadly the test was wrong.
'testSubject' should be 
'=?UTF-8?B?dGVzdFN1YmplY3Q=?=' and not
'=?UTF-8?B?dGVzdFN1YmplY3Q?='
Try it here: http://dogmamix.com/MimeHeadersDecoder/
Obviously jsmime and M-C::Network decoders treated the faulty input differently.

So let's ship this off to try one more time:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16ee96c41c5d
Attachment #8769903 - Attachment is obsolete: true
(Assignee)

Comment 29

9 months ago
Comment on attachment 8770431 [details] [diff] [review]
Proposed solution (v2) + test fixed.

So here we go again with r?.

Surprisingly, or maybe not surprisingly, the Xpcshell test came out green on *all* platforms after fixing the one true issue. No more error -11:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16ee96c41c5d5677d2c53bfab719d5337f4200b9
Flags: needinfo?(rkent)
Attachment #8770431 - Flags: review?(rkent)
(Assignee)

Comment 30

9 months ago
Comment on attachment 8770431 [details] [diff] [review]
Proposed solution (v2) + test fixed.

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

::: mailnews/mime/src/comi18n.cpp
@@ +31,5 @@
>      return;
>    }
> +
> +  nsAutoString res;
> +  mimeConverter->DecodeMimeHeader(header, default_charset, override_charset, 

I'll fix the trailing space later.
(Assignee)

Comment 31

9 months ago
Kent, if it's any consolation to you: I've downloaded the try build (which also includes the fix to bug 1023285, the other jsmime issue), and it works just fine. I haven't seen any adverse effects. I'd be happy to let this ride the trains for a while ;-)
(Assignee)

Comment 32

9 months ago
Created attachment 8770579 [details] [diff] [review]
Proposed solution (v2b) + test fixed.

Simplified string processing.
Attachment #8770431 - Attachment is obsolete: true
Attachment #8770431 - Flags: review?(rkent)
Attachment #8770579 - Flags: review?(rkent)

Comment 33

9 months ago
Comment on attachment 8770579 [details] [diff] [review]
Proposed solution (v2b) + test fixed.

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

OK looks good to me.

Since this was the last caller of DecodeRFC2047Header AFAICT, bonus points for filing a bug in core to remove this. Even more bonus points for adding the patch to remove this (I would have required that if the removed methods was in c-c instead of m-c).
Attachment #8770579 - Flags: review?(rkent) → review+
(Assignee)

Comment 34

9 months ago
Thanks for the quick review. Mojibake (https://en.wikipedia.org/wiki/Mojibake) is really a pet hate of mine ;-)

https://hg.mozilla.org/comm-central/rev/32170e6fb298 --> FIXED.

I don't think I can earn bonus points here since DecodeRFC2047Header is part of Necko:
https://dxr.mozilla.org/comm-central/source/mozilla/netwerk/mime/nsIMIMEHeaderParam.idl#165

I don't think they'd want that removed but I can ask:

====

Honza, Valentin or Patrick: We eliminated the last call to nsMIMEHeaderParamImpl::DecodeRFC2047Header. Would you like to cull this or hang onto it?
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr38: --- → wontfix
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 35

9 months ago
Comment on attachment 8770579 [details] [diff] [review]
Proposed solution (v2b) + test fixed.

[Approval Request Comment]
Regression caused by (bug #): Bug 858337 (jsmime)
User impact if declined: https://en.wikipedia.org/wiki/Mojibake
Testing completed (on c-c, etc.): Yes, in test suite, see comment #28.
Risk to taking this patch (and alternatives if risky):
Some risk, that's why I'm uplifting it to TB 49 so it can get into the next beta early.
Attachment #8770579 - Flags: approval-comm-aurora+
(Assignee)

Comment 36

9 months ago
(oops, forgot to mark it fixed in comment #34).
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
(Assignee)

Updated

9 months ago
Blocks: 858337
if you searched the addons repo for it you can make a patch to remove it.

otherwise I would just let it hang there until the great web-extensions migration

thanks for asking
Flags: needinfo?(mcmanus)
(Assignee)

Updated

9 months ago
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 38

9 months ago
(In reply to Kent James (:rkent) from comment #33)
> Since this was the last caller of DecodeRFC2047Header AFAICT, bonus points
> for filing a bug in core to remove this.
So I earned bonus points for filing bug 1286792. I might submit a patch there, too.
(Assignee)

Comment 39

8 months ago
Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/d27fd6055434
status-thunderbird49: affected → fixed
Duplicate of this bug: 1204499

Comment 41

4 months ago
Comment on attachment 8770579 [details] [diff] [review]
Proposed solution (v2b) + test fixed.

[Approval Request Comment]
This is tracking-tb45 and has been through beta. Any reason not to uplift?
Attachment #8770579 - Flags: approval-comm-esr45?
(Assignee)

Comment 42

4 months ago
I didn't want to annoy the ESR45 manager ;-)
if the question is for me - in general it's great to have patches, but I'll simply restate my opinion that we should avoid taking patches for things that lack more than marginal impact to the customer base.  

As to this specific patch, I've skimmed some comments (not read the whole bug) and what I read doesn't seem to indicate high value to 45.x customers. If I am incorrect, free free to uplift

Comment 44

4 months ago
Comment on attachment 8770579 [details] [diff] [review]
Proposed solution (v2b) + test fixed.

OK I've looked this over, and as far as I can tell it is a bug with some risk, whose goal is to have more consistent handling of faulty input. That sounds to me like a risk not worth taking.
Attachment #8770579 - Flags: approval-comm-esr45? → approval-comm-esr45-

Updated

4 months ago
status-thunderbird_esr45: affected → wontfix
tracking-thunderbird_esr45: ? → -
(Assignee)

Comment 45

4 months ago
I don't want to argue, but bug 1023285, which you took, seems way more risky.

Comment 46

4 months ago
Yes but it also had 6 duplicates, so it was clear that somebody cared about it.

But these are all judgement calls and I cannot claim perfect consistency.
You need to log in before you can comment on or make changes to this bug.