Closed Bug 1279869 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

45 Branch
defect
Not set

Tracking

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

RESOLVED FIXED
Thunderbird 50.0
Tracking Status
thunderbird47 --- wontfix
thunderbird48 --- fixed
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 49+ fixed

People

(Reporter: ossman, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
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
(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. :)
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
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)
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)
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.
(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.
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.
Attached patch This would do it (v1). (obsolete) — Splinter Review
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)
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?
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 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+
Can I take that as an r+ if I remove the print?
Sure!
Flags: needinfo?(mkmelin+mozilla)
r+ as per comment #13 and comment #14.
Assignee: nobody → mozilla
Attachment #8762687 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8763143 - Flags: review+
https://hg.mozilla.org/comm-central/rev/516a17742688
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
Yay! Thanks for fixing my somewhat unusual case. I'll try to send extra many patches as thanks. :)
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+
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".
Duplicate of this bug: 640018
Duplicate of this bug: 720646
Duplicate of this bug: 771165
Another one worth taking.
Flags: needinfo?(rkent)
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+

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.

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

You need to log in before you can comment on or make changes to this bug.