Closed Bug 780908 Opened 12 years ago Closed 12 years ago

Crash in nsHTMLEditor when forwarding email from PayPal occurs every time and in safe mode.

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Version:
15 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(firefox15-, thunderbird15 fixed, thunderbird16 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 17.0
Tracking Flags:
Tracking Status
firefox15 - ---
thunderbird15 --- fixed
thunderbird16 --- fixed

People

(Reporter: 4miller, Assigned: rkent)

References

(
URL
)

Details

(4 keywords, Whiteboard: [tbird topcrash][gs])

Crash Data

Attachments

(2 files, 1 obsolete file)

This is message that causes crash when forwarding
9.54 KB, text/plain
Details
Rev2: add a test
6.52 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.60 Safari/537.1

Steps to reproduce:

I opened email message from PayPal. I selected forward


Actual results:

Message window opened briefly then Thunderbird 15 crashed. Also crashed when trying again in safe mode


Expected results:

Message window should have opened for addressing
Larry, thanks for reporting this in getsatisfaction - at https://getsatisfaction.com/mozilla_messaging/topics/crash_when_forwarding_message

#1 crash for Thunderbird 15 beta.
bp-ff8ffaa9-93f0-4bb9-b820-c6d472120803 is one of Larry's.

Crash-stats comments hint this is highly reproducible when forwarding.  And is a regression in version 15. I suggest this is needs to be fixed before shipping.
URL: https://getsatisfaction.com/mozilla_m...
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::Assign(nsAString_internal const&)]
tracking-firefox15: --- → ?
Component: General → Editor
Ever confirmed: true
Keywords: crash, regression, topcrash
Summary: Crash when forwarding email from PayPal occurs every time and in safe mode. → Crash in nsHTMLEditor when forwarding email from PayPal occurs every time and in safe mode.
Whiteboard: [tbird topcrash][gs]
Mac and linux

bp-0c248379-d7e5-44e0-98d2-c30062120731
0	libmozalloc.dylib	TouchBadMemory	memory/mozalloc/mozalloc_abort.cpp:68
1	libmozalloc.dylib	mozalloc_abort	memory/mozalloc/mozalloc_abort.cpp:89
2	XUL	NS_DebugBreak_P	xpcom/base/nsDebugImpl.cpp:382
3	XUL	nsAString_internal::Assign	xpcom/string/src/nsTSubstring.cpp:348
4	XUL	nsHTMLEditor::ReplaceHeadContentsWithHTML	
5	XUL	nsHTMLEditor::RebuildDocumentFromSource	editor/libeditor/html/nsHTMLEditor.cpp:1523
6	XUL	nsMsgCompose::ConvertAndLoadComposeWindow	mailnews/compose/src/nsMsgCompose.cpp:711
7	XUL	nsMsgCompose::BuildBodyMessageAndSignature	mailnews/compose/src/nsMsgCompose.cpp:4499
8	XUL	nsMsgCompose::InitEditor	mailnews/compose/src/nsMsgCompose.cpp:1590
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::Assign(nsAString_internal const&)] → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::Assign(nsAString_internal const&)] [@ TouchBadMemory | mozalloc_abort | NS_DebugBreak_P | nsAString_internal::Assign]
OS: Windows 7 → All
The crash volume on this is not high enough to block Firefox release, un-tracking.
tracking-firefox15: ? → -
Can we get an example email that has this effect? That would make it much easier to debug
Keywords: testcase-wanted
Not a core bug, we're just being passed a bad string.  Assigning to mconley to investigate.
Component: Editor → Message Compose Window
Keywords: testcase-wanted
Product: Core → Thunderbird
Version: 15 Branch → 15
Assignee: nobody → mconley
Keywords: testcase-wanted
Larry, can you attach a test message to the bug report with "Add an attachment"
(In reply to Wayne Mery (:wsmwk) from comment #6)
> Larry, can you attach a test message to the bug report with "Add an
> attachment"
messages came from PayPal with financial information and transaction ids, it has been deleted.
this is a similar message to the ones that would cause the crash when forwarding. Please protect any personal/banking information that may appear.
Attachment #650877 - Attachment mime type: application/octet-stream → text/plain
I can verify a crash when I put this message into a local message folder, and tried to forward it. (Thunderbird Moz16 aurora build Windows 7).
Keywords: testcase-wanted → testcase
As I understand it, what is happening is that in nsHTMLEditor::RebuildDocumentFromSource we separately detect <head and <body then the crash occurs in this call:

res = ReplaceHeadContentsWithHTML(Substring(beginhead, beginbody));


The problem is that the html string begins like this:

<.H.T.M.L.>.<.B.O.D.Y.>

but later in that string we have the fragment:

<./.T.A.B.L.E.><.B.R.>.<.B.R.>.<.h.e.a.d. .t.i.t.l.e.=.".P.a.yP.a.l."./.>.<.b.o.d.y.>.

so beginhead is after beginbody, Substring(beginhead, beginbody) has a negative length, and when you to assign it to an nsAutoString the crash occurs.
This patch prevents the crash.
Assignee: mconley → kent
Status: NEW → ASSIGNED
Attachment #651015 - Flags: review?(ehsan)
Comment on attachment 651015 [details] [diff] [review]
Rev1: check that head is before body

Review of attachment 651015 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall, just one minor comment.  Please move the condition check to after the existing statements, and in each case set the boolean to false if you detect an invalid head position.

Also, can you please write a test for this?  The test should can be based on <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug607584.xul>.  You can use that as a template, and just modify what appears on this line <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug607584.xul#62> and after it and call rebuildDocumentFromSource instead and pass in an HTML source string with the invalid head positions.  Please ping me if you need help with writing the test.

Thanks!
Attachment #651015 - Flags: review?(ehsan)
Comment on attachment 651015 [details] [diff] [review]
Rev1: check that head is before body

>   nsReadingIterator<PRUnichar> beginhead;
>   nsReadingIterator<PRUnichar> endhead;
>   aSourceString.BeginReading(beginhead);
>   aSourceString.EndReading(endhead);
>-  bool foundhead = CaseInsensitiveFindInReadable(NS_LITERAL_STRING("<head"),
>-                                                   beginhead, endhead);
>+  bool foundhead = CaseInsensitiveFindInReadable(
>+           NS_LITERAL_STRING("<head"), beginhead, endhead) &&
>+           (!foundbody || beginhead.get() < beginbody.get());
Would it be better to set endhead to beginbody so that the find stops automatically? (Similarly for finding </head>.)
(In reply to comment #13)
> Comment on attachment 651015 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=651015
> Rev1: check that head is before body
> 
> >   nsReadingIterator<PRUnichar> beginhead;
> >   nsReadingIterator<PRUnichar> endhead;
> >   aSourceString.BeginReading(beginhead);
> >   aSourceString.EndReading(endhead);
> >-  bool foundhead = CaseInsensitiveFindInReadable(NS_LITERAL_STRING("<head"),
> >-                                                   beginhead, endhead);
> >+  bool foundhead = CaseInsensitiveFindInReadable(
> >+           NS_LITERAL_STRING("<head"), beginhead, endhead) &&
> >+           (!foundbody || beginhead.get() < beginbody.get());
> Would it be better to set endhead to beginbody so that the find stops
> automatically? (Similarly for finding </head>.)

That would change the meaning of endhead...  I think just modifying the boolean is going to be clearer.
Attached patch Rev2: add a test — — Splinter Review 

        Attachment #651015 -
        Attachment is obsolete: true

        Attachment #652520 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

Review of attachment 652520 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

        Attachment #652520 -
        Flags: review?(ehsan) → review+

    
  
Flags: in-testsuite+
Hardware: x86_64 → All

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/37f9c2c24931
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0

    
    

    
  
#2 crash so far for TB15, but perhaps it will drop.

Still, it was #1 crash for TB15.0b5, so I suspect we will want take this on aurora and beta so as to drive this into TB16, at a minimum. TB16 is affected bp-392b3f4f-3dce-4562-84d4-060322120823 TB16

    
    

    
  
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

[Approval Request Comment]
Regression caused by (bug #): bug# wasn't identified
User impact if declined: users have trouble forwarding mail
Testing completed (on c-c, etc.): ~2 weeks clean
Risk to taking this patch (and alternatives if risky): no regressions filed so far. Perhaps rkent or others could better asses risk of the code changed

        Attachment #652520 -
        Flags: approval-comm-beta?

        Attachment #652520 -
        Flags: approval-comm-aurora?

    
    

    
  
(In reply to Wayne Mery (:wsmwk) from comment #20)
> Comment on attachment 652520 [details] [diff] [review]
> Rev2: add a test
> 
> [Approval Request Comment]
> Regression caused by (bug #): bug# wasn't identified
> User impact if declined: users have trouble forwarding mail
> Testing completed (on c-c, etc.): ~2 weeks clean
> Risk to taking this patch (and alternatives if risky): no regressions filed
> so far. Perhaps rkent or others could better asses risk of the code changed

The small changes here all correct cases that otherwise would crash, so I would expect minimal risk.

    
    

    
  
Wayne, this is fixed in current aurora. Also, this is a core patch so it needs approval in core, not comm.

    
  
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::Assign(nsAString_internal const&)]
[@ TouchBadMemory | mozalloc_abort | NS_DebugBreak_P | nsAString_internal::Assign] → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::Assign(nsAString_internal const&)]
[@ TouchBadMemory | mozalloc_abort | NS_DebugBreak_P | nsAString_internal::Assign]
[@ mozalloc_abort | NS_DebugBreak_P | nsAString_internal::…

    
    

    
  
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

(In reply to Kent James (:rkent) from comment #22)
> Wayne, this is fixed in current aurora. Also, this is a core patch so it
> needs approval in core, not comm.

quite right. The fog is extra thick today


[Approval Request Comment]
ref comment 20 and comment 21
String or UUID changes made by this patch: none

        Attachment #652520 -
        Flags: approval-mozilla-beta?

        Attachment #652520 -
        Flags: approval-comm-beta?

        Attachment #652520 -
        Flags: approval-comm-aurora?

    
    

    
  
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

This is a crash regression in TB15 - we'll take this on mozilla-beta in support of the next TB version.

        Attachment #652520 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
So what is the process to push this to mozilla-beta? Can I just do it, or does someone else need to make that happen?

    
    

    
  
You can do it.

    
    

    
  
If you don't have a beta checkout, or if you need help, please let me know and I will be happy to land it for you.

    
  

          status-thunderbird16:
          --- → fixed

    
    

    
  
I landed this on a Thunderbird specific relbranch for TB 15.0.1:

https://hg.mozilla.org/releases/mozilla-release/rev/a347058c607a

          status-thunderbird15:
          --- → fixed

    
    

    
  
(In reply to Mark Banner (:standard8) from comment #29)
> I landed this on a Thunderbird specific relbranch for TB 15.0.1:
> 
> https://hg.mozilla.org/releases/mozilla-release/rev/a347058c607a

fwiw, SeaMonkey will piggyback on this relbranch for our 2.12.1

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: