209032 - crash if right clicking on bookmarked search result after opening the page from the sidebar

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Andras Milassin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030529

crash if i try to do the steps necessary to reproduce the bug. 



Reproducible: Always

Steps to Reproduce:
1. set mozilla to open search results in the sidebar
2. bookmark a (google) search result
3. open the bookmarks tab in the sidebar, click on the bookmarked search result and at the same moment right click on the (same) bookmark link

Actual Results:  
mozilla crashed (while trying to switch to the search tab in the sidebar)

Expected Results:  
switch to the search tab and close the opened right-click-window.

Talkback crash ID: TB20936385H and TB20931617K

Comment 1

[Andras Milassin]

Attachment: WinXp crash file

Comment 2

[Andras Milassin]

Oh, and the "bad" dll: gklayout.dll

Comment 3

[Jeff D. Hanson]

Confirmed:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030609
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5a) Gecko/20030606

Talkback:  TB20940017Q  TB20939991M  TB20939959G  TB20940261Z

Normal Google bookmark does not cause the problem.  Create a bookmark for a
search result.  Open the bookmark list in the Sidebar.  Left click and hold on
the new bookmark and then right-click while still holding down the left button.

Comment 4

[Andreas Kunz]

Confirming.
Crashes my fresh cvs debug build, stack: 

nsPopupSetFrame::ActivatePopup(nsPopupFrameList * 0x05fec148, int 0) line 563 +
9 bytes
nsPopupSetFrame::HidePopup(nsPopupSetFrame * const 0x05c15460, nsIFrame *
0x05de79c8) line 407
nsMenuPopupFrame::HideChain(nsMenuPopupFrame * const 0x05de7a50) line 1905
nsMenuDismissalListener::Rollup(nsMenuDismissalListener * const 0x05d61840) line 114
nsWindow::Destroy(nsWindow * const 0x05fec3ec) line 1606
nsView::~nsView() line 167
nsView::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsView::Destroy(nsView * const 0x05fec350) line 259 + 31 bytes
nsFrame::Destroy(nsFrame * const 0x05de79c8, nsIPresContext * 0x056e4a20) line 648
nsSplittableFrame::Destroy(nsSplittableFrame * const 0x05de79c8, nsIPresContext
* 0x056e4a20) line 72
nsContainerFrame::Destroy(nsContainerFrame * const 0x05de79c8, nsIPresContext *
0x056e4a20) line 141 + 13 bytes
nsBoxFrame::Destroy(nsBoxFrame * const 0x05de79c8, nsIPresContext * 0x056e4a20)
line 1065 + 13 bytes
nsMenuPopupFrame::Destroy(nsMenuPopupFrame * const 0x05de79c8, nsIPresContext *
0x056e4a20) line 2095
nsPopupSetFrame::Destroy(nsPopupSetFrame * const 0x05c153d8, nsIPresContext *
0x056e4a20) line 183
nsFrameList::DestroyFrames(nsIPresContext * 0x056e4a20) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x05c15250, nsIPresContext *
0x056e4a20) line 137
nsBoxFrame::Destroy(nsBoxFrame * const 0x05c15250, nsIPresContext * 0x056e4a20)
line 1065 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x056e4a20) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x05c15034, nsIPresContext *
0x056e4a20) line 137
nsBoxFrame::Destroy(nsBoxFrame * const 0x05c15034, nsIPresContext * 0x056e4a20)
line 1065 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x056e4a20) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x05c14f38, nsIPresContext *
0x056e4a20) line 137
ViewportFrame::Destroy(ViewportFrame * const 0x05c14f38, nsIPresContext *
0x056e4a20) line 68
FrameManager::Destroy(FrameManager * const 0x05e84c70) line 485
PresShell::Destroy(PresShell * const 0x03fbfea8) line 1831
DocumentViewerImpl::Destroy(DocumentViewerImpl * const 0x05e8cc48) line 1123
DocumentViewerImpl::Show(DocumentViewerImpl * const 0x041b46e8) line 1363
PresShell::UnsuppressAndInvalidate() line 4840
PresShell::UnsuppressPainting(PresShell * const 0x03fec9c8) line 4885
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x041b46e8, unsigned
int 0) line 943
nsDocShell::EndPageLoad(nsIWebProgress * 0x061995b4, nsIChannel * 0x056c4018,
unsigned int 0) line 4319
nsWebShell::EndPageLoad(nsIWebProgress * 0x061995b4, nsIChannel * 0x056c4018,
unsigned int 0) line 786
nsDocShell::OnStateChange(nsDocShell * const 0x0612228c, nsIWebProgress *
0x061995b4, nsIRequest * 0x056c4018, unsigned int 131088, unsigned int 0) line 4251
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x061995b4, nsIRequest *
0x056c4018, int 131088, unsigned int 0) line 1226
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x056c4018, unsigned int 0)
line 864
nsDocLoaderImpl::DocLoaderIsEmpty() line 762
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x061995a4, nsIRequest *
0x056c4018, nsISupports * 0x00000000, unsigned int 0) line 692
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x06199658, nsIRequest *
0x056c4018, nsISupports * 0x00000000, unsigned int 0) line 695 + 35 bytes
nsInputStreamChannel::OnStopRequest(nsInputStreamChannel * const 0x056c401c,
nsIRequest * 0x056c4150, nsISupports * 0x00000000, unsigned int 0) line 371
nsInputStreamPump::OnStateStop() line 499
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x056c4154,
nsIAsyncInputStream * 0x056c422c) line 339 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x056c4864) line 117
PL_HandleEvent(PLEvent * 0x056c4864) line 671 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x018dc550) line 606 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x018df490) line
391 + 12 bytes
nsWindow::DispatchPendingEvents() line 3624
nsWindow::ProcessMessage(unsigned int 512, unsigned int 0, long 20251000, long *
0x0012fc30) line 3970
nsWindow::WindowProc(HWND__ * 0x01c308fe, unsigned int 512, unsigned int 0, long
20251000) line 1330 + 27 bytes
USER32! 77e2a2b8()
USER32! 77e045b1()
USER32! 77e0a752()
nsAppShellService::Run(nsAppShellService * const 0x0195d0b8) line 484
main1(int 1, char * * 0x00262638, nsISupports * 0x01919020) line 1291 + 32 bytes
main(int 1, char * * 0x00262638) line 1678 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e9847c()

In
viewManager->SetViewVisibility(view, nsViewVisibility_kHide);
viewManager is 0

Bug 136388 seems to have a similar stack.

Comment 5

[Andreas Kunz]

moving and cc'ing bz (per biesi's advice)

Comment 6

[Andreas Kunz]

Also cc'ing roc - biesi now suddenly thinks it's more likely view manager...

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I _think_ the right thing here is to just add a null-check for the
viewmanager, for the same reason for which we already have a null-check for the
view.  roc, what do you think?

Comment 8

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

We could do that. But this code smells bad. Probably we should ensure the popup
is rolled up before we get to here.

Comment 9

[Anders Pedersen]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4)
Gecko/20030529
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4)
Gecko/20030529
> 
> crash if i try to do the steps necessary to reproduce the bug. 
> 
> 
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. set mozilla to open search results in the sidebar
> 2. bookmark a (google) search result
> 3. open the bookmarks tab in the sidebar, click on the bookmarked search
result and at the same moment right click on the (same) bookmark link
> 
> Actual Results:  
> mozilla crashed (while trying to switch to the search tab in the sidebar)
> 
> Expected Results:  
> switch to the search tab and close the opened right-click-window.
> 
> Talkback crash ID: TB20936385H and TB20931617K

Confirmed with:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040330
Microsoft Windows 2000 Pro 5.00.2195 SP4

talkbackid: TB8780Z

Comment 10

[Asa Dotzler [:asa]]

Ander's talkback stack looks like this:

