61829 - Signature duplication

Status: RESOLVED WORKSFORME

(MailNews Core :: Composition, defect, P3)

Description

[Myroslav Opyr]

Two accounts, both have the same e-mail address and signature. Second has
reply-to set.

BUG:
When replying to a mail message signature is appended to reply being composed.
When changing account in From: combobox another (the same) signature is appended.

I noticed that signature has extra CR/LF appended and if delete it before
changing From: account, then previous signature is being deleted properly and
new one appended.

Comment 1

[Myroslav Opyr]

Attachment: .signature.html file I use

Comment 2

[R.K.Aa.]

seems related to bug 29281 "signature filename gets copied to other servers"

Comment 3

[Myroslav Opyr]

IMHO, component is different. In bug 29281 we have problem with account manager.
I have a problem with composer...

Comment 4

[scottputterman]

changing milestone to unknown.  It will get changed back when we figure out what
milestone to put this bug in.

Comment 5

[pmock]

changing qa assign

Comment 6

[Koike Kazuhiko]

I reported the same problem as bug 75497 on April and marked as worksforme
several days after. Account switching works fine now.

Comment 7

[Jean-Francois Ducarroz]

I am able to reproduce this problem.

Comment 8

[scottputterman]

moving to 1.0.  It sounds like we need help reproducing though.

Comment 9

[Jean-Francois Ducarroz]

Putterman, I *can* reproduce this problem using the attached signature file!

Comment 10

[scottputterman]

*** Bug 95727 has been marked as a duplicate of this bug. ***

Comment 11

[Bob Lord]

From bug 95727:

To reproduce:
1. Go to a web page.
2. Click File/Send Link
3. Click the From: button in the mail compose window and select another email
account

Expected behavior: 
The client should replace the first signature with the signature from the newly
selected account.

Observed behavior:
The client appends the new sig to the page.

Note: This problem does *not* exist in a new compose window that did not start
with a forward action. You have to forward (and maybe reply, I don't know).

Comment 12

[Bob Lord]

Can we get this fixed soon now that we can reproduce it?  

Comment 13

[Jean-Francois Ducarroz]

moving it back to 0.9.7

Comment 14

[Jean-Francois Ducarroz]

*** Bug 104839 has been marked as a duplicate of this bug. ***

Comment 15

[Denis Antrushin]

During message creation extra <br> is inserted after signature at some point
So quick fix would to check (in ProcessSignature function) if last dom element
is BR and is so, check prev. for being moz signature.
But I think it's better to find where extra BR is inserted....

Comment 16

[Denis Antrushin]

Sorry, I meant SetSignature(), not ProcessSignature()...

Comment 17

[Jean-Francois Ducarroz]

I have a fix in my tree which need to be tested...

Comment 18

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

Instead of looking for an element with a specific class name at the end of the
message body, I an now looking for an element anywhere in the body which the id
is "moz-user-signature". To avoid removing the wrong element when replying or
forwarding inline a message, I delete the string id="moz-user-signature" in the
original message body before inserting it.

Comment 19

[Denis Antrushin]

Attachment: screenshot of replying to test message with patch applied

See attached screenshot for side effect you probably do not want to happen ;-)

I suspect that aBuf is a string containing replied/forwarder message body,
so why to look there at all?
After removal of diff block @@ -480,4 +481,14 @@ patch is working great.
(Also, to compile patch, I had to change nsStr::Delete to nsStr::Delete1,
but this is not important as this piece should be removed)

Comment 20

[Jean-Francois Ducarroz]

Denis, that's a risk but as the id choosen (moz-user-signature) is not a common
string that somebody will use, the probability that append in real condition is
almost null. I can write more complexe detection code but I prefer keep it as
simple as possible.

I have to remove it from the original message else I wont be able to figure out
which of the signatures is the correct one to remove/replace when switching
identity!

Also, nsStr::Delete get just replaced by nsStr::Delete1 for monobyte string and
nsStr::Delete2 for double bytes string. In our case, we should use nsStr::Delete2

Comment 21

[Denis Antrushin]

I still missing one thing.
The piece of code we're talking about just removes string from *text* of message,
But to find signature, you're searching for *DOM node* whose id attribute has the
same value.
I don't see how content of raw message may interfere with DOM node attributes...
Am I missing something?
As I said, patch is working even w/o this fragment, so why to keep it? :-)

Comment 22

[Jean-Francois Ducarroz]

aBuf contains the raw HTML data of the original message, not just text!

Comment 23

[Denis Antrushin]

Well, I'm an idiot :-(
But why not just insert aBuf into editor and then find element with given id
in the DOM tree and clear it before inserting signature?

Also, I'm concerned with performance. Is DOM traversal and nsStr::Delete
fast enough? In 90% messages sig is at the bottom of message, so we will
do much extra work with GetElementById - I afraid, we'll get bug saying
"replying to 900 KB html mail is slow"...
Isn't it better (if ever possible) to walk few nodes from the bottom of the tree?

Also, I still wonder from where comes that extra <BR>, probably it will
help us fix few other bugs about "extra line being inserted here and there".
OK, I better shut up, before I said something stupid again :-)

Comment 24

[scottputterman]

adding nsbeta1- and moving out.  But, if we get a fix and a review we can check
it in earlier.

Comment 25

[Jean-Francois Ducarroz]

In the proposed fix, I need to replace nsStr::Delete2(aBuf... by aBuf.Cut(...

Comment 26

[Rob W.W. Hooft]

I am seeing this signature duplication now too. Using two different accounts,
two different signature files. It did not exist in 0.9.7, but 0.9.9 does have
this problem for me.

Comment 27

[Todd Cohn]

*** Bug 133024 has been marked as a duplicate of this bug. ***

Comment 28

[fabio rotondo]

Note that it does not matter if you have different signature files or not.
All you need to have is two or more accounts with a signature.

Comment 29

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2 (not finish)

This patch is work in progress, see todo comment in code...

Comment 30

[Mike Cowperthwaite]

Myroslav Opyr, I cannot reproduce the symptom using your sample .sig file with 
Mozilla 1.8a6-1130 nor with 1.7.5.  I suspect the problem was fixed quite some 
time ago; this is a very old bug.  Further, there are other, more current bugs 
reported about switching between From identities at composition time, and I 
imagine this bug would have a lot of duplicates by now if it still existed.

So, I'm resolving this WFM, but if the symptom still exists, please reopen.

Comment 31

[Myroslav Opyr]

This started to work for quite time ago, but in some situations (I cannot
reproduce it for sure), signatures duplicate still. It is not annoying anymore,
but the bug is there sometimes.

The approved way of have the issue appear is: if I have an identity without
signature and when composing with that identity I copy & pate the whole message
in the message body and then switch to identity that has the signature, the one
that I copy & pasted doesn't disappear. I am not sure if this behavior is bug or
that should be that way. One can argue.

This is not only case to make signatures duplicate, but I cannot give
reproducible case at the moment.

Comment 32

[Mike Cowperthwaite]

> signatures duplicate still. It is not annoying anymore,
> but the bug is there sometimes.

See bug 218346 for one specific, and easily reproducible, case where the body 
can end up with multiple signatures.  I don't think that case applied to the 
original report for this bug.  If you discover a way to reproduce the problem 
that isn't related to that bug, feel free to reopen this one with the new 
information.