Closed Bug 431086 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: cpearce)

References

(Blocks 2 open bugs)

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+
Keywords: checkin-needed
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+
Comment on attachment 372532 [details] [diff] [review]
m-c patch without test

a1.9.1=dbaron
Keywords: checkin-needed
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: 11 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
Crashtest patch pushed:

http://hg.mozilla.org/mozilla-central/rev/d92006cd929e
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.