Closed Bug 1476788 Opened Last year Closed Last year

no subject causes extra "re:" on reply

Categories

(MailNews Core :: General, defect)

defect
Not set

Tracking

(thunderbird_esr6062+ fixed, thunderbird63 fixed, thunderbird64 fixed)

RESOLVED FIXED
Thunderbird 64.0
Tracking Status
thunderbird_esr60 62+ fixed
thunderbird63 --- fixed
thunderbird64 --- fixed

People

(Reporter: atnak55r, Assigned: infofrommozilla)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Not a security bug.
Group: mail-core-security
regression?  Or duplicate?
https://mzl.la/2zPohUy
Summary: no subject increase extra "re:" → no subject causes extra "re:" on reply
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).
<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().
Attachment #9009460 - Flags: review?(jorgk)
Bug 1481541 is a Dupe of this.
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.
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.
(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 :-(
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.
:-(
Attachment #9009460 - Attachment is obsolete: true
Attachment #9009460 - Flags: review?(jorgk)
Attachment #9009470 - Flags: review+
Oh, I forgot to mention: I s/striped/stripped/ ;-)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c6d36dbf6916
Reliably remove the "Re:" from an otherwise empty subject. r=jorgk DONTBUILD
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Blocks: 1466343
Component: Message Compose Window → General
Keywords: regression
Product: Thunderbird → MailNews Core
Version: 52 Branch → 52
Assignee: nobody → infofrommozilla
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.
Attachment #9009470 - Flags: approval-comm-esr60+
Attachment #9009470 - Flags: approval-comm-beta+
(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?
(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'.
(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"
(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: ...
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.