164781 - Context-menus crashes in tabs (with mozdev plugins) [@ nsHTMLImageElement::SetSrcInner] Trunk N701 M130A

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[greer]

Users are commenting on crashes that happen when right-clicking/closing tabs.
Many of the comments refer to the use of RadialContext (mozdev.org) or Optimoz
pie-menu plugin. I haven't been able to reproduce this one yet, but those
plugins may be the source of most of these crashes.

Here's the stack that Talkback is showing:

nsHTMLImageElement::SetSrcInner
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLImageElement.cpp
line 834]
nsHTMLImageElement::SetSrc
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLImageElement.cpp
line 937]
XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp
line 106]
XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp line 1996]
XPC_WN_GetterSetter
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp line
1291]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 840]
js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 931]
js_SetProperty [c:/builds/seamonkey/mozilla/js/src/jsobj.c line 2617]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 2634]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c line 856]
nsXPCWrappedJSClass::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp line 1195]
nsXPCWrappedJS::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp line 430]
PrepareAndDispatch
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
line 117]
SharedStub
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp
line 139]
nsEventListenerManager::HandleEventSubType
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp line
1275]
nsEventListenerManager::HandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp line
1441]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp line 3453]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp line 3434]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp line 3434]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp line 3434]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp line 3434]
PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp line 6113]
PresShell::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp line 6031]
nsViewManager::HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp line 2098]
nsView::HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp line 306]
nsViewManager::DispatchEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp line 1909]
HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp line 83]
nsWindow::DispatchEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 1038]
nsWindow::DispatchWindowEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 1055]
nsWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 5127]
ChildWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 5382]
nsWindow::ProcessMessage
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 3834]
nsWindow::WindowProc
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp line 1304]
USER32.dll + 0x2a290 (0x77e3a290)
USER32.dll + 0x45b1 (0x77e145b1)
USER32.dll + 0x5b1d (0x77e15b1d)
nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp line 452]
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1534]
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1881]
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp line 1899]
WinMainCRTStartup()
KERNEL32.DLL + 0x1ca90 (0x77e9ca90)
Source File:
c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLImageElement.cpp line
: 834

Comment 1

[greer]

Attachment: User comments from multiple releases

Comment 2

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Attachment: CrashReporter Log

Seeing a very similar crash with Optimoz mouse gestures installed on Mac OS X
10.2, Mozilla 1.1 final.

Comment 3

[Bill McGonigle (not currently reading bugmail; please contact directly)]

The above crash log wasn't from a right click, it was from the popup menus at:

http://www.tirerack.com/upgrade_garage/WheelSearch.jsp?&autoMake=Chevrolet&autoModel=K1500+4wd+Pick-up&autoYear=1996

Comment 4

[timeless]

Attachment: talkback fingers a null document

Comment 5

[timeless]

ok, let's try to get my patch reviewed. fwiw I filed bug 165360 because other
stuff in the function I patched looked strange to some people.

as for the macosx stack err.. it looks like stack overflow, perhaps dbradley or
someone can offer ideas, but I think we'll limit this bug to the talkback stack.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 97118 [details] [diff] [review]
talkback fingers a null document

>+      NS_WARNING("mNodeInfo->GetDocument returned null, perhaps the document went away or this is anonymous content? -- bug 164781");

Please break this up into multiple lines no longer then 80 chars.

r=sicking

Comment 7

[Johnny Stenback (:jst)]

Attachment: Same fix (plus others) and more cleanup...

I don't see the need for the warning, so there's no warning in this diff, this
also fixes the problem with not returning NS_CheckContentLoadPolicy()'s error
code to the caller, and it cleans up this code a lot, and removes the
let's-nest-if's-to-death mentality from this method :-)

Comment 8

[Johnny Stenback (:jst)]

Attachment: Same fix (plus others) and more cleanup...

Comment 9

[timeless]

*** Bug 165360 has been marked as a duplicate of this bug. ***

Comment 10

[timeless]

.

Comment 11

[timeless]

Comment on attachment 97207 [details] [diff] [review]
Same fix (plus others) and more cleanup...

r=timeless w/ this change:
<+  nsCOMPtr<nsIDocument> doc = mDocument;
>+  nsCOMPtr<nsIDocument> doc(mDocument);

Comment 12

[David Bradley]

IIRC, both forms end up being the same on all but some very old compilers.
(Don't know if we compile on any of those "old" compilers) The compiler is
allowed to optimize out the assignment. I know the constructor form in some
cases can trip up some compilers because in some situations they think its a
function prototype and not a variable declaration.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97207 [details] [diff] [review]
Same fix (plus others) and more cleanup...

> -  if (NS_SUCCEEDED(result) && !mDocument) {

The whole point of that function is that we only want to load the image when
srcinner is set and we're _not_ in a document (var foo = new Image()
situation).  Perhaps a more relevant check is that we do not have a frame; not
sure on that.  In either case, doing this in cases when the frame is also doing
an image load seems wrong, no?	At least without some disentangling first (eg
with this change I think images will fire two onload events).

> +  nsAutoString url;

I know you just copied this code, but what is its reason for existing?

Comment 14

[Jay Patel [:jay]]

Is this patch gonna get checked in anytime soon?  Looking at the latest Talkback
data, there are only a handful of crashes for Mozilla 1.3 Alpha, Netscape 7.01
and recent MozillaTrunk builds.

Still, if we have a fix ready to go, might as well get it in, right?  I'm going
to make this a topcrash- based on Talkback data and leave it open in case we
want to get the patch checked in.

Comment 15

[Jay Patel [:jay]]

updating summary with N701 and M130A for future reference.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Is this patch gonna get checked in anytime soon? 

Not unless someone fixes it to be correct first...

Comment 17

[Johnny Stenback (:jst)]

Mass-reassigning bugs.

Comment 18

[Boris Zbarsky [:bzbarsky]]

This is fixed since bug 83774 landed.  The whole method in question no longer
exists; its replacement (nsImageLoadingContent::ImageURIChanged) properly
handles a null document.

Marking fixed.

Comment 19

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Verifying crash at URL in comment 3 is no longer present.