Message-ID not preserved on "edit draft" of message manually stored in Drafts folder, breaking threading. 2nd problem: reusing message ID when "edit as new".

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: Pierre Ossman, Assigned: Jorg K (GMT+2))

Tracking

45 Branch
Thunderbird 50.0

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 fixed, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4549+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160512103502

Steps to reproduce:

1. An email thread was generated by a different program (git in this case) and saved in Drafts for further editing.

2. I opened the root message and edited it in Thunderbird.

3. I clicked save


Actual results:

The Message-ID was re-generated, breaking the thread and orphaning the other messages that were referring to the root message.


Expected results:

Message-ID should have been preserved, keeping the thread intact.

(or optionally, the other messages should have had their references updated, even though this is less than ideal as there might be external references that Thunderbird doesn't see)
(Assignee)

Comment 1

11 months ago
I don't understand what you did. You saved a whole e-mail thread (multiple messages) that came from where exactly(?) into the drafts folder? Why do you do that? The Drafts folder is a special folder not intended for message storage by the user. If you want to reuse a message, you can edit it as new.

Anyway, the complaint is that if you edit a message as new you get a new ID and the previous one is not added to the references. Also, if you copy a message to the drafts folder (which is questionable) and then edit it, the same happens.

As far as I can see this hasn't changed from TB 38, so no regression here.

Magnus, you know more about threading, can you please comment.
Flags: needinfo?(mkmelin+mozilla)
Summary: Message-ID not preserved on edit, breaking threading → Message-ID not preserved on "edit draft" or "edit as new", breaking threading
(Reporter)

Comment 2

11 months ago
(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #1)
> I don't understand what you did. You saved a whole e-mail thread (multiple
> messages) that came from where exactly(?) into the drafts folder? Why do you
> do that? The Drafts folder is a special folder not intended for message
> storage by the user. If you want to reuse a message, you can edit it as new.
> 

It came from git, specifically git format-patch and git imap. These produce an email thread that you are in many cases expected to modify before sending. But I suspect this issue would remain no matter which non-Thunderbird tool would have been used. The Drafts folder is a remote folder on the IMAP server, so it should be expected to be shared between programs.

> Anyway, the complaint is that if you edit a message as new you get a new ID
> and the previous one is not added to the references. Also, if you copy a
> message to the drafts folder (which is questionable) and then edit it, the
> same happens.

The action was Edit, not Edit as new. I would fully understand the latter (or any kind of copy operation) generating a completely new ID.

> 
> As far as I can see this hasn't changed from TB 38, so no regression here.
> 

Indeed, no regression. This was meant as a possible improvement. :)
(Assignee)

Comment 3

11 months ago
Thanks for clarifying. On a new message, we don't hand out a new ID every time it gets saved. So we'd have to look into why you get a new ID if you prepare a draft in an unusual way like you do.

(Note: Recent unrelated reference improvement in bug 1242676.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Message-ID not preserved on "edit draft" or "edit as new", breaking threading → Message-ID not preserved on "edit draft" of message manually stored in Drafts folder, breaking threading

Comment 4

11 months ago
Just to clarify: we do seem to preserve the message-id of the draft normally. 

Maybe it's just read from the .msf file instead of the message, and that fails if the message is from "outside".
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 5

11 months ago
A bit of debugging:
nsMsgComposeAndSend::GenerateMessageId() is called from nsMsgComposeAndSend::GatherMimeAttachments().

If mCompFields->GetMessageId() is empty, a new ID is generated.
That seems to be the case for completely new messages and "fake foreign" drafts which are edited.

Once a draft is saved and edited again, no new ID is allocated.

Now, how do we detect not to allocate a new ID. The answer is easy: We detect our own drafts by the HEADER_X_MOZILLA_DRAFT_INFO. If such a header exists, we set the message ID we found for further processing, that means, we don't allocate a new one later on. Otherwise, no message ID is set and a new one gets allocated later:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1251

We could easily change the processing to use the message ID of the message but I don't know what that would break.

Magnus, do you have an opinion?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 6

11 months ago
Magnus, it would be easy to reuse the message ID already present in a message stored manually in the Drafts folder, but do we want to encourage this?

Also, can we rely on the IDs prepared by a third party?

Another option would be to tell the people who are preparing drafts themselves to also add the internal draft header, currently:
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=1; DSN=1; uuencode=0;
 attachmentreminder=0; deliveryformat=4
They could try just using
X-Mozilla-Draft-Info: internal/draft;
since the list of other parameters is likely to grow/change one day.
(Reporter)

Comment 7

11 months ago
(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #6)
> 
> Also, can we rely on the IDs prepared by a third party?
> 

Message IDs in every other folder are generated by a third party, so I would assume most of the code is well prepared for odd IDs?

> Another option would be to tell the people who are preparing drafts
> themselves to also add the internal draft header, currently:
> X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=1; DSN=1; uuencode=0;
>  attachmentreminder=0; deliveryformat=4
> They could try just using
> X-Mozilla-Draft-Info: internal/draft;
> since the list of other parameters is likely to grow/change one day.

This is possible in my use case (git format-patch has a --add-header argument), but I doubt every other tool has such option. E.g. I can't do custom headers in Thunderbird itself, or Claws.
(Assignee)

Comment 8

11 months ago
As I tried to say: I'm not sure that your (ab)use case is supported (no offence intended). If you want to replace TB draft generation, you have to do it completely.

But let's hear Magnus' opinion.
(Assignee)

Comment 9

11 months ago
Created attachment 8762687 [details] [diff] [review]
This would do it (v1).

I left a debug line in place in case you want to try.

Note that I changed the logic a bit. If we edit one of our own drafts as new, that is draftinfo is found, we still shouldn't be reusing the message id, since that will be potentially used with the original draft.
Attachment #8762687 - Flags: feedback?(mkmelin+mozilla)
(Reporter)

Comment 10

10 months ago
I tested the patch on 45.1.1. Seems to work fine for my use case. :)

I did however notice another bug (which I verified without the patch as well); saving a message in a thread makes the mail list go bonkers. I see two scenarios:

 a) A message with 8 replies: The 8 replies get orphaned, and a new thread shows up with all 9 messages properly threaded.

 b) A message with a single reply: Sometimes the root message gets the subject from the reply

