Last Comment Bug 572290 - <title> value of HTML signature file shows up in signature
: <title> value of HTML signature file shows up in signature
Status: RESOLVED FIXED
[tb31needed][tb3needed]
: regression, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla2.0b4
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: CVE-2010-2769
  Show dependency treegraph
 
Reported: 2010-06-15 22:34 PDT by u331436
Modified: 2010-08-20 07:10 PDT (History)
10 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.9+
.9-fixed
.12+
.12-fixed


Attachments
Test case (384 bytes, text/html)
2010-08-09 13:09 PDT, :Ehsan Akhgari
no flags Details
Patch (v1) (8.68 KB, patch)
2010-08-09 18:32 PDT, :Ehsan Akhgari
bzbarsky: review+
christian: approval1.9.2.9+
christian: approval1.9.1.12+
Details | Diff | Splinter Review
Fix a mistake (4.08 KB, patch)
2010-08-09 18:33 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review

Description u331436 2010-06-15 22:34:59 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100615 Lightning/1.1a1pre SeaMonkey/2.0pre - not Firefox
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100615 Lightning/1.1a1pre SeaMonkey/2.0pre - not Firefox

when composing a new message (IMAP account) in HTML mode, the word "signature" is inserted before the signature itself.
My signature is a file (signature.html) configured to be picked as a default sig for this account.
This problem appear as a regression between Friday June 11th and Sunday 13th for the SM2.1 nightlies.

Reproducible: Always

Steps to Reproduce:
1.click on compose
2.new composition window opens, in HTML mode (as configured) with the word signature inserted
3.The word "signature" appear with the variable width default font, prior to the signature itself.


Expected Results:  
only the content of the signature file should appear

