Bugzilla@Mozilla – Bug 431086
Crash [@ nsHTMLEditor::HideResizers] with non-HTML root, changing contentEditable property a lot
Last modified: 2009-06-12 18:57:05 PDT
Summon comment box
Created attachment 318054 [details] testcase Loading the testcase makes Firefox crash [@ nsHTMLEditor::HideResizers]. The immediate cause of the crash is that mTopLeftHandle is null.
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 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.
Created attachment 345979 [details] [review] V1 with crashtest Same as v1 with review comment addressed, and crashtest. r+/sr+ peterv.
Comment on attachment 345979 [details] [review] V1 with crashtest a191=beltzner
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.
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.
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.
This patch needs approval before checkin.
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.
This patch should land -- Chris, can you take care of that today?
Created attachment 372532 [details] [review] m-c patch without test Patch without test for landing on m-c.
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.
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 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 on attachment 372532 [details] [review] m-c patch without test a1.9.1=dbaron
Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b1ef56223c4
Comment on attachment 372532 [details] [review] m-c patch without test approved for 1.9.0.10, a=dveditz for release-drivers
Can someone please land this on 1.9.0? I don't have CVS access.
I'll land this to 1.9.0.
Comment on attachment 372532 [details] [review] m-c patch without test Checked in this patch to 1.9.0
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.
This is fixed on trunk. The tests should still land when this bug uncloaks.
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
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
Doesn't seem like anything still needs to be checked in here.
Doesn't crash on 1.8.1.22pre
Crashtest patch pushed: http://hg.mozilla.org/mozilla-central/rev/d92006cd929e