1476788 - no subject causes extra "re:" on reply

Status: RESOLVED FIXED

(MailNews Core :: General, defect)

Description

[Ossan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

I use Thunderbird version 52.9.1 with Windows 10 Home 1803.

When I reply for an email with no subject, the replied email will always have subject "Re: Re: ".



Actual results:

Increasing "Re: " in subject for email with no subject.
Every time I reply the email, it will increase the number of "Re: " such as "Re: Re: Re: Re: ".



Expected results:

Just one "Re: " will be replied for email with no subject.

Comment 1

[:Gijs (he/him)]

Not a security bug.

Comment 2

[Wayne Mery (:wsmwk)]

regression?  Or duplicate?
https://mzl.la/2zPohUy

Comment 3

[Ossan]

I rather say adding "Re:" one by one.

received  → replied by TB 52.9.1 (maybe TB 52.9.0 is the same)
(no subject) → "Re:"
"Re:"  → "Re: Re:"
"Re: Re:"  → "Re: Re: Re:"
"Re: Re: Re:"  → "Re: Re: Re: Re:"
"Re: Re: Re: Re:"  → "Re: Re: Re: Re: Re:"
...


I checked the source of email with "Re: Re: Re:" in the message pane and it said "Re: Re: Re:" (same).
The message LIST pane said "Re: Re: Re: Re:"  (one-redundant).

Comment 4

[Alfred Peters [:infofrommozilla]]

Attachment: Reliably remove the "RE" from an otherwise empty subject.

<https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/local/src/nsParseMailbox.cpp#1239>

| if (NS_MsgStripRE(nsDependentCString(key), modifiedSubject))
|     flags |= nsMsgMessageFlags::HasRe;

NS_MsgStripRE() tests the input header 'key' for "Re:" in different languages and strips them. If it does so, the result is stored in  modifiedSubject. Otherwise modifiedSubject is empty.

A few lines later we decide with isEmpty(), which version we store:

| m_newMsgHdr->SetSubject(modifiedSubject.IsEmpty() ? key : modifiedSubject.get());

If key contained only REs, that's the wrong criterion.

The same mistake is made in nsNNTPNewsgroupList.cpp with news articles.

Fix: Check the return value instead of IsEmpty().

Comment 5

[Alfred Peters [:infofrommozilla]]

Bug 1481541 is a Dupe of this.

Comment 6

[Alfred Peters [:infofrommozilla]]

Here we could extend the test:

<https://dxr.mozilla.org/comm-central/source/comm/mailnews/base/test/TestMsgStripRE.cpp>

But this seems to be disabled.

Comment 8

[Jorg K (CEST = GMT+2)]

Bug 1481541 comment #2 claims that it was working in TB 52 until July 2018 when bug 1466343 hit TB 52.9. I tried TB 52.0 and yes, it's working better there. Looks like I broke this in bug 1466343:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208
But where did it go wrong? Can we look for that.

The caller "traditionally" checked the returned string, not the return value, although of course that a valid approach, too.

BTW, the past tense of "to strip" is "stripped" not "striped", as in having stripes ;-)

TestMsgStripRE.cpp is disabled, see bug 1466748.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #8)
> But where did it go wrong? Can we look for that.
I see it now. The crazy original code sometimes returned modifiedSubject:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208#l1.145
and sometimes it modified the original string in situ:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208#l1.168

So the original code always used the string which had the Re: removed, but sometimes returned in the first parameter, sometimes in the second. Jolly good :-(

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: bug1476788.patch - Reliably remove the "RE" from an otherwise empty subject.

Thanks a lot for fixing the regression I caused :-(

There's no excuse, but for a laugh, here's a description of the old interface:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208#l2.18
  @note This API is insane and should be fixed.
:-(

Comment 11

[Jorg K (CEST = GMT+2)]

Oh, I forgot to mention: I s/striped/stripped/ ;-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c6d36dbf6916
Reliably remove the "Re:" from an otherwise empty subject. r=jorgk DONTBUILD

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 9009470 [details] [diff] [review]
bug1476788.patch - Reliably remove the "RE" from an otherwise empty subject.

TB 60.1/60.2:
https://hg.mozilla.org/releases/comm-esr60/rev/4ab8f2f107badd76c47c34d873e97c3c4ee4d683
Ugly regression that's better fixed before it goes too far.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #4)
> NS_MsgStripRE() tests the input header 'key' for "Re:" in different
> languages and strips them.
That's the part I don't understand. I have mailnews.localizedRe set to "AW" and answering a German mail with "AW: So und so" still doesn't strip the "AW:". Is that expected?

Comment 15

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+2) from comment #14)

> That's the part I don't understand. I have mailnews.localizedRe set to "AW"
> and answering a German mail with "AW: So und so" still doesn't strip the
> "AW:". Is that expected?

Works for me.

1. 'mailnews.localizedRe' is casesensetive. So at least should be included: "AW, Aw, aw".

2. It seams the stored subject from the offline store/cache is used. If you still see an AW in the threadpane, try 'repair folder'.

Comment 16

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #15)

> 1. 'mailnews.localizedRe' is casesensetive. So at least should be included:
> "AW, Aw, aw".

Without the spaces! -> "AW,Aw,aw"

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #16)
> Without the spaces! -> "AW,Aw,aw"
Thanks, works for me now as well. After many years of accumulating Re: Aw: Re: ...

Comment 18

[Jorg K (CEST = GMT+2)]

TB 63 beta:
https://hg.mozilla.org/releases/comm-beta/rev/cabd893ac8e91b3e64f3d0158ab6effa2e6f291c