583587 - Add In-Reply-To or References header when forwarding a message

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Jonathan Protzenko [:protz]]

This topic has been brought up by a user of my "conversation view" extension. His complaint is, basically, that Gmail shows forwarded messages in the conversation view while my extension doesn't. As this sounds like a reasonable feature request, I'd like to support it.

However, Gloda does not return forwarded messages when calling getMessagesCollection. I think the reason is the message database does not link in any way the forwarded message to the original message.

What I suggest is adding an In-Reply-To header that points to the Message-Id of the original message in the forwarded message. That way, Gloda will properly regroup these messages.

1) I'm not sure this is correct, so please tell me if I'm completely wrong.
2) This has potentially deep implications, especially on the threaded view in the thread pane (will regroup the thread that has the forwarded message as a root with the original thread).

As I'm no expert in this area, I'd like to have your opinion on this. Thanks!

PS: I've filed this in the "compose" component because I think the compose code is responsible for setting up the proper headers, but please change if I'm wrong

Comment 1

[rsx11m]

The "References" header was removed in bug 464782 when forwarding inline just with 3.0 on purpose as it was confusing many recipients who didn't have a clue what it is used for. If you are talking about adding the References header as regular RFC-822/5322 header to your own message which is forwarding the other message, that sounds like an adequate replacement for any applications which wants to use this to also find it somewhere in the message. If Gloda covers it and would detect it in this way, I don't know.

Comment 2

[Jonathan Protzenko [:protz]]

(In reply to comment #1)
> The "References" header was removed in bug 464782 when forwarding inline just
> with 3.0 on purpose as it was confusing many recipients who didn't have a clue
> what it is used for. If you are talking about adding the References header as
> regular RFC-822/5322 header to your own message which is forwarding the other
> message,
Yes, I'm talking about References:/In-Reply-To: RFC-822 headers, not the boilerplate that's included in the message body when forwarding a given message.

> that sounds like an adequate replacement for any applications which
> wants to use this to also find it somewhere in the message. If Gloda covers it
> and would detect it in this way, I don't know.

Let's see what asuth thinks of this.

Comment 3

[David :Bienvenu]

Some mailers do seem to do this (essentially, add the in-reply-to header to a forward inline message as if you had replied to the original message). If we want them to thread together, it's the easiest way of getting that done.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Can you elaborate on the specific scenario you are trying to support?  As I understand it, there are 2 ways to forward messages:

1) Inline, where the only 'metadata' about the message is what is visible in the message.  And as rsx11m mentions, we removed the useful metadata we were contributing and not everyone else played the game anyways.

2) As an attachment.  All metadata is (presumably) retained.

If you are trying to handle case #2, gloda can already see the meta-data associated with the attached message and figure things out[1].

If you are trying to handle case #1, I presume one would turn to content analysis.  It would not surprise me if gmail did something like this.

I suspect your ideal is to handle both cases?


In terms of your proposal; it certainly would simplify things from an implementation perspective and provide useful metadata that could benefit other mailers.  I would propose the following implementation strategy could be reasonable:

- Set the references header to have _only_ the message-id of the message you are forwarding.  The rationale is to avoid information leakage; the reference chain can easily reveal information about who replied to whom based on the domains commonly embedded in the message-ids.  People are not likely to expect that forwarding a message might reveal information about other messages in the conversation, especially if they are forwarding inline and then selectively removing information from the email.

- Also add a new "X-Forwarded-Message-Id" header that has the message-id of the message you are forwarding.  The idea is to allow mailers that are specially aware of forwarding (potentially gloda) to be able to detect that the 'reference' is really a forwarded reference without trying to guess based on the presence of 'fw' in the subject or some other heuristic.

Of course, it's possible there's already an RFC for something like this out there already...

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

I forgot to add a footer note (the [1]) about current gloda not being really good at 'merging' conversations right now...

Comment 6

[Jonathan Protzenko [:protz]]

(In reply to comment #4)
> 
> I suspect your ideal is to handle both cases?
Yes, but only in the case where I am the one forwarding the message. That means we have control over which headers Thunderbird adds to the forwarded message, which makes implementing your proposal below feasible.

I'm not interested in inferring anything regarding messages that people forward to me (anyway, I don't have the original message, so it's not like it can help me build a conversation).
> 
> 
> - Set the references header to have _only_ the message-id of the message you
> are forwarding.
Can you confirm that doing this will include the forwarded message into the original GlodaConversation ?
> 
> - Also add a new "X-Forwarded-Message-Id" header that has the message-id of the
> message you are forwarding.
Do you want gloda to have a special behavior for forwarded messages? The first part seems to be enough for me.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #6)
> I'm not interested in inferring anything regarding messages that people forward
> to me (anyway, I don't have the original message, so it's not like it can help
> me build a conversation).

Well, if someone forwards you something you've already seen, it would help link that message up for context purposes.

> > - Set the references header to have _only_ the message-id of the message you
> > are forwarding.
> Can you confirm that doing this will include the forwarded message into the
> original GlodaConversation ?

Kinda.  There's a current gloda limitation where once we have assigned a message to a conversation we never change it.  In the cases where messages do not carry their entire 'references' chain around (ex: some microsoft mail clients only do a single in-reply-to/references), the order in which we index messages can result in a conversation partition.

However, this is not all that hard to rectify, and in your specifically described scenario, it is extremely likely that in the incremental case gloda will do the right thing because it will have already seen the message you are forwarding before it sees the forwarded message.

> > - Also add a new "X-Forwarded-Message-Id" header that has the message-id of the
> > message you are forwarding.
> Do you want gloda to have a special behavior for forwarded messages? The first
> part seems to be enough for me.

I think we want the ability for people to be able to break up conversations into separate conversations when it is useful to them.  Being able to automatically know where the 'weak' conversational ties are seems useful for visualization and interface purposes.  I could also see someone writing an extension to change the behaviour so people can say "never treat my forwarded messages as part of the conversation I forwarded them from, but do maintain a link".

Comment 8

[Jonathan Protzenko [:protz]]

Attachment: Simple patch that just adds standard References: headers

I just did the first part, and I'll discuss with Andrew later to decide if we should add an extra header somehow. The patch is small and doesn't change the nsICompFields interface.

Comment 9

[David :Bienvenu]

Functionally, the patch looks fine, though I haven't tried running with it yet. A few nits:

1. We don't tend to reference bug numbers in code comments - hg blame will give the curious the same information.

2. In general, this file doesn't use the K&R braces style, and our rule is to use the prevailing braces style on a per-file basis.

3. I don't think you can do cstring + cstring with the external linkages, so you'll need to use Append. (Or you can't use +=; I don't remember, but in any case, you'll need to use Append, not the operator overloads).

Comment 10

[David :Bienvenu]

Comment on attachment 463235 [details] [diff] [review]
Simple patch that just adds standard References: headers

minusing based on previous comments - also, you should be able to use GetMsgDBHdrFromURI which handles getting the message service for you...

Comment 11

[Jonathan Protzenko [:protz]]

Attachment: Same thing as before, with nits taken into account

(In reply to comment #9)
> 
> 1. We don't tend to reference bug numbers in code comments - hg blame will give
> the curious the same information.
Ok. It's just that I found many occurrences of that practice in the same file, so I thought it was standard practice.

protz@sleet-2:~/Code/comm-central/mailnews/compose/src $ egrep "bug [0-9]{4,6}" nsMsgCompose.cpp | wc -l
       6
> 
> 2. In general, this file doesn't use the K&R braces style, and our rule is to
> use the prevailing braces style on a per-file basis.
Fixed.
> 
> 3. I don't think you can do cstring + cstring with the external linkages, so
> you'll need to use Append. (Or you can't use +=; I don't remember, but in any
> case, you'll need to use Append, not the operator overloads).
Ok, I trust you on that one ;-).


(In reply to comment #10)
> Comment on attachment 463235 [details] [diff] [review]
> Simple patch that just adds standard References: headers
> 
> minusing based on previous comments - also, you should be able to use
> GetMsgDBHdrFromURI which handles getting the message service for you...

Fixed as well.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #9)
> 1. We don't tend to reference bug numbers in code comments - hg blame will give
> the curious the same information.

I find it useful to include bug numbers in cases where there was a really complex problem and/or lots of discussion about something but it is not feasible or justified to document all of that as a comment in the source code.  'Justification' for these purposes would be how relevant the information is to someone maintaining the code in terms of understanding why the code does what it does.

In this case, I think it is justified and important to specify exactly why we are adding a header.  The choice being made here is not intuitive and not due to a well-known mail RFC but is an arbitrary implementation choice and should be documented.  Given that our discussion on this bug has been extremely limited, there isn't much point in referencing the bug right now.

The comment should also make it clear that we are _only_ including the message-id of the forwarded message in order to avoid potential information leakage.

I would suggest that before moving forward with this patch much more a message be posted to tb-planning about the proposed change; this is right up the alley of the list.

I'm also inclined to require the patch to add something like my proposed X-Forwarded-Message-Id since it does not seem particularly hard to add to the patch or dangerous to do so.  I am not wedded to repeating the message-id in there, but it seems like it could be useful in case people start forwarding multiple messages in a single message and also maintaining the full references chain.  Presumably the x-forwarded header would then be able to unambiguously identify the multiple messages that were forwarded versus the references stew.

Comment 13

[Jonathan Protzenko [:protz]]

What worries me is that adding a X-Forwarded-Message-Id will probably imply that nsIMsgCompFields gets an extra field for that very header (looks like the standard course of action to implement it). However, according to https://wiki.mozilla.org/Thunderbird/Landing_Patches_on_Thunderbird3.1, if an IDL changes, chances are very low that this patch will actually be accepted in 3.1.

Actually, I'd be very much in favor of checking that in on both 1.9.2 and trunk, and then creating a followup bug to implement (only on c-c this time) the X-Forwarded-Message-Id.

What do you think?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #13)
> What worries me is that adding a X-Forwarded-Message-Id will probably imply
> that nsIMsgCompFields gets an extra field for that very header (looks like the
> standard course of action to implement it). However, according to
> https://wiki.mozilla.org/Thunderbird/Landing_Patches_on_Thunderbird3.1, if an
> IDL changes, chances are very low that this patch will actually be accepted in
> 3.1.

otherRandomHeaders:

https://developer.mozilla.org/en/Extensions/Thunderbird/HowTos/Common_Thunderbird_Use_Cases/Compose_New_Message#Add_Header

Comment 15

[David :Bienvenu]

Comment on attachment 464183 [details] [diff] [review]
Same thing as before, with nits taken into account

finally got a tree that built, and ran with this patch - you need to put <> around the references header value.

We probably want some sort of xpcshell test for this, if possible, though I don't know if we can do both smtp and pop3 in the same xpcshell test, or if we can fake it out be doing a send later and looking at the message in the outbox.

Comment 16

[Jonathan Protzenko [:protz]]

Ok I'll fix the brackets thing tomorrow.

Will a test be mandatory to land this patch? If so, I was more thinking of doing a mozmill test that simply consists in creating a fake message, forwarding it, saving as draft, and then checking that the draft has the right reference. Should work, no?

Comment 17

[David :Bienvenu]

(In reply to comment #16)
> 
> Will a test be mandatory to land this patch? If so, I was more thinking of
> doing a mozmill test that simply consists in creating a fake message,
> forwarding it, saving as draft, and then checking that the draft has the right
> reference. Should work, no?

Usually, we prefer to have xpcshell tests to test core functionality, but I'm OK with a mozmill test. Yes, I think saving as draft should work, and would be easier to write, since you can use the db hdr to get at the references header. I think we should have a test for this, since it's a pretty simple test to write.

Comment 18

[Jonathan Protzenko [:protz]]

Attachment: Final take, good for review

I think that patch is good for review. It basically does everything requested, including adding a custom X-Forwarded-Message-Id before sending the message. It also tests for the correctness of both kinds of headers in the two possible cases: forwarding inline and forwarding as attachemnts (mozmill test added).

Comment 19

[Wayne Mery (:wsmwk)]

does this totally fix bug 239257?

Comment 20

[Jonathan Protzenko [:protz]]

Yes it's exactly the same thing, I had missed that one.

Comment 22

[David :Bienvenu]

Comment on attachment 466122 [details] [diff] [review]
Final take, good for review

+  otherHeaders.Append(NS_LITERAL_CSTRING("\r\n"));
+  rv = mCompFields->SetOtherRandomHeaders(otherHeaders.get());

just return mCompFields->SetOther...then you won't need rv.

exercice should be exercise in a couple places.

I'll run with the patch today.

Comment 23

[David :Bienvenu]

Comment on attachment 466122 [details] [diff] [review]
Final take, good for review

r=me, modulo the previous nits I mentioned. The feature works, the test works, and even better, fails without the core change :-).

Comment 24

[Jonathan Protzenko [:protz]]

Attachment: Final take, commit this!

Comment 25

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3871e8092a3a

Comment 26

[Jonathan Protzenko [:protz]]

Comment on attachment 466474 [details] [diff] [review]
Final take, commit this!

Patch applies on comm-1.9.2, as this changes no IDL, requesting approval-thunderbird3.1.x

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 466474 [details] [diff] [review]
Final take, commit this!

Having thought about this, this is effectively an enhancement which also would change the default behaviour of Thunderbird. Therefore this is outside of the general scope of security and stability releases, so won't be accepted for 3.1.x.

We will consider for the 3.2 branch when that gets set up (hence setting flag).