Closed Bug 572290 Opened 14 years ago Closed 14 years ago

<title> value of HTML signature file shows up in signature

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b4
Tracking Flags:
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: u331436, Assigned: ehsan.akhgari)

References

Details

(4 keywords, Whiteboard: [tb31needed][tb3needed])

Attachments

(3 files)

Test case
384 bytes, text/html
Details
Patch (v1)
8.68 KB, patch
bzbarsky
: review+
christian
: approval1.9.2.9+
christian
: approval1.9.1.12+
Details | Diff | Splinter Review
Fix a mistake
4.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
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 !
Version: unspecified → Trunk
will check on test profile
Same issue with test profile
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>?).
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.
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: HTML composition of message insert the word "signature" before sig → <title> value of HTML signature file shows up in signature
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).
Keywords: regression
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.
Component: MailNews: Composition → Composition
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-composition → composition
How do you actually insert the signature in the email body editable field?
(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.
Hrm, no, I mean, how, as in, what API does the program use to do this!  :-)
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
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Blocks: CVE-2010-2769
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.
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [tb31needs][tb3needs]
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Keywords: testcase-wanted
Assignee: nobody → ehsan
Attached file Test case — 

    
  
Assignee: ehsan → nobody
Component: Composition → Editor
Keywords: testcase-wanted → testcase
Product: MailNews Core → Core
QA Contact: composition → editor

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v1)
        — 
          — Splinter Review
      

    


  
We were failing to honor mIgnoreNextCloseHead in the paranoid fragment sink.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED

        Attachment #464254 -
        Flags: review?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix a mistake
        — 
          — Splinter Review
      

    


  
I also discovered some mistakes in the existing test, which is fixed by this patch.

        Attachment #464256 -
        Flags: review?(roc)

    
    

    
  
I'm not a content peer and shouldn't be reviewing these patches, sorry

    
    

    
  
Comment on attachment 464254 [details] [diff] [review]
Patch (v1)

(bz: a quick review here is appreciated as this is a branch blocker)

        Attachment #464254 -
        Flags: review?(roc) → review?(bzbarsky)

    
  

        Attachment #464256 -
        Flags: review?(roc) → review?(bzbarsky)

    
    

    
  
Why is the AddLeaf change needed?

    
    

    
  
Comment on attachment 464256 [details] [diff] [review]
Fix a mistake

r=me

        Attachment #464256 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
(In reply to comment #19)
> Why is the AddLeaf change needed?

AddLeaf is what adds the text node under title to the document fragment.

    
    

    
  
So why does the normal fragment parser not need that?

    
    

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

        Attachment #464254 -
        Flags: review?(bzbarsky) → review+

    
    

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

    
    

    
  
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4

    
  

        Attachment #464254 -
        Flags: approval1.9.2.9?

        Attachment #464254 -
        Flags: approval1.9.1.12?

    
    

    
  
All three changesets were backed out for the leaks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
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.

        Attachment #464254 -
        Flags: approval1.9.2.9?

        Attachment #464254 -
        Flags: approval1.9.2.9+

        Attachment #464254 -
        Flags: approval1.9.1.12?

        Attachment #464254 -
        Flags: approval1.9.1.12+

    
    

    
  
Pushed the code fixes again...

http://hg.mozilla.org/mozilla-central/rev/962cb94339bf

    
    

    
  
Filed bug 586436 to get the test fixes in as well.

    
    

    
  
Verified for 1.9.1 and 1.9.2 with passing mochitest.
Keywords: verified1.9.1, 
          
            verified1.9.2
Whiteboard: [tb31needs][tb3needs] → [tb31needed][tb3needed]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: