301025 - Crash [@nsImageMap::IsAncestor] when removing comment in this document

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

To reproduce:
- follow the url
- Open DOM Inspector
- Remove the first comment you see in the document
Result -> crash.

I'll attach a testcase.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Click on the button to trigger the crash.

This also crashes Mozilla1.7.

From talkback ID: TB7536067X

nsImageMap::IsAncestorOf 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsImageMap.cpp,
line 962]
XPTC_InvokeByIndex 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2119]
XPC_WN_CallMethod 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1348]
js_Invoke 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line
1173]
js_Interpret 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line
3464]
js_Invoke 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line
1193]
js_InternalInvoke 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line
1270]
JS_CallFunctionValue 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line
3920]
nsJSContext::CallEventHandler 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1400]
nsJSEventListener::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/dom/src/events/nsJSEventListener.cpp,
line 184]
nsEventListenerManager::HandleEventSubType 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1580]
nsEventListenerManager::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1681]
nsGenericElement::HandleDOMEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp,
line 2126]
nsHTMLButtonElement::HandleDOMEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLButtonElement.cpp,
line 335]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6357]
PresShell::HandleEventWithTarget 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6255]
nsEventStateManager::CheckForAndDispatchClick 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp,
line 2980]
nsEventStateManager::PostHandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp,
line 1960]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6428]
PresShell::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6193]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2503]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2230]
HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1171]
nsWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 5791]
ChildWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 6037]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1348]
USER32.dll + 0x27b17 (0x77d37b17)
USER32.dll + 0x2cdce (0x77d3cdce)
USER32.dll + 0x4435 (0x77d14435)
USER32.dll + 0x9611 (0x77d19611)
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 146]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 61]
kernel32.dll + 0x1eb69 (0x77e5eb69)

Comment 2

[Mats Palmgren (inactive)]

|aContainer| is null for the #comment node (aChild) here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsDocument.cpp&rev=3.561&root=/cvsroot&mark=2231-2246#2230

and the image map is a document observer so we end up here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsImageMap.cpp&rev=3.110&root=/cvsroot&mark=1008,959,973#956

and we crash on |aContent->GetParent()| in |nsImageMap::IsAncestorOf|

I'm not sure if having |aContainer| == null is OK to begin with, but if it is,
a simple null check will do...

Boris?

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

like so?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189549 [details] [diff] [review]
Patch rev. 1

aContainer will be null any time a child of the document is removed or added
(like here).  So this code does need to deal.

That said, how about using nsContentUtils::IsDescendantOf instead of
reimplementing it (I know, this code predates the existence of that method...)?
 Note that you'd still need a null-check before calling that method.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though note that nsContentUtils::IsDescendantOf(x, x) will return true; this
code may want to compare aContent to mMap first.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 7

[Martijn Wargers (dead)]

Shouldn't you also remove the IsAncestorOf function here?
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsImageMap.h#119

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Oops, thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189965 [details] [diff] [review]
Patch rev. 3

Looks great!

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 189965 [details] [diff] [review]
Patch rev. 3

Simple null-check to prevent crash

Comment 11

[Mats Palmgren (inactive)]

Checked in to trunk 2005-07-23 18:21 PDT

-> FIXED

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-07-24-06 on Windows XP SeaMonkey trunk with
testcase: https://bugzilla.mozilla.org/attachment.cgi?id=189536.  No more crashing.