336576 - Crash when window gets destroyed during contextmenu event [@ nsDOMUIEvent::GetClientPoint]

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes current trunk Mozilla build when right clicking on the iframe.

This regressed between 2006-03-06 and 2006-03-07, probably a regression from bug 234455.

Talkback ID: TB18282936W
nsDOMUIEvent::GetClientPoint   nsDOMMouseEvent::GetClientX   XULPopupListenerImpl::LaunchPopup   XULPopupListenerImpl::PreLaunchPopup   XULPopupListenerImpl::ContextMenu   nsEventTargetChainItem::HandleEvent   nsEventTargetChainItem::HandleEventTargetChain   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventDispatcher::Dispatch   PresShell::HandleEventInternal   PresShell::HandlePositionedEvent   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

hmm, is this somehow related to Bug 304262

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fixes the crash

This fixes the crash, but IMO the context menu shouldn't become visible at all in this case - or at least not the way it does with the patch.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This fixes the crash and disables context menus when the "context" is deleted before showing the menu.

Change to nsDOMUIEvent fixes the crash, change to
nsEventListenerManager removes (IMO) extra assertion (which was
coming from aPresContext->PresShell()), and the modification to 
XULPopupListenerImpl disables context menu if primaryframe is not
found for the originalTarget of the event.

Does this make sense?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Mm... so if someone sets a content node display:none oncontextmenu we won't show the menu?  I'm not sure I'm happy with that.  The rest of the changes look good, though.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fixes the crash

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 220897 [details] [diff] [review]
fixes the crash

I think this would be good for branches too.
Should fix the problems related to Bug 304262.
(There are some TBs which have nsDOMUIEvent::GetClientPoint()  in the stack)

Comment 8

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

Verified FIXED using build 2006-05-12-07 of SeaMonkey trunk on Windows XP with https://bugzilla.mozilla.org/attachment.cgi?id=220762.

No crash.

Comment 9

[Martijn Wargers (dead)]

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

Comment 10

[Martijn Wargers (dead)]

(In reply to comment #7)
> (From update of attachment 220897 [details] [diff] [review] [edit])
> I think this would be good for branches too.

Smaug, so you want this on branches, right? This probably slipped through your attention.
This is number 14 on the list of crashes for FF1506
http://talkback-public.mozilla.org/reports/firefox/FF1506/index.html
and number 49 for FF2b2
http://talkback-public.mozilla.org/reports/firefox/FF2b2/index.html
(although this doesn't necessarily have to the same sort of crash)
Probably the patch needs a slight rewrite for the branches.

The testcase in this bug doesn't crash branch build, however, the testcase in bug 304262 does. If I understand Smaug correctly, that bug would be also be fixed by this patch for branches.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for branches

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

181 drivers say: Don't think we'd block on this, but we'll take a look at the patch as a non-blocker.

Comment 13

[Mike Schroepfer]

Comment on attachment 236388 [details] [diff] [review]
for branches

a=schrep for drivers for topcrash fix.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

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

Comment 15

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 16

[Adam Guthrie]

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

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 236388 [details] [diff] [review]
for branches

a=mconnor on behalf of drivers for 1.8.0.9 checkin

Comment 18

[juan becerra [:juanb]]

Verified, application doesn't crash using testcase in comment #1 with these:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

Notice that context menu shows in trunk when right clicking on the iframe.

Comment 20

[Martijn Wargers (dead)]

I'm seeing a broken context menu with the testcase in this bug. I filed bug 418070 for it now (although I thought someone already filed a bug for it, but I couldn't find it).