Closed Bug 1702197 Opened 5 years ago Closed 5 years ago

quotes in plain text mails get wrapped and displayed as non-quote

Categories

(MailNews Core :: MIME, defect)

Product:
MailNews Core
Component:
MIME
Type:
defect

Tracking

(thunderbird_esr78 unaffected, thunderbird89 fixed)

Status:
RESOLVED FIXED
Milestone:
90 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird89 --- fixed

People

(Reporter: thomas, Assigned: rnons)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

Re: [pve-devel] [RFC qemu-server++ 0_22] remote migration.eml
1.24 KB, text/plain
Details
Re: [pve-devel] [RFC qemu-server++ 0_22] remote migration_send-later-replayed.eml
1.49 KB, text/plain
Details
Re: [pve-devel] [RFC qemu-server++ 0_22] remote migration_original-quoted-from.eml
8.48 KB, text/plain
Details
test-message.eml
633 bytes, text/plain
Details
Bug 1702197 - Prevent using quoted-printable for format=flowed message. r=mkmelin
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-beta+
Details | Review
test-spanish-text.eml
1.84 KB, text/plain
Details
1702197-debug.patch
1.71 KB, patch
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0

Steps to reproduce:

Updated to 87.0b3 (64-bit), albeit the issue may be present since the first beta of version 87.

Ensured to have set the following overrides to make Thunderbird stop breaking my mails, patches (should be default for plain text mails, but that's another story).

mail.wrap_long_lines false
mailnews.wraplength 0
plain_text.wrap_long_lines false
temp.openpgp.wrapHtmlBeforeSend false

Actual results:

On replying to mails the quoted parts get line-wrapped, it seems that especially quotes of quotes, and higher levels of quote-nesting are affected, there longer lines often get wrapped, sometimes even "broken out of quote context", i.e., they look like I wrote some scrambled words in between a quote

The effect is not visible in the editor, only in the mail once actually sent, e.g., to mailing list or with bcc.

Breaks embedding patches too (I know that should be avoided, and I use git send-email most of the time, but until recently embedding patches worked so well that sometimes I do it in Thunderbird when adding one inline on replying)

Expected results:

No line wrapping/whitespace altering whatsoever. Also tried newer 88.0b1, still broken there.

For our convenience, could you please attach an e-mail that shows the issue upon reply. Then we just have to press "Reply" after setting the prefs. You know that you can "send later" (File > Send Later, Ctrl+Shift+Enter), so you will be able to see the message before it goes out in your Outbox. That's just a hint to avoid sending many test messages.

Had a bit of a hard time to reproduce this artificially and not all messages were it happened where without sensitive information.

Anyway, it triggered today when replying to a mailing list post, this was the text entried:

---- 8< ----
On 15.04.21 16:32, Fabian Grünbichler wrote:

I'll be able to test it soon as I'll need to migrate 200-300 vms between
2 datacenter soon.
looking forward to feedback :) you'll need to put the
proxmox-websocket-tunnel binary into $PATH of pveproxy/qm, after
building it with 'cargo build'.
Just to be sure: ensure you build it with cargo build --release or else
it may be quite slow
---- >8 ----

And Actually I got two different results from the original sent mail and from a "send later" retry (thanks for the tip!).
I'll attach them after this reply here.

This was the original where I saw something was odd, saved from my sent folder

this was from testing the same mail text again but using "send later" to see if the issue really gets triggered from this text, it somewhat was (not exactly like the first time, but still off for an extra line)

Argh, now the source in actual backticks to show it raw:

---- 8< ----

On 15.04.21 16:32, Fabian Grünbichler wrote:
>> I'll be able to test it soon as I'll need to migrate 200-300 vms between 
>> 2 datacenter soon.
> looking forward to feedback :) you'll need to put the 
> proxmox-websocket-tunnel binary into $PATH of pveproxy/qm, after 
> building it with 'cargo build'.
Just to be sure: ensure you build it with `cargo build --release` or else 
it may be quite slow

---- >8 ----

FYI: the error was always on my manual line break after the else, one time putting the it correctly afterwards but then introducing a new line for the may be slow and the other time introducing an extra newline

Thanks, could you also attach the original which you replied to, or do we have to make that up ourselves? Can you please attach message/rfc822 attachments as text/plain so they are easier viewable here without downloading the file.

Attachment #9216152 - Attachment is obsolete: true

(In reply to José M. Muñoz from comment #6)

Thanks, could you also attach the original which you replied to, or do we have to make that up ourselves? Can you please attach message/rfc822 attachments as text/plain so they are easier viewable here without downloading the file.

Done, and the original I quoted from is here: https://lists.proxmox.com/pipermail/pve-devel/2021-April/047632.html
But I'll attach it too.

OK, this is what I've done in TB 88 beta 3:
Set these prefs:
mailnews.send_plaintext_flowed to false since your answers don't have format=flowed
mail.strictly_mime to true since your answers have Content-Transfer-Encoding: quoted-printable
And according to your instructions:
mail.wrap_long_lines false
mailnews.wraplength 0
plain_text.wrap_long_lines false

I've also checked that mailnews.send.jsmodule and mailnews.smtp.jsmodule are true, so the new JS modules are used.

Then I imported the original e-mail (attachment 9216184 [details]), selected the five lines in question, and pasted your reply
"Just to be sure: ensure you build it with cargo build --release or else it may be quite slow"
as one line (without the quotes but with the backquotes you can't see here) after the quote after adding a preceding newline.

Then, Ctrl+Shift+Enter, and then I inspect the result in the outbox. My result is:

In-Reply-To: <1618496842.5t56y2jruz.astroid@nora.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 15 Apr 2021 16:32, Fabian Gr=C3=BCnbichler wrote:
>> I'll be able to test it soon as I'll need to migrate 200-300 vms betwe=
en=20
>> 2 datacenter soon.
> looking forward to feedback :) you'll need to put the=20
> proxmox-websocket-tunnel binary into $PATH of pveproxy/qm, after=20
> building it with 'cargo build'.

Just to be sure: ensure you build it with `cargo build --release` or else=20
it may be quite slow

Of course, since you don't have format=flowed, your long answer line will be wrapped/broken.

Your result is

In-Reply-To: <1618496842.5t56y2jruz.astroid@nora.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 15.04.21 16:32, Fabian Gr=C3=BCnbichler wrote:
>> I'll be able to test it soon as I'll need to migrate 200-300 vms betwe=
en=20
>> 2 datacenter soon.
> looking forward to feedback :) you'll need to put the=20
> proxmox-websocket-tunnel binary into $PATH of pveproxy/qm, after=20
> building it with 'cargo build'.

Just to be sure: ensure you build it with `cargo build --release` or else=20
it
may be quite slow

or

In-Reply-To: <1618496842.5t56y2jruz.astroid@nora.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 15.04.21 16:32, Fabian Gr=C3=BCnbichler wrote:
>> I'll be able to test it soon as I'll need to migrate 200-300 vms betwe=
en=20
>> 2 datacenter soon.
> looking forward to feedback :) you'll need to put the=20
> proxmox-websocket-tunnel binary into $PATH of pveproxy/qm, after=20
> building it with 'cargo build'.

Just to be sure: ensure you build it with `cargo build --release` or else=20

it may be quite slow

So somehow you have a gremlin adding some newlines to your reply :-(

IOW, I can't reproduce it. And there is no reason why send and send later should give different results, the message is prepared in the very same way, just not sent out.

Can you consistently reproduce this? Have I missed something in my steps?

and pasted your reply "Just to be sure: ensure you build it with cargo build --release or else it may be quite slow" as one line

I think you should not paste it as one line, but keep it as in comment 5.

Just to be sure: ensure you build it with `cargo build --release` or else 
it may be quite slow

will become

Just to be sure: ensure you build it with `cargo build --release` or else=20

it may be quite slow

The bug is when rendered, there is an extra blank line, but if "=20" is " =", then no extra blank is rendered.

It's the white space after else that's causing the problem. I can tell this is related to bug 1689804, I need some time to find a fix.

Yes, I can reproduce the issue now with adding

Just to be sure: ensure you build it with `cargo build --release` or else 
it may be quite slow

to the reply, with a space after the else in the first line. Works OK without that space. Is this really a rendering issue, looks like a generation issue to me, shouldn't it just generate:

Just to be sure: ensure you build it with `cargo build --release` or else=20
it may be quite slow

I found a much more horrible example which I will attach.

Attached file test-message.eml —

Test message reproducing broken quotes, here are the instructions:
Import message into folder, edit as new plain text message (right-click, Hold down Shift and click "Edit As New Message" to force plain text). Send the message later, Ctrl+Shift+Enter. The result is pretty bad.

Note that the accented characters are necessary and that I have the prefs set as per comment #11.

This happens in TB 78.9.1, so it really looks like a bad regression from bug 1689804.

NI'ing the authors from that bug, somehow one of them has a disabled account now.

Flags: needinfo?(infofrommozilla)
Flags: needinfo?(benc)

I will work on a fix next week, will possibly revert some changes from bug 1689804.

Flags: needinfo?(infofrommozilla)
Flags: needinfo?(benc)

Looking at bug 1689804 comment #79 down to bug 1689804 comment #82 it appears that the fix over there was incorrect or at least not optimal. Maybe it would be best to revert that bug completely since the fix for a minor problem has now created much more severe issues, many plain text e-mails are broken.

I.e. we have:

I think we keep having bugs, because the original fixes were not in the right place.

The right fix for the earlier bugs would be somewhere in MimeInlineTextPlainFlowed_parse_line, which then solves the regressions (i.e. this bug) as well.

You intend to revert this https://hg.mozilla.org/comm-central/rev/289ac419affc#l3.12 as well (sorry if asking prematurely, I "accidentally" saw the changes on try)? Looks like you're reverting this https://hg.mozilla.org/comm-central/rev/289ac419affc#l1.12 here:
https://hg.mozilla.org/try-comm-central/rev/8536e70a0687752678973cb51cc526c2a538f087#l1.39
Fixing mimeenc.cpp is important for backport to TB 78 and for those (few) people not using the JS modules yet. Once the patch is finalised, I'd like to try it with the various test cases here and in bug 1689804. It would be great if just adding line++; in mimetpfl.cpp fixed the problem.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → MIME
Product: Thunderbird → MailNews Core
Version: Thunderbird 88 → 78

(In reply to José M. Muñoz from comment #14)

Created attachment 9216354 [details]
test-message.eml

Test message reproducing broken quotes, here are the instructions:
Import message into folder, edit as new plain text message (right-click, Hold down Shift and click "Edit As New Message" to force plain text). Send the message later, Ctrl+Shift+Enter. The result is pretty bad.

Note that the accented characters are necessary and that I have the prefs set as per comment #11.

This happens in TB 78.9.1, so it really looks like a bad regression from bug 1689804.

Can you tell me what's the problem here, I tested on TB 78.9.1, it looked good.

Forgot to set strictly.mime, I see the problem now.

Yes, it's pretty terrible. As I said, since you fixed mimetpfl.cpp, it would just be a matter of reverting bug 1689804 completely. Fixing three bugs (see comment #18) with one line would be, how to pick the right superlative here, absolutely stunning :-) - Personally I'd also like to try with the Spanish text from bug 1689804 which is where the whole issue started apparently.

Prevent using quoted-printable for format=flowed message.

Assignee: nobody → remotenonsense
Status: NEW → ASSIGNED

I took the easy path of not using QP together with format=flowed, many cases become irrelevant as of this change. But still please help me test the patch.

You can download the built artifact from https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/XsJDnR_8RCCnu0koljfaFg/runs/0/artifacts/public/build/target.tar.bz2. The link is found from https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1fd0a912719d8a70f62e0bf27a3b4340cd8a5fdd, click the B on the Linux x64 opt row, select the Artifacts tab, scroll down to target.tar.bz2. Thanks.

Thank you, rnons!

I took the easy path of not using QP together with format=flowed

+1

Personally, I think we should keep quoted-printable disabled for everybody. (Same with base64 for text.) We managed without it for 15 years. It's not like there are less 8-bit capable servers today than there were 10 years ago.

Here are my test results. If tested with mailnews.wraplength 0 and mail.strictly_mime true to force QP.

Tests with format=flowed off:
Attachment 9216184 [details]: Select the five lines in question, plain text reply, add two additional lines, the first one with a trailing space.
Result: Working with using the new JS modules and the old C++ code.
Attachment 9216354 [details]: Edit as new plain text message and send.
Result: Working with using the new JS modules and the old C++ code.

Tests with format=flowed on:
Attachment 9200194 [details] a from bug 1689804: Select the text and paste as one long line into a new composition. I've used HTML, which will be reduced to plain text. Send. On the sent message, do a "Edit as new message" or "Forward".
Result with the old C++ code: The sent message looks fine, "Edit as new message" or "Forward" is missing the space in the first line between "e interpretaciones". So basically the original issue that bug 1689804 attempted to fix is back. The message is:

Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US

A lo largo de mi vida he sido testigo de muchas historias, experiencias e=
 interpretaciones de la vida. Y me doy cuenta de que lo que m=C3=A1s apre=
cio es la honestidad. En estos =C3=BAltimos meses siento que se ha polari=
[snip]

Result with the new JS modules: The sent message looks fine, however, the plaintext part has a line of over 1000 characters which infringes the RFC. CTE is 8bit. I suggest to use base64 instead of QP for format=flowed.

Can you please explain this change: https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12
I looked at the vicinity and can't quite work out what line and linep are meant to do.

Another observation. Dumping out line with |%s| in MimeInlineTextPlainFlowed_parse_line() after it's being initialised shows this:

|A lo largo de mi vida he sido testigo de muchas historias, experiencias e=
|
| interpretaciones de la vida. Y me doy cuenta de que lo que m=C3=A1s apre=
|

So that code is trying to remove the space stuffing before decoding QP. That won't work, see related discussion in bug 1689804 comment #79 and surrounding comments. To fix it properly, something in mimetpfl.cpp would have to be restructured.

I can see that https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12 was introduced to compensate for https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l4.12 which removes code that was re-introduced here https://hg.mozilla.org/comm-central/rev/f99d81cd24ed, full history in bug 1689804 comment #86.

Attached file test-spanish-text.eml —

Message produced with Spanish text that shows the original problem from bug 1689804 when forwarding/"edit as new message".

This is a very simple patch that adds two lines of debug. It shows that when displaying the message, the QP decoding happens before MimeInlineTextPlainFlowed_parse_line(), when forwarding, it happens after. That's why displaying works and forwarding doesn't. (This is of course all analysis that should have gone into bug 1689804.)

Attached patch additional-fix.patch (obsolete) — — Splinter Review

The forwarding problem can be fixed by removing
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3dba3a2cfaec4bb7a84bf36ebe3c1d
That code was added in bug 1230968 and added two hunks from bug 26734. Bug 1230968 was about long lines in a message encoded in ISO-2022-JP. So although TB these days only produces UTF-8 encoding, it's still thinkable that someone "edits as new" an old ISO-2022-JP message. I can create one in TB 78 and see what happens.

Anyway, the code added in bug 1230968 looks pretty wrong. The base class code in mimemsg.cpp calls into the child class, but only for mimeInlineTextPlainFlowedClass. That's what causes the stuffed space be removed before the QP decoding happens.

I'm just adding f? for you to try forwarding the Spanish message. Of course the patch is only a demonstration as it has if (0) and print statements.

Attachment #9217185 - Flags: feedback?(remotenonsense)

... it's still thinkable that someone "edits as new" an old ISO-2022-JP message. I can create one in TB 78 and see what happens.

No need to create one, there is already an example in bug 1230968 in attachment 8696494 [details].

As far as I can see, forwarding/"edit as new" of that message in TB 78 is totally broken, I see UTF-8 replacement characters <?> injected. That worked before and broke when switching to encoding_rs in bug 1363281. Unlike the predecessor uconv, encoding_rs inserts a <?> character when it sees a zero-length ASCII run (ESC ( B), which is exactly what happens at the end of each line:
`<ESC>$B%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H%F%9%H<ESC>(B
See bug 1508136 for details.

With the additional-fix.patch that reverts bug 1230968, the original issue of spaces being injected comes back, which is not much worse given today's circumstances. In that case, the ASCII run isn't empty since it contains the trailing space.

(In reply to Ben Bucksch (:BenB) from comment #24)

Thank you, rnons!

I took the easy path of not using QP together with format=flowed

👍 As I quoted in Bug 1689804:
RFC 3676 - 4.2. Generating Format=Flowed
| [Quoted-Printable] encoding SHOULD NOT be used with Format=Flowed
| unless absolutely necessary (for example, non-US-ASCII (8-bit)
| characters over a strictly 7-bit transport such as unextended
| [SMTP]).

Personally, I think we should keep quoted-printable disabled for everybody. (Same with base64 for text.) We managed without it for 15 years. It's not like there are less 8-bit capable servers today than there were 10 years ago.

Sadly there are some... <SIC!>
See Bug 1435903

(In reply to José M. Muñoz from comment #25)

Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US

A lo largo de mi vida he sido testigo de muchas historias, experiencias e=
 interpretaciones de la vida. Y me doy cuenta de que lo que m=C3=A1s apre=
cio es la honestidad. En estos =C3=BAltimos meses siento que se ha polari=
[snip]

Result with the new JS modules: The sent message looks fine, however, the plaintext part has a line of over 1000 characters which infringes the RFC. CTE is 8bit. I suggest to use base64 instead of QP for format=flowed.

Thanks a lot, I'm aware this case doesn't work as expected. But the idea is format=flowed should not be used with QP in the first place, and seems no other mail client will create this output, so do you think we can fix it separately?

Can you please explain this change: https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12
I looked at the vicinity and can't quite work out what line and linep are meant to do.

I don't know why two pointers were used, but currently line is used for Edit as new , linep is used for displaying in message pane.

About the zero-length ASCII run (ESC ( B) problem, I found bug 1675335 marked as wontfix. Perhaps we should ignore it here.

Too many issues have accumulated in this bug and here we're mixing a backout of bug 1689804 with some other stuff, namely, not using QP with format flowed in the new JS modules. So my suggestion would be to back out bug 1689804 from all branches first. That would fix this bug here and bug 1705274.

The next problem is that you want to avoid using QP together with format=flowed. I guess there is consent about that. That would be best done in another bug for the JS modules only. As I pointed out, you can't use CTE 8bit and produce lines longer than 1000 chars. That's what your code currently does. I suggest to use base64 in that case.

Backing out 1689804 will reopen that bug and we need to see how to fix it. Maybe your change to the line and length in that small hunk can contribute, although I can't see that
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325
is for "edit as new" and
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#354-355
is for displaying. I haven't looked in detail.

Finally we have the issue of the format=flowed, DelSP ISO-2022-JP message which is broken due to using encoding_rs now. Bug 1675335 is marked as wontfix, that means that encoding_rs will always produce <?> for a zero-length ASCII run. So we'd need to detect and remove those before passing the string to encoding_rs if we deem the problem important enough.

Summary:
Backout bug 1689804 and reopen, close this bug here and bug 1705274.
New bug for the format=flowed QP combination in the JS module.
New bug for "edit as new" of format=flowed, DelSp ISO-2022-JP.

you can't use CTE 8bit and produce lines longer than 1000 chars.

f=f will not produce lines longer than 1000 chars. In fact, f=f is specifically designed to avoid that. So, that's a non-issue.

Also, see comment 31. The same applies to base64.

"The intent of Format=Flowed is to allow user agents to generate flowed text which is non-obnoxious when viewed as pure, raw Text/Plain (without any decoding)"

f=f will not produce lines longer than 1000 chars. In fact, f=f is specifically designed to avoid that. So, that's a non-issue.

Well, with the patch as it stands, I saw a 1300 char 8bit line. So something was wrong.

Anyway, I made a suggestion how to structure the work, so each piece in the puzzle moves to a separate bug. Further to comment #34: Bug 1689804 comment #88 already requests a backout from TB 78 ESR. If you do that but don't back it out from the other branches and instead mix a (partial or modified) backout here with new code addressing a different problem, you're creating a very complicated code management situation.

I can't seem to reproduce 1300 char 8bit line. Can you provide a test eml file?

I'm not against backing out 1689804 from trunk. Although I think it's easier to land my patch if it doesn't introduce new problems. And file two new bugs:

  1. format=flowed, DelSp ISO-2022-JP
  2. format=flowed combined with quoted-printable

I can't seem to reproduce 1300 char 8bit line. Can you provide a test eml file?

Sorry, my mistake. With the Spanish text from attachment 9200194 [details] and mailnews.wraplength 0, I get a CTE 8bit e-mail with two lines, the first 971 chars long, about 1300 for both lines together. Apologies!

As for structuring the work: If you don't backout bug 1689804, you still need to clone it since as I pointed out in comment #25, using the old C++ sending, the original issue is still there. Or is that the new bug in your point 2?

Regardless of how you distribute the various pieces, let's come back to the questions:
From comment #25, comment #26:
Can you please explain this change: https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12
I looked at the vicinity and can't quite work out what line and linep are meant to do. I can see that https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12 was introduced to compensate for https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l4.12 which removes code that was re-introduced here https://hg.mozilla.org/comm-central/rev/f99d81cd24ed

From comment #34:
... your change to the line and length in that small hunk ..., ... I can't see that
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325
is for "edit as new" and
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#354-355
is for displaying.

One more thing: As Alfred outlined in comment #31, some users need to force QP by setting mail.strictly_mime due to bug 1435903. That's actually not a TB bug but a Yahoo problem. So with your patch, you don't use QP but 8bit instead, hence Yahoo users lose their workaround. I suggested before to use base64 if QP can't be used.

(In reply to José M. Muñoz from comment #38)

Regardless of how you distribute the various pieces, let's come back to the questions:
From comment #25, comment #26:
Can you please explain this change: https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12
I looked at the vicinity and can't quite work out what line and linep are meant to do. I can see that https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l5.12 was introduced to compensate for https://hg.mozilla.org/try-comm-central/rev/397a05f39f17558e9083c8f2894ce27d91cd446a#l4.12 which removes code that was re-introduced here https://hg.mozilla.org/comm-central/rev/f99d81cd24ed

As I said in comment #32, line is used for edit as new, if you put a printf above https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325, you will see it when editing a mail as new. So if the first character is a stuffed space, without moving line, the space will show up in the editor. I was using

0
 1
  2

as a test, save it as draft and then edit as new. Without line++, the 2nd and 3rd lines will have an extra leading space.

I will take a look at bug 1435903, maybe when mail.strictly_mime is true and format=flowed is enabled, use base64 is a good idea.

Thanks for the clarification. Yes, line is used at https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325
so moving that variable forward becomes necessary when removing
https://searchfox.org/comm-central/rev/30a3fdb9e1779af4c10d8e7b9f54e9feee9dde06/mailnews/mime/src/mimemsg.cpp#151-159

So the C++ changes look OK, however, as you can see in attachment 9217185 [details] [diff] [review],
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325
will become dead code one day, but we can leave that for the new "'edit as new'/forward QP format=flowed" bug, the original bug 1689804 which you intend to file again as your point 2 in comment #37. I hope I understood that correctly.

I've already voiced concern over not QP-encoding despite mail.strictly_mime being set. I wouldn't shift this out into another bug. Particularly bug 1435903 is about something quite different; in summary, that we ship out 8bit despite the server not advertising that ability. In case of Yahoo 8bit capability is advertised, but the server still messes up 8bit encoded mail.

In MimeEncoder.jsm you now have:

        if (this._isMainBody && this._contentType == "text/plain") {
          // From rfc3676#section-4.2, Quoted-Printable encoding SHOULD NOT be
          // used with Format=Flowed unless absolutely necessary.
          encodeP = !Services.prefs.getBoolPref(
            "mailnews.send_plaintext_flowed"
          );
        } else {
          encodeP = true;
        }

My suggestion would be:

        if (
          this._isMainBody &&
          this._contentType == "text/plain" &&
          // From rfc3676#section-4.2, Quoted-Printable encoding SHOULD NOT be
          // used with Format=Flowed unless absolutely necessary.
          Services.prefs.getBoolPref("mailnews.send_plaintext_flowed")
        ) {
          needsB64 = true;
        } else {
          encodeP = true;
        }
      }

I've linted and tested this, a message with the Spanish text comes out in base64 and as a bonus, forwarding/"edit as new" works.

Blocks: 1706809

Comment on attachment 9217185 [details] [diff] [review]
additional-fix.patch

(In reply to Ping Chen (:rnons) from comment #37)

... file two new bugs:

  1. format=flowed, DelSp ISO-2022-JP
  2. format=flowed combined with quoted-printable

I filed bug 1706809. I think it makes sense to treat the issue together since fixing one can impact the other, see comment #29.

Attachment #9217185 - Attachment is obsolete: true
Attachment #9217185 - Flags: feedback?(remotenonsense)
Attachment #9216959 - Attachment description: Bug 1702197 - Fix removing stuffed space in format=flowed message. r=mkmelin → Bug 1702197 - Prevent using quoted-printable for format=flowed message. r=mkmelin
Blocks: 1706822

(In reply to José M. Muñoz from comment #41)

So the C++ changes look OK, however, as you can see in attachment 9217185 [details] [diff] [review],
https://searchfox.org/comm-central/rev/cf2a1d1ccc968227ae2933590f470c85075ed42a/mailnews/mime/src/mimetpfl.cpp#325
will become dead code one day, but we can leave that for the new "'edit as new'/forward QP format=flowed" bug, the original bug 1689804 which you intend to file again as your point 2 in comment #37. I hope I understood that correctly.

Yes, I filed bug 1706822 just now. I think I tried your patch yesterday, seems not enough, let's leave it to 1706822.

I've already voiced concern over not QP-encoding despite mail.strictly_mime being set. I wouldn't shift this out into another bug. Particularly bug 1435903 is about something quite different; in summary, that we ship out 8bit despite the server not advertising that ability. In case of Yahoo 8bit capability is advertised, but the server still messes up 8bit encoded mail.

Thanks, I wasn't going to leave this to another bug. I have updated the patch to your suggestion of using base64.

(In reply to José M. Muñoz from comment #42)

Comment on attachment 9217185 [details] [diff] [review]
additional-fix.patch

(In reply to Ping Chen (:rnons) from comment #37)

... file two new bugs:

  1. format=flowed, DelSp ISO-2022-JP
  2. format=flowed combined with quoted-printable

I filed bug 1706809. I think it makes sense to treat the issue together since fixing one can impact the other, see comment #29.

Sorry, missed this message, not sure I understand. Is the format=flowed, DelSp ISO-2022-JP problem related to QP?

Keywords: checkin-needed-tb
Target Milestone: --- → 90 Branch

Sorry, missed this message, not sure I understand. Is the format=flowed, DelSp ISO-2022-JP problem related to QP?

Now we have bug 1706809 and bug 1706822. The former has all the details of the format=flowed, DelSp ISO-2022-JP and the format=flowed QP issues, including code references, etc. Bug 1706822 started off with a description related to QP and a bug title related to DelSp ISO-2022-JP, and was later changed to cover the QP issue.

I have the impression that the two issues are related, both are about format=flowed, of course the DelSp ISO-2022-JP additionally has the added empty ASCII run issue. As I wrote in bug 1706809 comment #1, you can easily fix the format=flowed QP issue by reverting the hunk from bug 1230968.

I suggest to proceed with the bug I filed since a) it was there first and b) has all the details. You could take care of the empty ASCII run in a "part 2" there. Anyway, you can structure the work as you deem fit. As a remark for your bug 1706822: Yes, the root cause is that QP decoding is done after f=f processing, but only for forward/"edit as new". And the processing for forward/"edit as new" was added in bug 1230968 to fix DelSp ISO-2022-JP.

BTW, thanks for going with the base64 suggestion.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5d80e2f8fe17
Prevent using quoted-printable for format=flowed message. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

This is a regression and should go into beta.

Flags: needinfo?(remotenonsense)

Comment on attachment 9216959 [details]
Bug 1702197 - Prevent using quoted-printable for format=flowed message. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1689804 and some others
User impact if declined: In some cases, flowed plain text mails are incorrect when encoded in QP
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): medium, there have been back and forth changes over the years to some mime c++ code, we often find new problems some time later. On the other hand, I think land it to beta will expose problems sooner.

Flags: needinfo?(remotenonsense)
Attachment #9216959 - Flags: approval-comm-beta?

Comment on attachment 9216959 [details]
Bug 1702197 - Prevent using quoted-printable for format=flowed message. r=mkmelin

[Triage Comment]
Approved for beta

Attachment #9216959 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: