317145 - SeaMonkey crash when using shift+F10 in MailNews compose-window [@ nsEventListenerManager::PrepareToUseCaretPosition]

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Ruediger Lahl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051119 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051119 SeaMonkey/1.5a

SeaMonkey crash when using shift+F10 or the context-key on Windows-keyboards in the MailNews compose window. It don't crash, when using the right mouse-button.

Reproducible: Always

Steps to Reproduce:
1.open MailNews in SeaMonkey
2.answer a mail/posting(place cursor in the text)
3.press shift+F10 to get the context menu

Actual Results:  
crash

Expected Results:  
open the context menu

Build 2005111404 was good
Build 2005111504 has the bug

Talkback-ID is TB12028355G (Thanks to Tobias Fischer)

Comment 1

[Tobias Fischer]

I can confirm this crash [@ nsEventListenerManager::PrepareToUseCaretPosition 55f3f54f] using 
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051118 SeaMonkey/1.5a Mnenhy/0.7.2.10018 {Build ID: 2005111809}

Rüdiger, can you add the Keywords "crash" and "regression"?

Add TB-Stack:

nsEventListenerManager::PrepareToUseCaretPosition  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 2378]
nsEventListenerManager::FixContextMenuEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 2258]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 1730]
nsWindowRoot::HandleChromeEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsWindowRoot.cpp, line 255]
nsGlobalWindow::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsGlobalWindow.cpp, line 1538]
nsDocument::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocument.cpp, line 4200]
nsXULElement::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp, line 1909]
nsXULElement::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp, line 1906]
nsXULElement::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp, line 1906]
nsXULElement::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp, line 1906]
nsXULElement::HandleChromeEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp, line 2597]
nsGlobalWindow::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsGlobalWindow.cpp, line 1538]
nsDocument::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocument.cpp, line 4200]
nsGenericElement::HandleDOMEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp, line 2112]
PresShell::HandleEventInternal  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 6021]
PresShell::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 5857]
nsViewManager::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp, line 2504]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp, line 2237]
HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp, line 176]
nsWindow::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1141]
nsWindow::DispatchMouseEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 5961]
ChildWindow::DispatchMouseEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 6209]
nsWindow::WindowProc  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1330]
USER32.dll + 0x3158f (0x77e3158f)
USER32.dll + 0x2c19d (0x77e2c19d)
USER32.dll + 0x2c1ca (0x77e2c1ca)
ntdll.dll + 0x11baf (0x77891baf)
USER32.dll + 0x2c159 (0x77e2c159)
USER32.dll + 0x3158f (0x77e3158f)
USER32.dll + 0x2afa1 (0x77e2afa1)
USER32.dll + 0x2afc7 (0x77e2afc7)
nsWindow::WindowProc  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1337]
USER32.dll + 0x3158f (0x77e3158f)
USER32.dll + 0x31dc9 (0x77e31dc9)
USER32.dll + 0x31e7e (0x77e31e7e)
nsAppStartup::Run  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/components/startup/src/nsAppStartup.cpp, line 208]
main  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1737]
WinMain  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1761]
KERNEL32.dll + 0x28989 (0x77e98989)

Comment 2

[Tobias Fischer]

O.k., after a little investigation this seems to be an Regression from Checkin for Bug 271498, so I am cc'ing Brett here for now. 
Just don't know if your latest Patch for Bug 271498 will fix this too, so would be good if you can take a look on it. 

Comment 3

[Ruediger Lahl]

(In reply to comment #1)
> I can confirm this crash[...]

Thank you.

> Rüdiger, can you add the Keywords "crash" and "regression"?

[X]Done

Comment 4

[Ruediger Lahl]

(In reply to comment #2)
> O.k., after a little investigation this seems to be an Regression from Checkin
> for Bug 271498,

Hm, I can't find a Checkin for this bug. The last entry in that bug is older than the appearence of this bug.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Hm, I can't find a Checkin for this bug. The last entry in that bug is older
> than the appearence of this bug.

It was checked in without a comment, apparently...
http://bonsai.mozilla.org/cvsquery.cgi?who=brettw%25gmail.com&whotype=match&date=explicit&mindate=2005-11-14+18%3A00&maxdate=2005-11-14+18


http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/events/src/nsEventListenerManager.cpp&mark=2378&rev=#2378
widgetView probably needs a null check, GetViewFor returns null on failure.

Comment 6

[Brett Wilson]

(In reply to comment #5)
> (In reply to comment #4)
> > Hm, I can't find a Checkin for this bug. The last entry in that bug is older
> > than the appearence of this bug.
> 
> It was checked in without a comment, apparently...

Oops, sorry. 

> widgetView probably needs a null check, GetViewFor returns null on failure.

Yes, you are right. Do we know why GetViewFor would be null? ie, should I do anything special if it doesn't work?

Comment 7

[Brett Wilson]

Attachment: NULL check GetViewFor

This adds a null check for the call to GetViewFor in my previous context menu patch. I don't have a seamonkey build going, so I'm not sure if this fixes the problem, but this value should be checked anyway.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 204084 [details] [diff] [review]
NULL check GetViewFor

>Index: content/events/src/nsEventListenerManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/events/src/nsEventListenerManager.cpp,v
>retrieving revision 1.216
>diff -u -u -8 -p -r1.216 nsEventListenerManager.cpp
>--- content/events/src/nsEventListenerManager.cpp	17 Nov 2005 18:31:22 -0000	1.216
>+++ content/events/src/nsEventListenerManager.cpp	23 Nov 2005 23:53:24 -0000
>@@ -2369,16 +2369,17 @@ nsEventListenerManager::PrepareToUseCare
>   rv = caret->GetCaretCoordinates(nsICaret::eRenderingViewCoordinates,
>                                   domSelection, &caretCoords, &isCollapsed,
>                                   &view);
>   NS_ENSURE_SUCCESS(rv, PR_FALSE);
> 
>   // in case the view used for caret coordinates was something else, we need
>   // to bring those coordinates into the space of the widget view
>   nsIView* widgetView = nsIView::GetViewFor(aEventWidget);
>+  NS_ENSURE_TRUE(widgetView, PR_FALSE);

Don't use NS_ENSURE_TRUE if this is not an error case (it emits a warning to the console). r=me with that fixed.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What are these events where the widget doesn't have a view?

Comment 10

[Andrew Schultz]

With the patch applied, I get no crash, but I get 17 of these assertions:
###!!! ASSERTION: null widget ptr: 'nsnull != aWidget', file /build/andrew/moz-debug/mozilla/view/src/nsView.cpp, line 296

Comment 11

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

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

Comment 12

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

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

Comment 13

[Brett Wilson]

Attachment: Better patch to fix the actual problem

Roc, can you take a look at this? I'm not totally sure what is happening in this code, and neither does anybody else I've talked to.

The problem was caused when "if (nsnull == *aDOMEvent) {" gets triggered, and it sets the event widget to null. Then later when I ask for the view of the widget, it is null (of course). The null widget apparently gets handled somewhere else to reset the event coordinates to be the upper left (according to pinkerton).

I don't think this is necessary for context menu events, which always should have a good widget (maybe?). I moved my context menu code above this check and everything works fine.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm afraid that's not correct. We still need to create a mouse DOM event here even if the caret position is used.

pinkerton originally wrote this hack of setting the widget to null, and it's bogus IMHO. Maybe he can tell us where it's being null-tested, and we can find a better way to fix that. Seems to me that a reasonable way would be to set the widget to aPresContext()->GetViewManager()->GetRootWidget() and the event's coordinates to (0,0).

Comment 15

[Mike Pinkerton (not reading bugmail)]

roc's solution is prolly better than my (4 year old) hack. I don't recall where the widget was being testing, probably in the XPMenu code when showing the actual context menu (that's a guess).

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

severity -> blocker, I write a lot of emails, and on german keyboards, altgr is next to the context menu key, and often needed, and with this bug I crash if I miss altgr and hit the context menu key.

Comment 17

[Brett Wilson]

Attachment: Patch addressing roc's suggestions

This patch does roc's suggestion of using the root widget instead of NULLing out the event widget. I also moved some things around. I tested this on seamonkey and FF trunk.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 205988 [details] [diff] [review]
Patch addressing roc's suggestions

yeah!!! looks good.

Comment 19

[Brett Wilson]

Fix checked in

Comment 20

[Ruediger Lahl]

(In reply to comment #19)

> Fix checked in

The patch works for me in SM-2005121603. Thanks to all for fixing the bug.

Comment 21

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

Verified FIXED using trunk SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051216 Mozilla/1.0

Comment 22

[Brett Wilson]

Fixed on 1.8 branch as part of branch patch for bug 271498.