Last Comment Bug 431086 - Crash [@ nsHTMLEditor::HideResizers] with non-HTML root, changing contentEditable property a lot
: Crash [@ nsHTMLEditor::HideResizers] with non-HTML root, changing contentEdit...
Status: VERIFIED FIXED
: [sg:critical?] post 1.8-branch
: crash, regression, testcase, verified1.9.0.11, verified1.9.1
Product: Core
Classification: Components
Component: Editor
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.2a1
Assigned To: Chris Pearce [:cpearce]
: editor
:
:
: 306939 336383
  Show dependency treegraph
 
Reported: 2008-04-27 13:56 PDT by Jesse Ruderman
Modified: 2009-06-12 18:57 PDT (History)
11 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.11+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsHTMLEditor::HideResizers]


Attachments
testcase (430 bytes, application/xhtml+xml)
2008-04-27 13:56 PDT, Jesse Ruderman
no flags Details
Patch v1 (4.18 KB, patch)
2008-04-28 17:18 PDT, Chris Pearce [:cpearce]
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review
V1 with crashtest (5.67 KB, patch)
2008-11-02 14:05 PST, Chris Pearce [:cpearce]
chris: review+
chris: superreview+
mbeltzner: approval1.9.1+
mbeltzner: approval1.9.1b2-
Details | Diff | Splinter Review
1.9.0.X patch (6.46 KB, patch)
2009-04-07 18:55 PDT, Chris Pearce [:cpearce]
chris: review+
chris: superreview+
Details | Diff | Splinter Review
m-c patch without test (4.59 KB, patch)
2009-04-13 19:54 PDT, Chris Pearce [:cpearce]
chris: review+
chris: superreview+
dbaron: approval1.9.1+
dveditz: approval1.9.0.11+
Details | Diff | Splinter Review
m-c crashtest patch (1.16 KB, patch)
2009-04-13 19:55 PDT, Chris Pearce [:cpearce]
chris: review+
chris: superreview+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-04-27 13:56:21 PDT
Created attachment 318054 [details]
testcase

Loading the testcase makes Firefox crash [@ nsHTMLEditor::HideResizers].  The immediate cause of the crash is that mTopLeftHandle is null.
Comment 1 Chris Pearce [:cpearce] 2008-04-28 17:18:48 PDT
Created attachment 318282 [details] [review]
Patch v1

When the testcase makes the document abspos and editable, it becomes resizable. It doesn't really make sense to have the document resizable, but regardless ShowResizers() is called from some editor batching stuff in the execCommand call. This fails, because it can't create resizer nodes which are direct descendants of the document element, but it leaves the mResizedObject field pointing to the document element. Later on when we tear down the editor, we call HideResizers(), which sees mResizedObject is !null, and so assumes that all the resizers exist, and dereferences one of them to delete it, causing the crash.

This patch:
* Moves the implementation of ShowResizers() into an inner method, when it fails, the outer will call HideResizers() so that the mResizedObject field doesn't point to something that isn't acually able to be resized.
* Fixes HideResizers() to be null safe.
Comment 2 Peter Van der Beken [:peterv] 2008-07-23 18:21:57 PDT
Comment on attachment 318282 [details] [review]
Patch v1

A little ugly, but it probably prevents a couple of crashes. I would also move up the call to GetParentNode in nsHTMLEditor::ShowResizers, (before any of the code that sets members) and return early if parentNode is null.
Comment 3 Chris Pearce [:cpearce] 2008-11-02 14:05:03 PST
Created attachment 345979 [details] [review]
V1 with crashtest

Same as v1 with review comment addressed, and crashtest. r+/sr+ peterv.
Comment 4 Mike Beltzner [:beltzner] 2008-11-26 18:23:15 PST
Comment on attachment 345979 [details] [review]
V1 with crashtest

a191=beltzner
Comment 5 Daniel Veditz 2009-04-03 00:12:43 PDT
Has this landed? The crash looks kind of bad. bclary got a "probably exploitable" from a crash in HdeResizers, and I get what looks like stack corruption (below).

Exception Faulting Address: 0x0
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:60a01329 mov esi,dword ptr [eax]

Basic Block:
    60a01329 mov esi,dword ptr [eax]
       Tainted Input Operands: eax
    60a0132b mov dword ptr [ebp-20h],eax
       Tainted Input Operands: eax
    60a0132e lea ecx,[ebp-0ch]
    60a01331 lea eax,[ebp-18h]
    60a01334 mov dword ptr [ebp-18h],ecx
    60a01337 add esi,1ch
       Tainted Input Operands: esi
    60a0133a call xul!gfxskipcharsbuilder::getallcharskept+0x19f (606bc77a)

Exception Hash (Major/Minor): 0x7b213a71.0x82f650e

Stack Trace:
xul!gfxWindowsPlatform::PrefChangedCallback+0xcd81
xul!JVM_ShutdownJVM+0x78c3
xul!JVM_ShutdownJVM+0x7baa
xul!gfxWindowsNativeDrawing::gfxWindowsNativeDrawing+0xe21c
xul!JSD_DebuggerOn+0xc451
xul!gfxTextRun::AdjustAdvancesForSyntheticBold+0x68b2
xul!JSD_DebuggerOnForUser+0xa248
xul!gfxFont::Release+0x556ab
xul!gfxTextRun::Draw+0x114f
xul!gfxTextRun::Draw+0x4039
xul!NS_CycleCollectorForget_P+0x233ef
xul!NS_CycleCollectorForget_P+0x1e72a
js3250!js_Invoke+0x2cb
js3250!JS_DHashTableFinish+0xbe7
js3250!JS_GetConstructor+0x1c2
js3250!JS_GetReservedSlot+0x9921
js3250!JS_GetReservedSlot+0x2e6c
Unknown
Unknown
Unknown
Unknown
Unknown
MOZCRT19!_realloc_crt+0x247e
js3250!JS_DHashTableOperate+0x102
nspr4!PR_Now+0x1092
nspr4!PR_ExitMonitor+0x40
xul!NS_ShutdownXPCOM_P+0x3cb5
js3250!js_Invoke+0x38e
xul!NS_CycleCollectorForget_P+0x1eea8
xul!NS_CycleCollectorForget_P+0xe438
xul!NS_InvokeByIndex_P+0x179
xul!gfxWindowsPlatform::ResolveFontName+0x4b23
xul!NS_CycleCollectorForget_P+0xd53
xul!NS_CycleCollectorForget_P+0xe74b
xul!NS_UTF16ToCString_P+0x5344
xul!gfxASurface::AddRef+0x25d9
xul!NS_UTF16ToCString_P+0xb13b
xul!gfxPlatform::IsCMSEnabled+0x8cad
xul!gfxTextRun::SetPotentialLineBreaks+0x677d
xul!gfxTextRun::SetPotentialLineBreaks+0x677d
xul!gfxWindowsPlatform::UpdateFontList+0x68a9
xul!NS_CycleCollectorSuspect_P+0x225d
xul!gfxASurface::AddRef+0x27cf
xul!gfxASurface::CheckSurfaceSize+0x2f55
Instruction Address: 0x60a01329

Description: Possible Stack Corruption
Short Description: PossibleStackCorruption
Exploitability Classification: UNKNOWN
Recommended Bug Title: Possible Stack Corruption starting at xul!gfxWindowsPlatform::PrefChangedCallback+0xcd81 (Hash=0x7b213a71.0x82f650e)

The stack trace contains one or more locations for which no symbol or module could be found. This may be a sign of stack corruption.
Comment 6 Chris Pearce [:cpearce] 2009-04-03 12:50:41 PDT
I was trying to roll this into the patch for bug 399046, but I haven't had the time to work out test-failure issues with that patch. That stack trace looks like it's from some other bug? This patch could land if you're worried about it.
Comment 7 Chris Pearce [:cpearce] 2009-04-07 18:55:22 PDT
Created attachment 371589 [details] [review]
1.9.0.X patch

Patch v1 back-ported to 1.9.0.X branch. I don't have CVS access, so someone else will need to check this in.
Comment 8 Samuel Sidler (old account; do not CC) 2009-04-07 19:54:26 PDT
This patch needs approval before checkin.
Comment 9 Samuel Sidler (old account; do not CC) 2009-04-10 15:32:30 PDT
Can we get this landed on (at least) mozilla-central before approving for 1.9.0? Be sure to *not* checkin the test until we open this bug back up.
Comment 10 Mike Shaver (:shaver) 2009-04-13 10:06:34 PDT
This patch should land -- Chris, can you take care of that today?
Comment 11 Chris Pearce [:cpearce] 2009-04-13 19:54:35 PDT
Created attachment 372532 [details] [review]
m-c patch without test

Patch without test for landing on m-c.
Comment 12 Chris Pearce [:cpearce] 2009-04-13 19:55:50 PDT
Created attachment 372533 [details] [review]
m-c crashtest patch

m-c crashtest patch. Only adds the crashtest, so we can land the fix, then later land the test.
Comment 13 Chris Pearce [:cpearce] 2009-04-13 20:10:27 PDT
Pushed "m-c patch without test" to m-c: http://hg.mozilla.org/mozilla-central/rev/81a03acd4d59

If it sticks, I'll push to 1.9.1.

Once it's stuck to both trees, someone else will need to land on 1.9.0, my hg account doesn't give me access to CVS.
Comment 14 Chris Pearce [:cpearce] 2009-04-13 21:26:34 PDT
Comment on attachment 372532 [details] [review]
m-c patch without test

May I carry forward approval for this patch? It's the previously approved patch, with the crashtest removed to prevent potential disclosure of an exploit.
Comment 15 David Baron [:dbaron] 2009-04-14 11:11:35 PDT
Comment on attachment 372532 [details] [review]
m-c patch without test

a1.9.1=dbaron
Comment 16 Chris Pearce [:cpearce] 2009-04-14 22:58:10 PDT
Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b1ef56223c4
Comment 17 Daniel Veditz 2009-04-15 15:44:02 PDT
Comment on attachment 372532 [details] [review]
m-c patch without test

approved for 1.9.0.10, a=dveditz for release-drivers
Comment 18 Chris Pearce [:cpearce] 2009-04-15 16:39:50 PDT
Can someone please land this on 1.9.0? I don't have CVS access.
Comment 19 Olli Pettay [:smaug] 2009-04-17 08:06:07 PDT
I'll land this to 1.9.0.
Comment 20 Olli Pettay [:smaug] 2009-04-17 08:27:50 PDT
Comment on attachment 372532 [details] [review]
m-c patch without test

Checked in this patch to 1.9.0
Comment 21 Daniel Veditz 2009-04-17 10:23:38 PDT
Comment on attachment 371589 [details] [review]
1.9.0.X patch

This is a redundant approval request, right? Everything you needed for 1.9.0.10 has landed on the 1.9.0 branch? clearing flag.
Comment 22 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-04-20 19:51:59 PDT
This is fixed on trunk. The tests should still land when this bug uncloaks.
Comment 23 Henrik Skupin (:whimboo) 2009-04-22 16:23:55 PDT
Verified fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090421 Minefield/3.6a1pre ID:20090421032809

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848
Comment 24 Al Billings [:abillings] 2009-05-11 15:15:36 PDT
Verified fixed for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre. It crashes 1.9.0.10.

I do notice the following asserts when I run the testcase:

###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file /Users/abillings/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 387
WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject) failed: file /Users/abillings/mozilla/content/base/src/nsContentUtils.cpp, line 2615
Comment 25 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-05-13 03:08:23 PDT
Doesn't seem like anything still needs to be checked in here.
Comment 26 Daniel Veditz 2009-05-29 10:14:00 PDT
Doesn't crash on 1.8.1.22pre
Comment 27 Jesse Ruderman 2009-06-12 18:57:05 PDT
Crashtest patch pushed:

http://hg.mozilla.org/mozilla-central/rev/d92006cd929e

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