531175 - Crash [@ nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&)]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Crash [@ nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&)]
It's #196 in the frame poisoning list in bug 526587:
https://bugzilla.mozilla.org/attachment.cgi?id=414317

It's relatively rare, only 31 crashes in 4 weeks (29 Windows, 2 OSX).
There are crash reports for Firefox 3.0x 3.5x 3.6x and 3.7x
http://crash-stats.mozilla.com/report/list?query_search=signature&query_type=exact&query=nsPresShellEventCB%3A%3AHandleEvent%28nsEventChainPostVisitor%26%29&date=&range_value=4&range_unit=weeks&do_query=1&signature=nsPresShellEventCB%3A%3AHandleEvent%28nsEventChainPostVisitor%26%29

Stack for bp-5c9708a6-063f-4de4-8b27-013ef2091122:
@0x12f837	
nsPresShellEventCB::HandleEvent	layout/base/nsPresShell.cpp:1375
nsEventTargetChainItem::HandleEventTargetChain	content/events/src/nsEventDispatcher.cpp:346
nsEventDispatcher::Dispatch	content/events/src/nsEventDispatcher.cpp:514
PresShell::HandleEventInternal	layout/base/nsPresShell.cpp:6323
PresShell::HandlePositionedEvent	layout/base/nsPresShell.cpp:6211
PresShell::HandleEvent	layout/base/nsPresShell.cpp:6071
nsViewManager::HandleEvent	view/src/nsViewManager.cpp:1400
nsViewManager::DispatchEvent	view/src/nsViewManager.cpp:1359
HandleEvent	view/src/nsView.cpp:168
nsWindow::DispatchEvent	widget/src/windows/nsWindow.cpp:1051
nsWindow::DispatchWindowEvent	widget/src/windows/nsWindow.cpp:1071
nsWindow::DispatchMouseEvent	widget/src/windows/nsWindow.cpp:6614
ChildWindow::DispatchMouseEvent	widget/src/windows/nsWindow.cpp:6761
nsWindow::ProcessMessage	widget/src/windows/nsWindow.cpp:4618
nsWindow::WindowProc	widget/src/windows/nsWindow.cpp:1267
InternalCallWinProc	
UserCallWinProcCheckWow	
DispatchMessageWorker	
DispatchMessageW	
nsAppShell::ProcessNextNativeEvent	widget/src/windows/nsAppShell.cpp:165
nsBaseAppShell::OnProcessNextEvent	widget/src/xpwidgets/nsBaseAppShell.cpp:278
nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:508
nsBaseAppShell::Run	widget/src/xpwidgets/nsBaseAppShell.cpp:170
nsAppStartup::Run	toolkit/components/startup/src/nsAppStartup.cpp:193
PR_GetEnv	
wmain	toolkit/xre/nsWindowsWMain.cpp:110
__tmainCRTStartup	obj-firefox/memory/jemalloc/src/crtexe.c:591
BaseProcessStart

Comment 1

[Mats Palmgren (inactive)]

The problem appears to be that nsPresShell::mCurrentEventFrame points
to a destroyed frame:
http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/35bb84e06502/layout/base/nsPresShell.cpp#l1321

We reset mCurrentEventFrame and all the frames in mCurrentEventFrameStack
(if it's the same as the destroyed frame) in PresShell::ClearFrameRefs()
but not in PresShell::NotifyDestroyingFrame(), and ClearFrameRefs() is only
called for some frames:

417 nsFrame::Destroy()
...
441   shell->NotifyDestroyingFrame(this);
442 
443   if ((mState & NS_FRAME_EXTERNAL_REFERENCE) ||
444       (mState & NS_FRAME_SELECTED_CONTENT)) {
445     shell->ClearFrameRefs(this);
446   }

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#417

I can't find any code that adds NS_FRAME_EXTERNAL_REFERENCE for frames
referenced by mCurrentEventFrame or mCurrentEventFrameStack.

The straight-forward fix is to move the reset code from ClearFrameRefs()
to NotifyDestroyingFrame(). (nsFrame::Destroy() is the only consumer
of these methods on all branches AFAICT)

Or, change the type s/nsIFrame*/nsWeakFrame/ for mCurrentEventFrame and mCurrentEventFrameStack - which would add the NS_FRAME_EXTERNAL_REFERENCE bit
to those frames (and thus reach ClearFrameRefs()).
http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/35bb84e06502/layout/base/nsPresShell.cpp#l1566

Patch coming up...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Looks like we (theoretically) have the same problem for mDrawEventTargetFrame
(NS_DEBUG only though) and mFramesToDirty, so I moved the code clearing
those also.  They are now under the !mIgnoreFrameDestruction condition so
I added clearing them in PresShell::Destroy() like for mCurrentEventFrame/
mCurrentEventFrameStack.

Comment 3

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

Is this covered by Olli's patch in bug 530880?

Comment 4

[Mats Palmgren (inactive)]

Yes, that's essentially the same fix, without the assertions I added
for the mIgnoreFrameDestruction case.

Comment 5

[Mats Palmgren (inactive)]

Fixed by bug 530880.