The problem resolves itself by leaving the folder and opening it again.

Should I file a separate bug for this?
(Assignee)

Comment 11

10 months ago
One bug per issue ;-) And in the other bug please explain what you're doing. Sounds like more (ab)use in the drafts folder since that's where message are saved. Perhaps you can attach the full mailbox to the new bug.

And since you tested this patch, you must be able to build TB. So your patch is welcome for the other issue.

Comment 12

10 months ago
Comment on attachment 8762687 [details] [diff] [review]
This would do it (v1).

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

Looks good to me!
Attachment #8762687 - Flags: feedback?(mkmelin+mozilla) → feedback+
(Assignee)

Comment 13

10 months ago
Can I take that as an r+ if I remove the print?

Comment 14

10 months ago
Sure!
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 15

10 months ago
Created attachment 8763143 [details] [diff] [review]
Proposed solution (v1a)

r+ as per comment #13 and comment #14.
Assignee: nobody → mozilla
Attachment #8762687 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8763143 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Assignee)

Comment 16

10 months ago
https://hg.mozilla.org/comm-central/rev/516a17742688
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Reporter)

Comment 17

10 months ago
Yay! Thanks for fixing my somewhat unusual case. I'll try to send extra many patches as thanks. :)
(Assignee)

Comment 18

10 months ago
Comment on attachment 8763143 [details] [diff] [review]
Proposed solution (v1a)

I'll uplift this to Aurora (TB 49) and Beta (TB 48), mainly because editing a draft as new message should *not* maintain the ID, something that was also fixed here.

As for ESR 45:
[Approval Request Comment]
Regression caused by (bug #): No regression, was always wrong.
User impact if declined: Duplicate IDs for "edit as new" drafts
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Small change, should be good to go for TB 45.x where x>=3. Let's bake it for a while on Aurora/Beta.
Attachment #8763143 - Flags: approval-comm-esr45?
Attachment #8763143 - Flags: approval-comm-beta+
Attachment #8763143 - Flags: approval-comm-aurora+
(Assignee)

Updated

10 months ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → affected
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
(Assignee)

Comment 19

10 months ago
Changing the summary to reflect the second problem fixed here.
Summary: Message-ID not preserved on "edit draft" of message manually stored in Drafts folder, breaking threading → Message-ID not preserved on "edit draft" of message manually stored in Drafts folder, breaking threading. 2nd problem: reusing message ID when "edit as new".
(Assignee)

Comment 20

10 months ago
Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/33a99963f843

Beta (TB 48):
https://hg.mozilla.org/releases/comm-beta/rev/40a022b30ba6
status-thunderbird48: affected → fixed
status-thunderbird49: affected → fixed
(Assignee)

Updated

10 months ago
Duplicate of this bug: 640018
(Assignee)

Updated

10 months ago
Duplicate of this bug: 720646
(Assignee)

Updated

10 months ago
Duplicate of this bug: 771165
(Assignee)

Comment 24

7 months ago
Another one worth taking.
Flags: needinfo?(rkent)

Comment 25

7 months ago
Comment on attachment 8763143 [details] [diff] [review]
Proposed solution (v1a)

http://hg.mozilla.org/releases/comm-esr45/rev/33c72ce3a2f2
Flags: needinfo?(rkent)
Attachment #8763143 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

7 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 49+
You need to log in before you can comment on or make changes to this bug.