288763 - problems with context menupopup cause a crash on window close [@ nsFrameManager::CaptureFrameStateFor ]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Nickolay_Ponomarev]

The same testcase as in bug 287308 causes another crash:
1. Right-click the label once - the menupopup doesn't show.
2. Right-click the label again - the menupopup shows up.
3. Close the window - Firefox crashes.

Here's the testcase:
<?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>

I couldn't find any duplicates, although the bug is old: Boris said this also
crashes with Jan 2003 build.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050401
Firefox/1.0+

Comment 1

[Nickolay_Ponomarev]

Attachment: testcase

Comment 2

[Adam Hauner]

TB4786935Z:
nsFrameManager::CaptureFrameStateFor 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp,
line 1436]
nsFrameManager::CaptureFrameState 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp,
line 1477]
nsCSSFrameConstructor::CaptureStateFor 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 11590]

Comment 3

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050329
TB4787202K, TB4787183Q

Same file, slightly different talkback, crashing some lines later.
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB4787183Q
nsFrameManager::CaptureFrameStateFor 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp,
line 1457]
nsFrameManager::CaptureFrameState 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp,
line 1498]
nsCSSFrameConstructor::CaptureStateFor 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 11585]

Comment 4

[Boris Zbarsky [:bzbarsky]]

We sorta know why this crashes -- the menu frame has a pointer to an
already-destroyed popup that it tries to mess with.  So talkback info isn't
really helpful...

What would be useful would be documentation on how menus are theoretically
supposed to work in XUL.

Comment 5

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

Attachment: fixes the crash

This fixes the crash, though I'm not quite sure whether we want to take this
kind of fix.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What's the caller of the Destroy() method in question?  And why is this caller not removing the menu from its parent?

Comment 7

[[e-mail]]

As with bug 336662 I think there's a fundamental issue with trying to use a submenu as a context menu i.e. nobody knows who really owns the popup.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So perhaps we should simply disallow that?

Comment 9

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

Attachment: Prevent using submenus as context menus and tooltips

Fixes also bugs which this bug blocks.
I think this should go in to branches, at least 1.8.
And could be in 1.9 until Bug 279703 gets fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Do context menus on items in the bookmarks menu still work with that?

Comment 11

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

(In reply to comment #10)
> Do context menus on items in the bookmarks menu still work with that?
> 

yes

Comment 12

[Asaf Romano (gone)]

We've seen the same crash when using a <xul:tooltip> as the child, see https://bugzilla.mozilla.org/attachment.cgi?id=230294

Comment 13

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

(In reply to comment #12)
> We've seen the same crash when using a <xul:tooltip> as the child, see
> https://bugzilla.mozilla.org/attachment.cgi?id=230294
> 

That is Bug 272578

Comment 14

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

Attachment: allow using submenus as context menus and tooltips

this is an alternative patch. Bit hackish though.

Comment 15

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

Attachment: allow using submenus as context menus and tooltips, less hackish

Comment 16

[[e-mail]]

(In reply to comment #15)
>Created an attachment (id=230714)
>allow using submenus as context menus and tooltips, less hackish
Will this make it possible to use <button type="menu" contextmenu="_child"> ?

Comment 17

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

Comment on attachment 230714 [details] [diff] [review]
allow using submenus as context menus and tooltips, less hackish

Sorry, this is not good either

Comment 18

[Neil Deakin]

Comment on attachment 230638 [details] [diff] [review]
Prevent using submenus as context menus and tooltips

smaug, do you still want me to review this or are you working on a better patch?

Comment 19

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

(In reply to comment #18)
> (From update of attachment 230638 [details] [diff] [review] [edit])
> smaug, do you still want me to review this or are you working on a better
> patch?
> 

Actually, could you say your opinion whether we should support submenus
as context menus or tooltips. If not, then review, please :)

Comment 20

[Neil Deakin]

Comment on attachment 230638 [details] [diff] [review]
Prevent using submenus as context menus and tooltips

>+  nsCOMPtr<nsIContent> popup = do_QueryInterface(popupContent);
>+  nsIContent* parent = popup ? popup->GetParent() : nsnull;

Should be ok without the condition, no?

>+  if (parent && popupType == eXULPopupType_context) {

Why only check for context menus? What about popup="somemenu"?

>+      if (menu) {
>+        NS_WARNING("Trying to use a submenu as a context menu!");
>+        return NS_ERROR_FAILURE;
>+      }

I think this should return NS_OK and just make it defined that popups inside a menu cannot be opened this way. No caller checks the return value anyway, so no big deal.

>+  // Submenus can't be used as tooltips, bug 288763.
>+  // Similar code also in XULPopupListenerImpl::LaunchPopup.
>+  nsIContent* parent = mCurrentTooltip->GetParent();
>+  if (parent) {
>+    nsIDocument* doc = parent->GetOwnerDoc();
>+    nsIPresShell* presShell = doc ? doc->GetShellAt(0) : nsnull;
>+    nsIFrame* frame = presShell ? presShell->GetPrimaryFrameFor(parent) : nsnull;
>+    if (frame) {
>+      nsIMenuFrame* menu = nsnull;
>+      CallQueryInterface(frame, &menu);
>+      if (menu) {
>+        NS_WARNING("Trying to use a submenu as a tooltip!");
>+        mCurrentTooltip = nsnull;
>+        return NS_ERROR_FAILURE;
>+      }
>+    }
>+  }
>+

I think that this code should go inside GetTooltipFor instead.

Comment 21

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

Attachment: Prevent using submenus as context menus and tooltips, v2

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 231617 [details] [diff] [review]
Prevent using submenus as context menus and tooltips, v2

>Index: content/xul/content/src/nsXULPopupListener.cpp
>+    nsIDocument* doc = parent->GetOwnerDoc();

I think you want GetCurrentDoc() here.

Is it ok if we have an ancestor menu that's not a direct parent?

>Index: layout/xul/base/src/nsXULTooltipListener.cpp
>+    nsIDocument* doc = parent->GetOwnerDoc();

Same questions here.

>+  NS_ADDREF(*aTooltip = tooltip);

How about |tooltip.swap(*aTooltip)| ?

>Index: layout/xul/base/src/nsXULTooltipListener.h
>+  nsresult FindTooltip(nsIContent* aTarget, nsIContent** aTooltip);
>   nsresult GetTooltipFor(nsIContent* aTarget, nsIContent** aTooltip);

Document how these differ?

sr=bzbarsky with those nits picked.

Comment 23

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

(In reply to comment #22)
> 
> Is it ok if we have an ancestor menu that's not a direct parent?
> 
It is.

Comment 24

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

Attachment: I'll check in this

Comment 25

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

Verified FIXED in trunk build 2006-09-23-08 of SeaMonkey under Windows XP using the testcase at https://bugzilla.mozilla.org/attachment.cgi?id=179385; no crash.