workaround: edit all outgoing message to remove the word.... annoying !
Comment 1 u331436 2010-06-15 22:35:51 PDT
will check on test profile
Comment 2 u331436 2010-06-15 22:43:53 PDT
Same issue with test profile
Comment 3 Jens Hatlak (:InvisibleSmiley) 2010-06-18 12:09:36 PDT
I created a test HTML signature file in Composer (SM's HTML editor component). After some testing I found that the value inside <title> is displayed in the MailNews Compose window where the HTML file is included as the signature (in HTML mode, of course). Could you please check whether the same applies to you (i.e. does "signature" appear somewhere in your HTML signature file, e.g. the <title>?).
Comment 4 u331436 2010-06-18 22:36:34 PDT
Yessss ! spot on !
Without anything between the <title></title> (or without any <title> line in the code) the composition window is fine !
That is an easy workaround.
Should this bug be closed here ?
BTW, I am not sure that the bug should not be corrected anyway as Kompozer for instance ask for a title when saving an HTML file.
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-06-19 01:51:52 PDT
Confirming and adjusting summary. I haven't checked whether this is a regression yet but it sure is a bug; <title> information shouldn't appear in the signature when composing a message. If it is a regression, then maybe from the recent changes in the editor component, or maybe fallout from the HTML5 parser (which is on by default for some time already).
Comment 6 Jens Hatlak (:InvisibleSmiley) 2010-06-19 10:15:14 PDT
Last-known-good: 2010-06-14 -> regression.

Candidates:
http://hg.mozilla.org/comm-central/rev/db911c5871d2
http://hg.mozilla.org/mozilla-central/rev/208d7f999037
(maybe more)

I'd put my bet on the second one since the first one was only BR handling changes (apart from test changes).

I wonder whether TB is affected, too (would be a surprise if not).
Comment 7 Jens Hatlak (:InvisibleSmiley) 2010-06-19 10:34:44 PDT
Same with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100619 Shredder/3.2a1pre -> over to MailNews Core, although this might actually be a Core bug.
Comment 8 :Ehsan Akhgari 2010-07-30 14:10:00 PDT
How do you actually insert the signature in the email body editable field?
Comment 9 Jens Hatlak (:InvisibleSmiley) 2010-07-30 14:25:57 PDT
(In reply to comment #8)
> How do you actually insert the signature in the email body editable field?

SM/TB does that automatically for you if you select an HTML file in Account Settings/<account>/"Attach the signature from a file instead". To reproduce this bug, select a file with these contents there (I hope Bugzilla won't transform this, otherwise: mainly a title tag with "title" and a body tag with "body" as contents):

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
    <title>title</title>
  </head>
  <body>
    body
  </body>
</html>

While this bug is present, you'll see "\n\n-- \ntitle body" when you compose a new message for the modified account.
Comment 10 :Ehsan Akhgari 2010-07-30 14:26:52 PDT
Hrm, no, I mean, how, as in, what API does the program use to do this!  :-)
Comment 11 Jens Hatlak (:InvisibleSmiley) 2010-07-31 00:23:24 PDT
htmlEditor->InsertHTML, since we're talking about HTML mode here.
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp
nsMsgCompose::ConvertAndLoadComposeWindow
nsMsgCompose::SetIdentity
Comment 12 :Ehsan Akhgari 2010-07-31 06:54:34 PDT
Does the title element itself get added to the mail document as well, or is it just the text node?

By the way, I think all 1.9.1 and 1.9.2 branch nightlies should also be affected by this.  Can someone please confirm?  If that is the case, this bug needs to block the next release of both branches.
Comment 13 Jens Hatlak (:InvisibleSmiley) 2010-07-31 07:42:08 PDT
Mail bodies (source):

Latest Shredder 3.0 nightly:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>

<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
<div class="moz-signature">-- <br>
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<title>title</title>
body </div>
</body>

</html>

Latest Shredder trunk nightly, html5.enable = true (default):

<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    <div class="moz-signature">-- <br>
      title body </div>
  </body>
</html>

Latest Shredder trunk nightly, html5.enable = false:

<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    <div class="moz-signature">-- <br>
      title body </div>
  </body>
</html>

Haven't checked TB 3.1.
Comment 14 :Ehsan Akhgari 2010-08-09 13:09:49 PDT
Created attachment 464140 [details]
Test case
Comment 15 :Ehsan Akhgari 2010-08-09 18:32:37 PDT
Created attachment 464254 [details] [diff] [review]
Patch (v1)

We were failing to honor mIgnoreNextCloseHead in the paranoid fragment sink.
Comment 16 :Ehsan Akhgari 2010-08-09 18:33:47 PDT
Created attachment 464256 [details] [diff] [review]
Fix a mistake

I also discovered some mistakes in the existing test, which is fixed by this patch.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-09 18:38:33 PDT
I'm not a content peer and shouldn't be reviewing these patches, sorry
Comment 18 :Ehsan Akhgari 2010-08-09 18:40:29 PDT
Comment on attachment 464254 [details] [diff] [review]
Patch (v1)

(bz: a quick review here is appreciated as this is a branch blocker)
Comment 19 Boris Zbarsky [:bz] (TPAC) 2010-08-09 21:55:18 PDT
Why is the AddLeaf change needed?
Comment 20 Boris Zbarsky [:bz] (TPAC) 2010-08-09 21:56:36 PDT
Comment on attachment 464256 [details] [diff] [review]
Fix a mistake

r=me
Comment 21 :Ehsan Akhgari 2010-08-10 08:17:41 PDT
(In reply to comment #19)
> Why is the AddLeaf change needed?

AddLeaf is what adds the text node under title to the document fragment.
Comment 22 Boris Zbarsky [:bz] (TPAC) 2010-08-10 08:52:07 PDT
So why does the normal fragment parser not need that?
Comment 23 :Ehsan Akhgari 2010-08-10 15:27:15 PDT
(In reply to comment #22)
> So why does the normal fragment parser not need that?

Because in case of the normal fragment parser, the title element would still be added, and will just be popped off in CloseContainer when </head> is encountered.  But <title> is not in the whitelist for the paranoid fragment sink, so the title text ends up getting added as a direct child of the fragment root.
Comment 24 Boris Zbarsky [:bz] (TPAC) 2010-08-10 15:47:32 PDT
Comment on attachment 464254 [details] [diff] [review]
Patch (v1)

OK, great.  Now can we get all that into a comment in the code? ;)

I hope there are no other whitelisted leaves that we care about, btw...
Comment 25 :Ehsan Akhgari 2010-08-10 16:18:42 PDT
(In reply to comment #24)
> Comment on attachment 464254 [details] [diff] [review]
> Patch (v1)
> 
> OK, great.  Now can we get all that into a comment in the code? ;)

Of course!  I'll do that before landing.

> I hope there are no other whitelisted leaves that we care about, btw...

I tried to think of others, but couldn't come up with any.  I really hate this part of the code base, BTW.  It has been extremely hard for me to analyze all the cases before actually seeing bugs coming from users, so I hope this is the last regression that I'm fixing here.  :-)
Comment 26 :Ehsan Akhgari 2010-08-10 18:21:56 PDT
The bug fix itself:

http://hg.mozilla.org/mozilla-central/rev/0aa4ad290221

The test fix:

http://hg.mozilla.org/mozilla-central/rev/40ad71a60486

The comment addition (forgot to add it to the original changeset):

http://hg.mozilla.org/mozilla-central/rev/1b12ed9d1c48
Comment 27 Doug Turner (:dougt) 2010-08-10 21:09:05 PDT
the test fix light up Md3:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281492510.1281494162.8440.gz
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-08-10 21:21:56 PDT
All three changesets were backed out for the leaks.
Comment 29 christian 2010-08-11 10:30:54 PDT
Comment on attachment 464254 [details] [diff] [review]
Patch (v1)

a=LegNeato for 1.9.2.9 and 1.9.1.12.

Please note that for these releases code-freeze is this Thursday, 2010-08-12 @ 11:59 pm PST. This needs to be landed as soon as possible.
Comment 30 :Ehsan Akhgari 2010-08-11 12:02:58 PDT
Pushed the code fixes again...

http://hg.mozilla.org/mozilla-central/rev/962cb94339bf
Comment 32 :Ehsan Akhgari 2010-08-11 14:23:12 PDT
Filed bug 586436 to get the test fixes in as well.
Comment 33 Al Billings [:abillings] 2010-08-16 16:03:48 PDT
Verified for 1.9.1 and 1.9.2 with passing mochitest.

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