1279869 - 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".

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Pierre Ossman]

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)

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Pierre Ossman]

(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. :)

Comment 3

[Jorg K (CEST = GMT+2)]

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.)

Comment 4

[Magnus Melin [:mkmelin]]

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".

Comment 5

[Jorg K (CEST = GMT+2)]

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?

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Pierre Ossman]

(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.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 10

[Pierre Ossman]

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?

Comment 11

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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

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

Looks good to me!

Comment 13

[Jorg K (CEST = GMT+2)]

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

Comment 14

[Magnus Melin [:mkmelin]]

Sure!

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a)

r+ as per comment #13 and comment #14.

Comment 16

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/516a17742688

Comment 17

[Pierre Ossman]

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

Comment 18

[Jorg K (CEST = GMT+2)]

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.

Comment 19

[Jorg K (CEST = GMT+2)]

Changing the summary to reflect the second problem fixed here.

Comment 20

[Jorg K (CEST = GMT+2)]

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

Beta (TB 48):
https://hg.mozilla.org/releases/comm-beta/rev/40a022b30ba6

Comment 24

[Jorg K (CEST = GMT+2)]

Another one worth taking.

Comment 25

[Kent James (:rkent)]

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

http://hg.mozilla.org/releases/comm-esr45/rev/33c72ce3a2f2

Comment 26

[nicolas]

I still see the same issue on TB 60.3.0

• Generates patches from git into DRAFT (git format-patch | git imap-send)
• Editing the patch within the draft folder and saving it keeps the Message-ID

• When sending the patch, Message-ID has changed.
• Same thing for "Send Later". Message-Id is changed in the Unsent folder.

Comment 27

[Jorg K (CEST = GMT+2)]

This bug is long fixed and closed, please file a new bug with the appropriate details.