nsPopupSetFrame::ActivatePopup[/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,[ine[65]
nsPopupSetFrame::HidePopup[/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,[ine[07]
nsMenuPopupFrame::HideChain[/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp,[ine[893]
nsMenuDismissalListener::Rollup[/mozilla/layout/xul/base/src/nsMenuDismissalListener.cpp,[ine[14]
nsWindow::Destroy[/mozilla/widget/src/windows/nsWindow.cpp,[ine[622]
nsView::~nsView[/mozilla/view/src/nsView.cpp,[ine[67]
nsView::`scalar[eleting[estructor'
nsIView::Destroy[/mozilla/view/src/nsView.cpp,[ine[53]
nsFrame::Destroy[/mozilla/layout/html/base/src/nsFrame.cpp,[ine[46]
nsSplittableFrame::Destroy[/mozilla/layout/html/base/src/nsSplittableFrame.cpp,[ine[2]
nsContainerFrame::Destroy[/mozilla/layout/html/base/src/nsContainerFrame.cpp,[ine[42]
nsBoxFrame::Destroy[/mozilla/layout/xul/base/src/nsBoxFrame.cpp,[ine[066]
nsPopupSetFrame::Destroy[/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,[ine[83]
nsFrameList::DestroyFrames[/mozilla/layout/base/src/nsFrameList.cpp,[ine[30]
nsContainerFrame::Destroy[/mozilla/layout/html/base/src/nsContainerFrame.cpp,[ine[37]
nsBoxFrame::Destroy[/mozilla/layout/xul/base/src/nsBoxFrame.cpp,[ine[066]
nsFrameList::DestroyFrames[/mozilla/layout/base/src/nsFrameList.cpp,[ine[30]
nsContainerFrame::Destroy[/mozilla/layout/html/base/src/nsContainerFrame.cpp,[ine[37]
nsBoxFrame::Destroy[/mozilla/layout/xul/base/src/nsBoxFrame.cpp,[ine[066]
nsFrameList::DestroyFrames[/mozilla/layout/base/src/nsFrameList.cpp,[ine[30]
nsContainerFrame::Destroy[/mozilla/layout/html/base/src/nsContainerFrame.cpp,[ine[37]
ViewportFrame::Destroy[/mozilla/layout/html/base/src/nsViewportFrame.cpp,[ine[8]
nsFrameManager::Destroy[/mozilla/layout/html/base/src/nsFrameManager.cpp,[ine[31]
PresShell::Destroy[/mozilla/layout/html/base/src/nsPresShell.cpp,[ine[878]
DocumentViewerImpl::Destroy[/mozilla/content/base/src/nsDocumentViewer.cpp,[ine[227]
DocumentViewerImpl::Show[/mozilla/content/base/src/nsDocumentViewer.cpp,[ine[475]
PresShell::UnsuppressAndInvalidate[/mozilla/layout/html/base/src/nsPresShell.cpp,[ine[811]
PresShell::ProcessReflowCommands[/mozilla/layout/html/base/src/nsPresShell.cpp,[ine[421]
ReflowEvent::HandleEvent[/mozilla/layout/html/base/src/nsPresShell.cpp,[ine[178]
PL_HandleEvent[/mozilla/xpcom/threads/plevent.c,[ine[74]
PL_ProcessPendingEvents[/mozilla/xpcom/threads/plevent.c,[ine[12]
_md_EventReceiverProc[/mozilla/xpcom/threads/plevent.c,[ine[415]
nsAppShellService::Run[/mozilla/xpfe/appshell/src/nsAppShellService.cpp,[ine[24]
main1[/mozilla/xpfe/bootstrap/nsAppRunner.cpp,[ine[309]
main[/mozilla/xpfe/bootstrap/nsAppRunner.cpp,[ine[713]
WinMain[/mozilla/xpfe/bootstrap/nsAppRunner.cpp,[ine[735]
WinMainCRTStartup()
KERNEL32.DLL[[x287e7[0x7c5987e7)

Comment 11

[Boris Zbarsky [:bzbarsky]]

So just like the stack in comment 4, huh?  Makes sense, since none of the code
discussed in comment 4 through comment 8 has really changed...

Comment 12

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

Attachment: fix

There's two parts to this bug. The first part is that after
nsMenuPopupFrame::Destroy on an open popup menu has destroyed most of its frame
info, it destroys its view and then widget which triggers rollup ... but rollup
requires stuff from the frame that we've already destroyed. My fix calls
HideChain() before destroying the guts of the frame, ensuring that if the menu
is open at destruction time, we'll close it while we still can.

Another problem is that while nsPopupSetFrame is destroying its popup frames,
its mPopupList is not in a consistent state (in particular it contains
already-destroyed popups), and that kills us because
nsMenuPopupFrame::HideChain needs to call back to nsPopupSetFrame to do some
work. So we need to keep the mPopupList in a consistent state as remove and
destroy its elements.

This patch fixes the bug here.

Comment 13

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

Comment on attachment 145145 [details] [diff] [review]
fix

feel like reviewing this?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145145 [details] [diff] [review]
fix

Looks reasonable.  r+sr=bzbarsky

Comment 15

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

Attachment: fix

Actually that patch is bad. We don't want to call HideChain() on a menupopup
that's not actually popped up.

This approach is better. We hide and dismiss the menupopup from
nsPopupSetFrame::Destroy, and that only applies to menupopups that are actually
popped up.

Comment 16

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

Comment on attachment 145149 [details] [diff] [review]
fix

I think this is better.

But because this code is not well understood, and this bug is not very
important, this should wait for 1.8a

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145149 [details] [diff] [review]
fix

How come you need both HideChain and DismissChain?

What happens if this code is triggered off an event (eg see
nsMenuFrame::Execute) in such that HideChain() has already been called?

Comment 18

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

Attachment: new fix

Okay. This is a more disciplined fix. If and only if the dismissal listener is
active and pointing at a popup we own, then we tell it to rollup the popup(s).
Then we go ahead and destroy the popups, keeping our list in a consistent
state.

This is the first patch that both prevents the crash reported here and doesn't
crash when I select Quit from the File menu :-).

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145231 [details] [diff] [review]
new fix

Hmm...	This code is such a spaghetti soup of methods that all do more or less
the same thing.... :(

Would you mind documenting GetCurrentMenuParent() and GetEntryByFrame() where
they are declared, since you seem to have figured out what they do?

r+sr=bzbarsky with that, though if you can poke hyatt into giving this a look
that would probably be a good idea (mail him direct; he doesn't read bugmail).

Comment 20

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

sure, I'll do that.

Comment 21

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

checked into trunk.

Comment 22

[[e-mail]]

I'd like to believe that this fixes the case of a submenu being removed from the
document while it's open... in which case the only similar issue still remaining
is the case of a menuitem being removed from its own command handler.