Closed Bug 780908 Opened 13 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: