See testcase, which crashes current trunk build while holding the mouse button down on the flash embed (the green bordered box). This regressed between 2009-04-05 and 2009-04-06: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-04-05+04%3A00%3A00&enddate=2009-04-06+06%3A00%3A00 I'm guessing a regression from bug 485275. The iframe content consists of this: <embed type="application/x-shockwave-flash" style="position: fixed; outline: 10px solid green;"> http://crash-stats.mozilla.com/report/index/b06b134c-b9ad-473e-912b-870fe2090507?p=1 0 xul.dll nsDisplayListBuilder::EnterPresShell layout/base/nsDisplayList.cpp:181 1 xul.dll xul.dll@0x3b2b73 2 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:1618 3 xul.dll DisplayLine layout/generic/nsBlockFrame.cpp:6055 4 xul.dll nsBlockFrame::BuildDisplayList layout/generic/nsBlockFrame.cpp:6134 5 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:1576 6 xul.dll DisplayLine layout/generic/nsBlockFrame.cpp:6055 7 xul.dll nsBlockFrame::BuildDisplayList layout/generic/nsBlockFrame.cpp:6134 8 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:1618 9 xul.dll CanvasFrame::BuildDisplayList layout/generic/nsHTMLFrame.cpp:502 10 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:1576 11 xul.dll nsGfxScrollFrameInner::BuildDisplayList layout/generic/nsGfxScrollFrame.cpp:1349 12 xul.dll nsHTMLScrollFrame::BuildDisplayList layout/generic/nsGfxScrollFrame.h:260 13 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:1576 14 xul.dll ViewportFrame::BuildDisplayList layout/generic/nsViewportFrame.cpp:109 15 xul.dll nsIFrame::BuildDisplayListForStackingContext layout/generic/nsFrame.cpp:1286 16 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:1051 17 xul.dll PresShell::Paint layout/base/nsPresShell.cpp:5573 18 xul.dll nsViewManager::RenderViews view/src/nsViewManager.cpp:609 19 xul.dll nsViewManager::Refresh view/src/nsViewManager.cpp:511 20 xul.dll nsViewManager::DispatchEvent view/src/nsViewManager.cpp:1105 21 xul.dll HandleEvent view/src/nsView.cpp:167 22 xul.dll nsWindow::DispatchEvent widget/src/windows/nsWindow.cpp:947 23 xul.dll nsWindow::DispatchWindowEvent widget/src/windows/nsWindow.cpp:972 24 xul.dll nsWindow::OnPaint widget/src/windows/nsWindow.cpp:6127 25 xul.dll nsWindow::ProcessMessage widget/src/windows/nsWindow.cpp:4279 26 xul.dll nsWindow::WindowProc widget/src/windows/nsWindow.cpp:1163 27 user32.dll user32.dll@0x8733 28 user32.dll user32.dll@0x8815 29 user32.dll user32.dll@0x18e9f 30 user32.dll user32.dll@0x18eeb 31 ntdll.dll ntdll.dll@0xe472 32 xul.dll nsWindow::DispatchStarvedPaints widget/src/windows/nsWindow.cpp:4029 33 user32.dll user32.dll@0x1a4e7 34 user32.dll user32.dll@0x1b108 35 xul.dll nsWindow::DispatchPendingEvents widget/src/windows/nsWindow.cpp:4060 36 xul.dll nsReadFromRawBuffer xpcom/io/nsPipe3.cpp:1174 37 xul.dll xul.dll@0x3ebed6 38 xul.dll nsWindow::WindowProc widget/src/windows/nsWindow.cpp:1163 39 xul.dll xul.dll@0x311c61 40 @0x0

At first glance, if the part of bug 485275 that landed in that window had any effect in this stack trace then I would expect there to be two (or more) nsViewManager::RenderViews frames together in the stack trace.

Doesn't happen on Linux, so that makes it very hard for me to debug. Is it Windows only? I can put a null check on the caret in nsDisplayListBuilder::EnterPresShell. This is probably a good idea regardless of this bug as I don't think there are any guarantees on mCaret being non-null (every access to mCaret in nsPresShell.cpp does a null check for example). But that doesn't answer why we are only seeing this now.

It's enough just to have a plugin in an iframe that has position: fixed, click it to give it focus, and then click reload, and that will cause it to crash. Reloading just the iframe doesn't do it, neither does back/forward on the iframe or the root document. If anybody that does Windows builds wants to comment out lines 599 & 600 of nsViewManager.cpp ("if (displayRootVM && displayRootVM != this)" and "return displayRootVM->RenderViews(aView, aRC, aRegion);") and try the testcase that would tell us if bug 485275 is causing the problem.

I finally got a Windows VM working to build and debug mozilla. I get a different stack trace when using a debug build, but I think it's the same underlying problem. The stack is below, but here is my analysis. What is happening is the pres shell of the iframe is in the middle of being destroyed, as part of this the plugin is hidden with a call to nsWindow::Show, this goes into kernel code, and eventually a mouse move event is sent to mozilla code (not sure exactly why, maybe Windows sends a mouse event to whatever window is now under the mouse after a hide), and this tries to build a display list to get the frame for the coordinates of the mouse event, which will fail when it recurses into the pres shell for the iframe. The landing for bug 485275 was the trigger, but not directly. Because the crash in the debug build happens in event processing, and the crash in an opt build happens during painting, I think it just upset the balance and caused this race condition to fail. Bug 336582 is a similar bug involving focus and a pres shell for a subdocument being destroyed. To fix this I think we should check in nsDisplayListBuilder::EnterPresShell if the pres shell is destroying, and return a boolean indicating to the caller if it can enter the pres shell. And here is the stack trace. ntdll.dll!7c90120e() [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll] > xpcom_core.dll!Break(const char * aMsg=0x00000000) Line 489 C++ xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=, const char * aStr=, const char * aExpr=, const char * aFile=, int aLine=) Line 356 C++ gklayout.dll!nsFrameManager::GetPrimaryFrameFor(nsIContent * aContent=0x033aafb8, int aIndexHint=-1) Line 335 + 0x16 bytes C++ gklayout.dll!PresShell::GetPrimaryFrameFor(nsIContent * aContent=0x033aafb8) Line 5092 C++ gklayout.dll!nsCSSRendering::FindRootFrame(nsIFrame * aForFrame=0x05056454) Line 943 + 0x11 bytes C++ gklayout.dll!nsCSSRendering::FindRootFrameBackground(nsIFrame * aForFrame=0x05056454) Line 985 + 0x9 bytes C++ gklayout.dll!PresShell::UpdateCanvasBackground() Line 5602 C++ gklayout.dll!nsDisplayListBuilder::EnterPresShell(nsIFrame * aReferenceFrame=0x04f365a0, const nsRect & aDirtyRect={...}) Line 176 C++ gklayout.dll!nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x03876420, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 367 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x04d30010, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=4) Line 1618 + 0xf bytes C++ gklayout.dll!DisplayLine(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aLineArea={...}, const nsRect & aDirtyRect={...}, nsLineList_iterator & aLine={...}, int aDepth=0) Line 6127 + 0x22 bytes C++ gklayout.dll!nsBlockFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 6206 + 0x1a bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x04d2fdb4, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1576 + 0xe bytes C++ gklayout.dll!DisplayLine(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aLineArea={...}, const nsRect & aDirtyRect={...}, nsLineList_iterator & aLine={...}, int aDepth=0) Line 6127 + 0x22 bytes C++ gklayout.dll!nsBlockFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 6206 + 0x1a bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x04d2fb8c, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=1) Line 1618 + 0xf bytes C++ gklayout.dll!CanvasFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 503 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x050b3d04, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1576 + 0xe bytes C++ gklayout.dll!nsGfxScrollFrameInner::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 1350 C++ gklayout.dll!nsHTMLScrollFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 261 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x050b3e84, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1576 + 0xe bytes C++ gklayout.dll!ViewportFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 109 + 0x13 bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder * aBuilder=0x0012e338, const nsRect & aDirtyRect={...}, nsDisplayList * aList=0x0012e194) Line 1287 C++ gklayout.dll!nsLayoutUtils::GetFrameForPoint(nsIFrame * aFrame=0x050b3c20, nsPoint aPt={...}, int aShouldIgnoreSuppression=0, int aIgnoreRootScrollFrame=0) Line 925 + 0x16 bytes C++ gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x039d7210, nsGUIEvent * aEvent=0x0012e738, nsEventStatus * aEventStatus=0x0012e5f4) Line 5929 + 0x1a bytes C++ gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x04d2cdd0, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012e738, int aCaptured=0) Line 1349 C++ gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x039d7210, nsEventStatus * aStatus=0x0012e688) Line 1325 + 0x1a bytes C++ gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012e738) Line 170 C++ gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012e738, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 967 + 0x3 bytes C++ gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x00000000) Line 988 C++ gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=322, unsigned int wParam=0, long lParam=5374110, int aIsContextMenuKey=0, short aButton=100) Line 6569 + 0xb bytes C++ gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=322, unsigned int wParam=0, long lParam=5374110, int aIsContextMenuKey=0, short aButton=0) Line 6716 + 0x14 bytes C++ gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=300, unsigned int wParam=0, long lParam=5374110, int aIsContextMenuKey=0, short aButton=100) Line 6562 C++ gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=300, unsigned int wParam=0, long lParam=5374110, int aIsContextMenuKey=0, short aButton=0) Line 6716 + 0x14 bytes C++ gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=512, unsigned int & wParam=0, long & lParam=5374110, long * aRetValue=0x0012ead0) Line 4442 C++ gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000202d4, unsigned int msg=0, unsigned int wParam=0, long lParam=5374110) Line 1183 + 0x14 bytes C++ user32.dll!7e418734() user32.dll!7e418816() user32.dll!7e4189cd() user32.dll!7e41929b() user32.dll!7e419402() user32.dll!7e418a10() gkwidget.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=0) Line 171 C++ gkwidget.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=0) Line 152 C++ gkwidget.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x00a7f040, int mayWait=0, unsigned int recursionDepth=1) Line 288 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0, int * result=0x0012eccc) Line 501 C++ xpcom_core.dll!NS_ProcessPendingEvents_P(nsIThread * thread=0x00000001, unsigned int timeout=100) Line 183 + 0x16 bytes C++ gkwidget.dll!nsWindow::DispatchPendingEvents() Line 4068 C++ gkwidget.dll!nsWindow::ProcessMessageForPlugin(const tagMSG & aMsg={...}, long * aResult=0x00000000, int & aCallDefWndProc=1) Line 5435 C++ gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=641, unsigned int & wParam=1, long & lParam=-1073741809, long * aRetValue=0x0012eee8) Line 4107 + 0x18 bytes C++ gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000302d6, unsigned int msg=0, unsigned int wParam=1, long lParam=-1073741809) Line 1183 + 0x14 bytes C++ user32.dll!7e418734() user32.dll!7e418816() gkwidget.dll!DetectWindowMove(int code=0, unsigned int wParam=22142644, long lParam=197334) Line 163 + 0x5 bytes C++ user32.dll!7e428ea0() user32.dll!7e428eec() ntdll.dll!7c90e473() user32.dll!7e4194be() user32.dll!7e42c174() user32.dll!7e4292e3() imm32.dll!76392b12() user32.dll!7e46b90e() user32.dll!7e46bdf2() xpcom_core.dll!nsObserverService::Release() Line 76 + 0x41 bytes C++ gkplugin.dll!nsCOMPtr<nsIObserverService>::~nsCOMPtr<nsIObserverService>() Line 510 + 0x6 bytes C++ gkplugin.dll!NS_NotifyPluginCall(unsigned int startTime=) Line 192 + 0x19 bytes C++ nspr4.dll!_MD_CURRENT_THREAD() Line 310 C nspr4.dll!PR_GetThreadPrivate(unsigned int index=1) Line 234 C xpcom_core.dll!NS_LogAddRef_P(void * aPtr=0x002d5137, unsigned long aRefcnt=1, const char * aClazz=0x00bc8dc4, unsigned int classSize=8) Line 915 + 0x12 bytes C++ nspr4.dll!PR_GetThreadPrivate(unsigned int index=1241704) Line 234 C gkwidget.dll!nsBaseAppShell::Release() Line 50 + 0x43 bytes C++ xpcom_core.dll!nsAutoLockBase::~nsAutoLockBase() Line 333 + 0xf bytes C++ xpcom_core.dll!nsComponentManagerImpl::GetService(const nsID & aClass={...}, const nsID & aIID={...}, void * * result=0x00000001) Line 1835 + 0x25 bytes C++ nspr4.dll!_MD_CURRENT_THREAD() Line 310 C user32.dll!7e42b401() gkwidget.dll!DetectWindowMove(int code=2118326563, unsigned int wParam=0, long lParam=0) Line 163 C++ gkwidget.dll!DetectWindowMove(int code=1241872, unsigned int wParam=1969043519, long lParam=55775) Line 163 + 0x5 bytes C++ MSCTFIME.IME!755c44f5() user32.dll!7e46c8cf() user32.dll!7e428eec() ntdll.dll!7c90e473() user32.dll!7e42af62() gkwidget.dll!nsWindow::Show(int bState=0) Line 1708 C++ gklayout.dll!nsPluginInstanceOwner::PrepareToStop(int aDelayedStop=1) Line 4288 C++ gklayout.dll!nsObjectFrame::StopPluginInternal(int aDelayedStop=1) Line 2058 C++ gklayout.dll!nsObjectFrame::Destroy() Line 617 C++ gklayout.dll!nsFrameList::DestroyFrames() Line 67 + 0x8 bytes C++ gklayout.dll!nsAbsoluteContainingBlock::DestroyFrames(nsIFrame * aDelegatingFrame=0x04f365a0) Line 342 C++ gklayout.dll!ViewportFrame::Destroy() Line 68 C++ gklayout.dll!nsFrameManager::Destroy() Line 291 C++ gklayout.dll!PresShell::Destroy() Line 1880 C++ gklayout.dll!DocumentViewerImpl::DestroyPresShell() Line 4255 C++ gklayout.dll!DocumentViewerImpl::Destroy() Line 1533 C++ docshell.dll!nsDocShell::Destroy() Line 4254 C++ gklayout.dll!nsFrameLoader::Finalize() Line 293 C++ gklayout.dll!nsDocument::MaybeInitializeFinalizeFrameLoaders() Line 5167 + 0x17 bytes C++ gklayout.dll!nsRunnableMethod<nsDocument,void>::Run() Line 265 C++ gklayout.dll!nsContentUtils::AddScriptRunner(nsIRunnable * aRunnable=0x03aea9e8) Line 4373 C++ gklayout.dll!nsDocument::FinalizeFrameLoader(nsFrameLoader * aLoader=0x03a66830) Line 5122 + 0x6 bytes C++ gklayout.dll!nsFrameLoader::Destroy() Line 763 + 0x1e bytes C++ gklayout.dll!nsGenericHTMLFrameElement::DestroyContent() Line 2941 C++ gklayout.dll!nsGenericElement::DestroyContent() Line 3518 + 0x12 bytes C++ gklayout.dll!nsGenericElement::DestroyContent() Line 3518 + 0x12 bytes C++ gklayout.dll!nsDocument::Destroy() Line 6886 + 0x12 bytes C++ gklayout.dll!DocumentViewerImpl::Destroy() Line 1498 C++ gklayout.dll!DocumentViewerImpl::Show() Line 1839 C++ gklayout.dll!nsPresContext::EnsureVisible(int aUnsuppressFocus=0) Line 1577 C++ gklayout.dll!PresShell::UnsuppressAndInvalidate() Line 4507 + 0xb bytes C++ gklayout.dll!PresShell::UnsuppressPainting() Line 4555 + 0x7 bytes C++ gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0) Line 1037 C++ docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x03641464, nsIChannel * aChannel=0x04d148e8, unsigned int aStatus=0) Line 5717 C++ docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x03641464, nsIRequest * aRequest=0x04d148e8, unsigned int aStateFlags=80824552, unsigned int aStatus=0) Line 5588 C++ docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x03641464, nsIRequest * aRequest=0x04d148e8, int aStateFlags=131088, unsigned int aStatus=0) Line 1259 + 0x1a bytes C++ docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x04d148e8, unsigned int aStatus=0) Line 891 C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=31883016) Line 787 C++ docshell.dll!nsDocLoader::ChildDoneWithOnload(nsIDocumentLoader * aChild=0x03b798b8) Line 205 + 0x9 bytes C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=56890448) Line 791 C++ docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x04fbcbf0, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0) Line 684 C++ necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x04fbcbf0, nsISupports * ctxt=0x00000000, unsigned int aStatus=0) Line 683 C++ gklayout.dll!nsDocument::DoUnblockOnload() Line 7028 C++ gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1) Line 6974 C++ gklayout.dll!nsDocument::DispatchContentLoadedEvents() Line 3918 C++ gklayout.dll!nsRunnableMethod<nsDocument,void>::Run() Line 265 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fc54) Line 511 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000001, int mayWait=1) Line 230 + 0xd bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 170 + 0x9 bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 194 C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00a7af98, const nsXREAppData * aAppData=0x00a79b20) Line 3361 C++ firefox.exe!NS_internal_main(int argc=1, char * * argv=0x00a7af98) Line 157 C++ firefox.exe!wmain(int argc=10989464, unsigned short * * argv=0x00a794c0) Line 112 C++ firefox.exe!__tmainCRTStartup() Line 583 + 0x19 bytes C firefox.exe!wmainCRTStartup() Line 403 C kernel32.dll!7c817077()

Comment on attachment 376673 [details] [diff] [review] patch + + rv = + aFrame->BuildDisplayListForStackingContext(&builder, dirtyRect, &list); put that all on one line + if (builder.EnterPresShell(aRootFrame, rect)) { nsresult rv = Lose the blank line Otherwise great!

I've kinda lost track of which of your bugs should land on 1.9.1 ... can you make sure that anything that needs to land on 1.9.1 is nominated for blocking-1.9.1? Thanks!!! Also, if you keep this up we should get you commit access soonish. That's a bit easier if you make a contribution to another module with a different reviewer.

(In reply to comment #6) > I've kinda lost track of which of your bugs should land on 1.9.1 ... can you > make sure that anything that needs to land on 1.9.1 is nominated for > blocking-1.9.1? Thanks!!! Done. > Also, if you keep this up we should get you commit access soonish. That's a bit > easier if you make a contribution to another module with a different reviewer. I will keep my eyes open for something to work on outside of layout. Thanks!

I was unable to get an automated test that actually crashed. This is what I got in case anyone comes along later and wants to build on it. After the patch for this bug gets landed you will need to make nsDisplayListBuilder::EnterPresShell always return true so that the crash can actually be triggered.

I just realized that this patch isn't enough :-(. Basically, handling the mouse event during frame destruction can almost certainly be turned into a crash if the event handler reaches back into the document whose frames are being destroyed. But I'm not sure how to avoid hiding or moving the plugin widget during frame destruction here. I guess scripts are blocked due to frame destruction so we don't have to worry about arbitrary badness. Still, CSS :hover rules could restyle things in arbitrary ways, say by forcing a frame reconstruct of the parent document ... bad news. I wonder if on Windows we could detect synchronous mouse events using InSendMessage and redispatch them as asynchronous events instead?

Are we missing a script blocker somewhere? Because currently PresShell::HandleEvent is very strict and doesn't allow any event handling if there are script blockers.

At the start of PresShell::HandleEvent we check if the presshell is destroying (along with a script check and a few more), but then later in the function (nsPresShell.cpp:5952) the event is retargeted at another shell without checking if that new shell is being destroyed. I was investigating adding a check for that there before, but I let it go for some reason. So we should probably mirror the checks at the start of PresShell::HandleEvent when retargeting to a different shell at least.

I think there should be a scriptblocker somewhere in the stack when destroying a presshell. Apparently there isn't.

Good, OK, so we're just missing a script blocker. There should always be one during frame destruction, so I guess we need one somewhere in here: gklayout.dll!ViewportFrame::Destroy() Line 68 C++ gklayout.dll!nsFrameManager::Destroy() Line 291 C++ gklayout.dll!PresShell::Destroy() Line 1880 C++ gklayout.dll!DocumentViewerImpl::DestroyPresShell() Line 4255 C++ gklayout.dll!DocumentViewerImpl::Destroy() Line 1533 C++ docshell.dll!nsDocShell::Destroy() Line 4254 C++ gklayout.dll!nsFrameLoader::Finalize() Line 293 C++ I'd suggest PresShell::Destroy or possibly DocumentViewerImpl::DestroyPresShell. If we have that script blocker we shouldn't need any additional protection.

Maybe we should add the script blocker to DocumentViewerImpl::DestroyPresShell and have an assertion in PresShell::Destroy that scripts have been blocked by the caller.

(In reply to comment #14) > Good, OK, so we're just missing a script blocker. There should always be one > during frame destruction, Can we also add an assertion to nsFrame::Destroy that we're in a script blocker?

Yes!

Looks like we will still need the IsDestroying check in EnterPresShell. After adding the script blockers I now get a crash during a paint when entering the subpresshell (the View Manager has a script blocker already in place). This looks like Martijn's original stack trace. So I will create a patch doing both the destroying check and adding the script blockers. Here is the stack. ntdll.dll!7c90120e() [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll] > xpcom_core.dll!Break(const char * aMsg=0x00000000) Line 489 C++ xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=, const char * aStr=, const char * aExpr=, const char * aFile=, int aLine=) Line 356 C++ gklayout.dll!nsFrameManager::GetPrimaryFrameFor(nsIContent * aContent=0x050a2a28, int aIndexHint=-1) Line 335 + 0x16 bytes C++ gklayout.dll!PresShell::GetPrimaryFrameFor(nsIContent * aContent=0x050a2a28) Line 5095 C++ gklayout.dll!nsCSSRendering::FindRootFrame(nsIFrame * aForFrame=0x054bf8dc) Line 943 + 0x11 bytes C++ gklayout.dll!nsCSSRendering::FindRootFrameBackground(nsIFrame * aForFrame=0x054bf8dc) Line 985 + 0x9 bytes C++ gklayout.dll!PresShell::UpdateCanvasBackground() Line 5605 C++ gklayout.dll!nsDisplayListBuilder::EnterPresShell(nsIFrame * aReferenceFrame=0x05b1bdc0, const nsRect & aDirtyRect={...}) Line 176 C++ gklayout.dll!nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0567f4a8, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 370 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x054b1338, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=4) Line 1621 + 0xf bytes C++ gklayout.dll!DisplayLine(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aLineArea={...}, const nsRect & aDirtyRect={...}, nsLineList_iterator & aLine={...}, int aDepth=0) Line 6127 + 0x22 bytes C++ gklayout.dll!nsBlockFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 6206 + 0x1a bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x054b10dc, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1579 + 0xe bytes C++ gklayout.dll!DisplayLine(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aLineArea={...}, const nsRect & aDirtyRect={...}, nsLineList_iterator & aLine={...}, int aDepth=0) Line 6127 + 0x22 bytes C++ gklayout.dll!nsBlockFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 6206 + 0x1a bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x054b0eb4, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=1) Line 1621 + 0xf bytes C++ gklayout.dll!CanvasFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 503 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x054909ac, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1579 + 0xe bytes C++ gklayout.dll!nsGfxScrollFrameInner::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 1350 C++ gklayout.dll!nsHTMLScrollFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 261 C++ gklayout.dll!nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder * aBuilder=0x00000000, nsIFrame * aChild=0x05490b2c, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}, unsigned int aFlags=0) Line 1579 + 0xe bytes C++ gklayout.dll!ViewportFrame::BuildDisplayList(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, const nsDisplayListSet & aLists={...}) Line 109 + 0x13 bytes C++ gklayout.dll!nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder * aBuilder=0x0012e304, const nsRect & aDirtyRect={...}, nsDisplayList * aList=0x0012e2fc) Line 1290 C++ gklayout.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext * aRenderingContext=0x03b21138, nsIFrame * aFrame=0x054908c8, const nsRegion & aDirtyRegion={...}, unsigned int aBackground=4294967295) Line 1051 + 0x13 bytes C++ gklayout.dll!PresShell::Paint(nsIView * aView=0x037bb428, nsIRenderingContext * aRenderingContext=0x03b21138, const nsRegion & aDirtyRegion={...}) Line 5641 + 0xf bytes C++ gklayout.dll!nsViewManager::RenderViews(nsView * aView=0x03acb508, nsIRenderingContext & aRC={...}, const nsRegion & aRegion={...}) Line 610 C++ gklayout.dll!nsViewManager::Refresh(nsView * aView=0x0542c208, nsIRenderingContext * aContext=0x0000003c, nsIRegion * aRegion=0x03b21138, unsigned int aUpdateFlags=1) Line 513 C++ gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x03b16ed8, nsEventStatus * aStatus=0x0012e718) Line 1107 C++ gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012e820) Line 170 C++ gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012e820, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 967 + 0x3 bytes C++ gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012e820, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 993 C++ gkwidget.dll!nsWindow::OnPaint(HDC__ * aDC=0x00000000) Line 6151 + 0x18 bytes C++ gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=15, unsigned int & wParam=0, long & lParam=0, long * aRetValue=0x0012eb38) Line 4299 + 0x9 bytes C++ gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000203ee, unsigned int msg=0, unsigned int wParam=0, long lParam=0) Line 1183 + 0x14 bytes C++ user32.dll!7e418734() user32.dll!7e418816() gkwidget.dll!DetectWindowMove(int code=0, unsigned int wParam=22142644, long lParam=132078) Line 163 + 0x5 bytes C++ user32.dll!7e428ea0() user32.dll!7e428eec() ntdll.dll!7c90e473() user32.dll!7e42aef1() user32.dll!7e42aedc() gkwidget.dll!nsWindow::DispatchStarvedPaints(HWND__ * aWnd=0x00020326, long aMsg=0) Line 4045 + 0x7 bytes C++ user32.dll!7e42a4e8() user32.dll!7e42b109() gkwidget.dll!nsWindow::DispatchPendingEvents() Line 4080 + 0xe bytes C++ gkwidget.dll!nsWindow::ProcessMessageForPlugin(const tagMSG & aMsg={...}, long * aResult=0x00000000, int & aCallDefWndProc=1) Line 5435 C++ gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=641, unsigned int & wParam=1, long & lParam=-1073741809, long * aRetValue=0x0012eee8) Line 4107 + 0x18 bytes C++ gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000203f0, unsigned int msg=0, unsigned int wParam=1, long lParam=-1073741809) Line 1183 + 0x14 bytes C++ user32.dll!7e418734() user32.dll!7e418816() gkwidget.dll!DetectWindowMove(int code=0, unsigned int wParam=22142644, long lParam=132080) Line 163 + 0x5 bytes C++ user32.dll!7e428ea0() user32.dll!7e428eec() ntdll.dll!7c90e473() user32.dll!7e4194be() user32.dll!7e42c174() user32.dll!7e4292e3() imm32.dll!76392b12() user32.dll!7e46b90e() user32.dll!7e46bdf2() xpcom_core.dll!nsObserverService::Release() Line 76 + 0x41 bytes C++ gkplugin.dll!nsCOMPtr<nsIObserverService>::~nsCOMPtr<nsIObserverService>() Line 510 + 0x6 bytes C++ gkplugin.dll!NS_NotifyPluginCall(unsigned int startTime=) Line 192 + 0x19 bytes C++ nspr4.dll!_MD_CURRENT_THREAD() Line 310 C nspr4.dll!PR_GetThreadPrivate(unsigned int index=1) Line 234 C xpcom_core.dll!NS_LogAddRef_P(void * aPtr=0x002d5137, unsigned long aRefcnt=1, const char * aClazz=0x00b8e5f4, unsigned int classSize=8) Line 915 + 0x12 bytes C++ nspr4.dll!PR_GetThreadPrivate(unsigned int index=1241704) Line 234 C gkwidget.dll!nsBaseAppShell::Release() Line 50 + 0x43 bytes C++ xpcom_core.dll!nsAutoLockBase::~nsAutoLockBase() Line 333 + 0xf bytes C++ xpcom_core.dll!nsComponentManagerImpl::GetService(const nsID & aClass={...}, const nsID & aIID={...}, void * * result=0x00000001) Line 1835 + 0x25 bytes C++ nspr4.dll!_MD_CURRENT_THREAD() Line 310 C user32.dll!7e42b401() gkwidget.dll!DetectWindowMove(int code=2118326563, unsigned int wParam=0, long lParam=0) Line 163 C++ gkwidget.dll!DetectWindowMove(int code=1241872, unsigned int wParam=1969043519, long lParam=21590) Line 163 + 0x5 bytes C++ MSCTFIME.IME!755c44f5() user32.dll!7e46c8cf() user32.dll!7e428eec() ntdll.dll!7c90e473() user32.dll!7e42af62() gkwidget.dll!nsWindow::Show(int bState=0) Line 1708 C++ gklayout.dll!nsPluginInstanceOwner::PrepareToStop(int aDelayedStop=1) Line 4288 C++ gklayout.dll!nsObjectFrame::StopPluginInternal(int aDelayedStop=1) Line 2058 C++ gklayout.dll!nsObjectFrame::Destroy() Line 617 C++ gklayout.dll!nsFrameList::DestroyFrames() Line 67 + 0x8 bytes C++ gklayout.dll!nsAbsoluteContainingBlock::DestroyFrames(nsIFrame * aDelegatingFrame=0x05b1bdc0) Line 342 C++ gklayout.dll!ViewportFrame::Destroy() Line 68 C++ gklayout.dll!nsFrameManager::Destroy() Line 291 C++ gklayout.dll!PresShell::Destroy() Line 1883 C++ gklayout.dll!DocumentViewerImpl::DestroyPresShell() Line 4256 C++ gklayout.dll!DocumentViewerImpl::Destroy() Line 1533 C++ docshell.dll!nsDocShell::Destroy() Line 4254 C++ gklayout.dll!nsFrameLoader::Finalize() Line 293 C++ gklayout.dll!nsDocument::MaybeInitializeFinalizeFrameLoaders() Line 5167 + 0x17 bytes C++ gklayout.dll!nsRunnableMethod<nsDocument,void>::Run() Line 265 C++ gklayout.dll!nsContentUtils::AddScriptRunner(nsIRunnable * aRunnable=0x05073878) Line 4373 C++ gklayout.dll!nsDocument::FinalizeFrameLoader(nsFrameLoader * aLoader=0x054184d8) Line 5122 + 0x6 bytes C++ gklayout.dll!nsFrameLoader::Destroy() Line 763 + 0x1e bytes C++ gklayout.dll!nsGenericHTMLFrameElement::DestroyContent() Line 2941 C++ gklayout.dll!nsGenericElement::DestroyContent() Line 3518 + 0x12 bytes C++ gklayout.dll!nsGenericElement::DestroyContent() Line 3518 + 0x12 bytes C++ gklayout.dll!nsDocument::Destroy() Line 6886 + 0x12 bytes C++ gklayout.dll!DocumentViewerImpl::Destroy() Line 1498 C++ gklayout.dll!DocumentViewerImpl::Show() Line 1839 C++ gklayout.dll!nsPresContext::EnsureVisible(int aUnsuppressFocus=0) Line 1577 C++ gklayout.dll!PresShell::UnsuppressAndInvalidate() Line 4510 + 0xb bytes C++ gklayout.dll!PresShell::UnsuppressPainting() Line 4558 + 0x7 bytes C++ gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0) Line 1037 C++ docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x0356b52c, nsIChannel * aChannel=0x03996ed8, unsigned int aStatus=0) Line 5717 C++ docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x0356b52c, nsIRequest * aRequest=0x03996ed8, unsigned int aStateFlags=60387032, unsigned int aStatus=0) Line 5588 C++ docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x0356b52c, nsIRequest * aRequest=0x03996ed8, int aStateFlags=131088, unsigned int aStatus=0) Line 1259 + 0x1a bytes C++ docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x03996ed8, unsigned int aStatus=0) Line 891 C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=31709712) Line 787 C++ docshell.dll!nsDocLoader::ChildDoneWithOnload(nsIDocumentLoader * aChild=0x03655888) Line 205 + 0x9 bytes C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=56014104) Line 791 C++ docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x055bbd58, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0) Line 684 C++ necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x055bbd58, nsISupports * ctxt=0x00000000, unsigned int aStatus=0) Line 683 C++ gklayout.dll!nsDocument::DoUnblockOnload() Line 7028 C++ gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1) Line 6974 C++ gklayout.dll!nsDocument::DispatchContentLoadedEvents() Line 3918 C++ gklayout.dll!nsRunnableMethod<nsDocument,void>::Run() Line 265 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fc54) Line 511 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000001, int mayWait=1) Line 230 + 0xd bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 170 + 0x9 bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 194 C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00a79d68, const nsXREAppData * aAppData=0x00a78aa0) Line 3361 C++ firefox.exe!NS_internal_main(int argc=1, char * * argv=0x00a79d68) Line 157 C++ firefox.exe!wmain(int argc=10984808, unsigned short * * argv=0x00a786b0) Line 112 C++ firefox.exe!__tmainCRTStartup() Line 583 + 0x19 bytes C firefox.exe!wmainCRTStartup() Line 403 C kernel32.dll!7c817077()

We should really avoid painting anything, or doing much of anything, while we're in frame destruction. The basic problem is that Windows sends us synchronous events while we're in frame destruction. So far we've observed mouse-move being dispatched, but that (and other input messages) fizzles out in PresShell::HandleEvent with the script-blocker in place, OK. In the above stack we have a WM_IME_SETCONTEXT message being dispatched to the plugin (according to http://wiki.winehq.org/List_Of_Windows_Messages). That would be harmless except that ProcessMessageForPlugin decides to also synchronously paint a bunch of other windows at the same time via DispatchPendingEvents. We really should avoid doing that; ignoring the paints, in PresShell::Paint say, would be bad because we'll leave window contents as unpainted garbage. The simplest thing we could do is just not call DispatchPendingEvents() when the plugin message was WM_IME_SETCONTEXT. If we observe other events being sent synchronously to the plugin we can do the same thing. It would actually be useful to fire up Spy++ --- or just some printfs in nsWindow::ProcessMessage --- to get a complete list of the Windows messages sent synchronously while we hide the plugin widget.

I used Winspector (Spy++ doesn't seem to come with the express edition of vc++) to observe the following messages being sent immediately after I clicked reload (after focusing the plugin). All messages have a matching return immediately after, except for three: two return where indicated (WM_NCDESTROY doesn't return at all). WM_PAINT WM_NCPAINT WM_ERASEBKGND WM_PAINT return WM_IME_SETCONTEXT WM_SETFOCUS WM_SHOWWINDOW WM_WINDOWPOSCHANGING WM_WINDOWPOSCHANGED WM_KILLFOCUS WM_IME_SETCONTEXT WM_CANCELMODE WM_ENABLE WM_WINDOWPOSCHANGING WM_CHILDACTIVATE WM_WINDOWPOSCHANGED WM_MOVE WM_WINDOWPOSCHANGED return WM_SETICON WM_SETICON WM_DESTROY WM_NCDESTROY no return for WM_NCDESTROY Of these only WM_IME_SETCONTEXT will cause nsWindow::ProcessMessageForPlugin to call DispatchPendingEvents(), the rest will return false and nsWindow::ProcessMessage will handle them. Making nsWindow::ProcessMessageForPlugin not call DispatchPendingEvents when the message is WM_IME_SETCONTEXT fixes the crash. I get some assertations with reloading youtube videos, but I think (hope) they are unrelated. Will try a clean build later and hopefully post a patch.

The assertions were unrelated. The checking of message type and determining when to dispatch synchronously seems kind of fragile to me. Is it not possible to remove the focus from the plugin at the very beginning of frame destruction (it gets removed in the middle of frame destruction anyway)?

The problem is that removing focus itself dispatches synchronous messags and "the beginning of frame destruction" might not be safe either. This should get easier to handle when my patches land in 1.9.2 that move plugins to be children of the toplevel window and also create a per-presshell list of windowed plugins.

Comment on attachment 377551 [details] [diff] [review] patch v3 Thanks!!! Would be nice to have an automated testcase here, but we won't block on that.

Comment on attachment 377551 [details] [diff] [review] patch v3 Closes a pretty nasty hole with crashy implications of undefined scope. Better fix it on 1.9.1. the patch is pretty safe.

Comment on attachment 377551 [details] [diff] [review] patch v3 (blockers don't need approvals)

The assert added in this bug is firing. See bug 495728.