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

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Message Compose Window
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: 4miller, Assigned: rkent)

Tracking

(4 keywords)

15 Branch
Thunderbird 17.0
crash, regression, testcase, topcrash
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15-, thunderbird15 fixed, thunderbird16 fixed)

Details

(Whiteboard: [tbird topcrash][gs], crash signature, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
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"
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
Created attachment 650877 [details]
This is message that causes crash when forwarding

this is a similar message to the ones that would cause the crash when forwarding. Please protect any personal/banking information that may appear.
(Assignee)

Updated

5 years ago
Attachment #650877 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 651015 [details] [diff] [review]
Rev1: check that head is before body

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

Comment 15

5 years ago
Created attachment 652520 [details] [diff] [review]
Rev2: add a test
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+
Keywords: checkin-needed
(Assignee)

Comment 17

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

Checked in https://hg.mozilla.org/integration/mozilla-inbound/rev/37f9c2c24931
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/37f9c2c24931
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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?
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
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_int&hellip;
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+
(Assignee)

Comment 25

5 years ago
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.
(Assignee)

Comment 28

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

Checked in https://hg.mozilla.org/releases/mozilla-beta/rev/c892527135cd
(Assignee)

Updated

5 years ago
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
Duplicate of this bug: 792703
You need to log in before you can comment on or make changes to this bug.