Last Comment Bug 780908 - Crash in nsHTMLEditor when forwarding email from PayPal occurs every time and in safe mode.
: Crash in nsHTMLEditor when forwarding email from PayPal occurs every time and...
Status: RESOLVED FIXED
[tbird topcrash][gs]
: crash, regression, testcase, topcrash
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 15 Branch
: All All
: -- critical (vote)
: Thunderbird 17.0
Assigned To: Kent James (:rkent)
:
:
Mentors:
https://getsatisfaction.com/mozilla_m...
: 792703 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-07 08:34 PDT by 4miller
Modified: 2012-12-10 14:09 PST (History)
13 users (show)
rkent: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
-
fixed
fixed


Attachments
This is message that causes crash when forwarding (9.54 KB, text/plain)
2012-08-10 07:38 PDT, 4miller
no flags Details
Rev1: check that head is before body (1.70 KB, patch)
2012-08-10 15:29 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
Rev2: add a test (6.52 KB, patch)
2012-08-16 12:25 PDT, Kent James (:rkent)
ehsan: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description 4miller 2012-08-07 08:34:55 PDT
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
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2012-08-07 13:20:32 PDT
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.
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2012-08-07 13:25:12 PDT
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
Comment 3 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-07 16:08:44 PDT
The crash volume on this is not high enough to block Firefox release, un-tracking.
Comment 4 Mark Banner (:standard8, afk until Dec) 2012-08-08 12:38:52 PDT
Can we get an example email that has this effect? That would make it much easier to debug
Comment 5 :Ehsan Akhgari 2012-08-08 12:39:46 PDT
Not a core bug, we're just being passed a bad string.  Assigning to mconley to investigate.
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2012-08-08 12:58:16 PDT
Larry, can you attach a test message to the bug report with "Add an attachment"
Comment 7 4miller 2012-08-08 14:07:57 PDT
(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.
Comment 8 4miller 2012-08-10 07:38:21 PDT
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.
Comment 9 Kent James (:rkent) 2012-08-10 09:04:23 PDT
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).
Comment 10 Kent James (:rkent) 2012-08-10 09:43:51 PDT
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.
Comment 11 Kent James (:rkent) 2012-08-10 15:29:12 PDT
Created attachment 651015 [details] [diff] [review]
Rev1: check that head is before body

This patch prevents the crash.
Comment 12 :Ehsan Akhgari 2012-08-14 08:06:09 PDT
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!
Comment 13 neil@parkwaycc.co.uk 2012-08-15 10:07:18 PDT
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>.)
Comment 14 :Ehsan Akhgari 2012-08-15 12:54:11 PDT
(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.
Comment 15 Kent James (:rkent) 2012-08-16 12:25:57 PDT
Created attachment 652520 [details] [diff] [review]
Rev2: add a test
Comment 16 :Ehsan Akhgari 2012-08-16 12:54:34 PDT
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

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

Looks great!
Comment 17 Kent James (:rkent) 2012-08-17 08:41:31 PDT
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

Checked in https://hg.mozilla.org/integration/mozilla-inbound/rev/37f9c2c24931
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-08-17 19:23:04 PDT
https://hg.mozilla.org/mozilla-central/rev/37f9c2c24931
Comment 19 Wayne Mery (:wsmwk, NI for questions) 2012-08-29 12:04:07 PDT
#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 20 Wayne Mery (:wsmwk, NI for questions) 2012-08-29 12:16:55 PDT
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
Comment 21 Kent James (:rkent) 2012-08-29 12:37:10 PDT
(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.
Comment 22 Kent James (:rkent) 2012-08-29 13:19:06 PDT
Wayne, this is fixed in current aurora. Also, this is a core patch so it needs approval in core, not comm.
Comment 23 Wayne Mery (:wsmwk, NI for questions) 2012-08-29 20:09:04 PDT
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
Comment 24 Alex Keybl [:akeybl] 2012-08-31 16:06:07 PDT
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.
Comment 25 Kent James (:rkent) 2012-08-31 16:48:09 PDT
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?
Comment 26 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-01 07:57:43 PDT
You can do it.
Comment 27 :Ehsan Akhgari 2012-09-01 08:13:41 PDT
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.
Comment 28 Kent James (:rkent) 2012-09-01 15:38:29 PDT
Comment on attachment 652520 [details] [diff] [review]
Rev2: add a test

Checked in https://hg.mozilla.org/releases/mozilla-beta/rev/c892527135cd
Comment 29 Mark Banner (:standard8, afk until Dec) 2012-09-07 08:34:34 PDT
I landed this on a Thunderbird specific relbranch for TB 15.0.1:

https://hg.mozilla.org/releases/mozilla-release/rev/a347058c607a
Comment 30 Justin Wood (:Callek) 2012-09-07 11:51:13 PDT
(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
Comment 31 Wayne Mery (:wsmwk, NI for questions) 2012-12-10 14:09:50 PST
*** Bug 792703 has been marked as a duplicate of this bug. ***

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