Closed Bug 431086 Opened 17 years ago Closed 16 years ago

Crash [@ nsHTMLEditor::HideResizers] with non-HTML root, changing contentEditable property a lot

Categories

(Core :: DOM: Editor, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: cpearce)

References

Details

(5 keywords, Whiteboard: [sg:critical?] post 1.8-branch)

Crash Data

Attachments

(5 files, 1 obsolete file)

Attached file testcase
Loading the testcase makes Firefox crash [@ nsHTMLEditor::HideResizers]. The immediate cause of the crash is that mTopLeftHandle is null.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #318282 - Flags: review?(peterv)
Comment on attachment 318282 [details] [diff] [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.
Attachment #318282 - Flags: superreview+
Attachment #318282 - Flags: review?(peterv)
Attachment #318282 - Flags: review+
Same as v1 with review comment addressed, and crashtest. r+/sr+ peterv.
Attachment #318282 - Attachment is obsolete: true
Attachment #345979 - Flags: superreview+
Attachment #345979 - Flags: review+
Attachment #345979 - Flags: approval1.9.1b2?
Keywords: checkin-needed
Attachment #345979 - Flags: approval1.9.1b2?
Attachment #345979 - Flags: approval1.9.1b2-
Attachment #345979 - Flags: approval1.9.1?
Comment on attachment 345979 [details] [diff] [review] V1 with crashtest a191=beltzner
Attachment #345979 - Flags: approval1.9.1? → approval1.9.1+
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.
Group: core-security
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Whiteboard: [sg:critical?]
Flags: blocking1.9.0.10? → blocking1.9.0.10+
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.
Attached patch 1.9.0.X patchSplinter Review
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.
Attachment #371589 - Flags: superreview+
Attachment #371589 - Flags: review+
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing 1.9.0]
This patch needs approval before checkin.
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs landing 1.9.0] → [sg:critical?]
Attachment #371589 - Flags: approval1.9.0.10?
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?
Whiteboard: [sg:critical?] → [sg:critical?] needs trunk/1.9.1 landing
Patch without test for landing on m-c.
Attachment #372532 - Flags: superreview+
Attachment #372532 - Flags: review+
m-c crashtest patch. Only adds the crashtest, so we can land the fix, then later land the test.
Attachment #372533 - Flags: superreview+
Attachment #372533 - Flags: review+
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.
Whiteboard: [sg:critical?] needs trunk/1.9.1 landing → [sg:critical?] needs 1.9.1 landing
Attachment #372532 - Flags: approval1.9.1?
Comment on attachment 372532 [details] [diff] [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.
Attachment #372532 - Flags: approval1.9.1? → approval1.9.1+
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attachment #372532 - Flags: approval1.9.0.10?
Whiteboard: [sg:critical?] needs 1.9.1 landing → [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical?][needs approval 1.9.0]
Comment on attachment 372532 [details] [diff] [review] m-c patch without test approved for 1.9.0.10, a=dveditz for release-drivers
Attachment #372532 - Flags: approval1.9.0.10? → approval1.9.0.10+
Can someone please land this on 1.9.0? I don't have CVS access.
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs approval 1.9.0] → [sg:critical?][needs landing 1.9.0]
I'll land this to 1.9.0.
Comment on attachment 372532 [details] [diff] [review] m-c patch without test Checked in this patch to 1.9.0
Keywords: fixed1.9.0.10
Whiteboard: [sg:critical?][needs landing 1.9.0] → [sg:critical?]
Comment on attachment 371589 [details] [diff] [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.
Attachment #371589 - Flags: approval1.9.0.10?
This is fixed on trunk. The tests should still land when this bug uncloaks.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
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.
Keywords: checkin-needed
Doesn't crash on 1.8.1.22pre
Flags: wanted1.8.1.x-
Keywords: regression
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Group: core-security
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsHTMLEditor::HideResizers]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: