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)
Core
DOM: Editor
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)
430 bytes,
application/xhtml+xml
|
Details | |
5.67 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
beltzner
:
approval1.9.1+
beltzner
:
approval1.9.1b2-
|
Details | Diff | Splinter Review |
6.46 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
|
Details | Diff | Splinter Review |
4.59 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
dbaron
:
approval1.9.1+
dveditz
:
approval1.9.0.11+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
cpearce
:
review+
cpearce
:
superreview+
|
Details | Diff | Splinter Review |
Loading the testcase makes Firefox crash [@ nsHTMLEditor::HideResizers]. The immediate cause of the crash is that mTopLeftHandle is null.
Assignee | ||
Comment 1•17 years ago
|
||
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•17 years ago
|
||
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+
Assignee | ||
Comment 3•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Attachment #345979 -
Flags: approval1.9.1b2?
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Attachment #345979 -
Flags: approval1.9.1b2?
Attachment #345979 -
Flags: approval1.9.1b2-
Attachment #345979 -
Flags: approval1.9.1?
Comment 4•17 years ago
|
||
Comment on attachment 345979 [details] [diff] [review]
V1 with crashtest
a191=beltzner
Attachment #345979 -
Flags: approval1.9.1? → approval1.9.1+
Comment 5•16 years ago
|
||
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?]
Updated•16 years ago
|
Flags: blocking1.9.0.10? → blocking1.9.0.10+
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing 1.9.0]
Comment 8•16 years ago
|
||
This patch needs approval before checkin.
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs landing 1.9.0] → [sg:critical?]
Assignee | ||
Updated•16 years ago
|
Attachment #371589 -
Flags: approval1.9.0.10?
Comment 9•16 years ago
|
||
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•16 years ago
|
||
This patch should land -- Chris, can you take care of that today?
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] needs trunk/1.9.1 landing
Assignee | ||
Comment 11•16 years ago
|
||
Patch without test for landing on m-c.
Attachment #372532 -
Flags: superreview+
Attachment #372532 -
Flags: review+
Assignee | ||
Comment 12•16 years ago
|
||
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+
Assignee | ||
Comment 13•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #372532 -
Flags: approval1.9.1?
Assignee | ||
Comment 14•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Comment 16•16 years ago
|
||
Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b1ef56223c4
Assignee | ||
Updated•16 years ago
|
Attachment #372532 -
Flags: approval1.9.0.10?
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [sg:critical?] needs 1.9.1 landing → [sg:critical?]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs approval 1.9.0]
Comment 17•16 years ago
|
||
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+
Assignee | ||
Comment 18•16 years ago
|
||
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]
Comment 19•16 years ago
|
||
I'll land this to 1.9.0.
Comment 20•16 years ago
|
||
Comment on attachment 372532 [details] [diff] [review]
m-c patch without test
Checked in this patch to 1.9.0
Updated•16 years ago
|
Keywords: fixed1.9.0.10
Whiteboard: [sg:critical?][needs landing 1.9.0] → [sg:critical?]
Comment 21•16 years ago
|
||
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
Comment 23•16 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Comment 24•16 years ago
|
||
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
Keywords: fixed1.9.0.11 → verified1.9.0.11
Doesn't seem like anything still needs to be checked in here.
Keywords: checkin-needed
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Updated•16 years ago
|
Group: core-security
Reporter | ||
Comment 27•16 years ago
|
||
Crashtest patch pushed:
http://hg.mozilla.org/mozilla-central/rev/d92006cd929e
Flags: in-testsuite? → in-testsuite+
Updated•14 years ago
|
Crash Signature: [@ nsHTMLEditor::HideResizers]
You need to log in
before you can comment on or make changes to this bug.
Description
•