287308 - element with not correctly resolved context attribute crashes mozilla on window close [@ nsPopupSetFrame::DoLayout ]

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Nickolay_Ponomarev]

The following XUL code crashes Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8b2) Gecko/20050319 Firefox/1.0+ but not Firefox 1.0.x. It also crashes
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305

<?xml version="1.0" encoding="UTF-8"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <menubar>
    <menu>
      <menupopup id="mypopup">
        <menuitem label="Test"/>
      </menupopup>
    </menu>
  </menubar>
  <label value="Right-click me!" context="mypopup"/>
</window>

To reproduce, load this in the browser and right-click the label. Even if the
popup had children, it wouldn't show (not sure that is correct) and when closing
the window, browser crashes.

The key here is 
1) to put <menupopup/> in a place where it wouldn't be "found" by context
property. Note that if you remove it altogether, the crash doesn't occur.
2) to use context, not menucontext on <label/>. From my limited testing,
menucontext works relatively fine with this (that is not crashes, while still
not showing the popup).

I'm not able to track down the regression, the only thing I can say it happens
with 20050128 build, the earliest Firefox trunk build I installed on my system.

Talkback has never worked for me, so someone else will have to submit the report
if it is needed. The bug is reproducible for me on a clean profile for me, so it
shouldn't be much of a trouble.

Not sure if it should go here, in Menus or in a completely separate place.

Comment 1

[Nickolay_Ponomarev]

Attachment: testcase

Comment 2

[Nickolay_Ponomarev]

duh, didn't finish the summary when clicked submit..

Comment 3

[Adam Hauner]

TB4540519Y:
nsPopupSetFrame::DoLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,
line 229]
nsIFrame::Layout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 802]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1103]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1103]
nsRootBoxFrame::Reflow 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp,
line 227]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsContainerFrame.cpp,
line 954]
ViewportFrame::Reflow 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsViewportFrame.cpp,
line 240]
IncrementalReflow::Dispatch 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 907]
PresShell::ProcessReflowCommands 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 6435]
PresShell::WillPaint 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 6193]
0x778b0c24

A bit different stack:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB4540501W

Comment 4

[Xanthor Sccas]

Confirm on GNU/Linux with Moz1.8b1 (But for me, the popup is shown)
TB4590364Y

Comment 5

[[e-mail]]

I don't really know what's wrong but I'm guessing that Mozilla is getting
confused here because the CSS frame constructor is assuming that the parent menu
is going to handle the opening and closing of the popup and therefore gets
confused when the popup code tries to open it as a context menu. 

Comment 6

[Boris Zbarsky [:bzbarsky]]

This regressed between 2004-04-12-09 and 2004-04-13-09 builds.

Probably a regression from bug 209032.

Note that in a debug build the stack looks nothing like what the stack in
comment 3 looks like.  There, the crash happens in nsPopupSetFrame::Destroy, and
things look like:

#0  0x41519f85 in nsPopupSetFrame::Destroy(nsPresContext*) (this=0x889f978, 
    aPresContext=0x8755c60)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp:193
193           mPopupList->mPopupFrame->Destroy(aPresContext);
(gdb) p mPopupList->mPopupFrame
$2 = (nsIFrame *) 0x0

This seems to be something that could just happen (say via styling the popup to
be display:none).  Other code in this file does a null-check on
mPopupList->mPopupFrame, so I think this should too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes the crash for me...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 9

[Nickolay_Ponomarev]

Does not entirely fix the crash for me, for 20050330 build, reopening.

It's not the same crash, though. If you right-click the label once, it doesn't
crash anymore. You need to right-click the label twice - after second click the
popup now appears, and when you close the window it crashes.

I can file a separate bug on that, if you prefer.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Please file a separate bug on that, yes, since it's a different problem.  That's
not a recent regression -- the build I just tested from Jan 2003 crashes with
those steps.  Please cc me on the new bug, ok?

Comment 11

[Nickolay_Ponomarev]

ok, filed bug 288763.

marking this one verified: the original steps to reproduce no longer cause a
crash with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050401 Firefox/1.0+. You're right, that other bug is old.