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

VERIFIED FIXED

Status

()

Core
XUL
--
critical
VERIFIED FIXED
13 years ago
7 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: smaug)

Tracking

({crash, testcase})

Trunk
x86
Windows XP
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xul frame construction], crash signature)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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+
(Reporter)

Comment 1

13 years ago
Created attachment 179385 [details]
testcase

Comment 2

13 years ago
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]
Summary: problems with context menupopup cause a crash on window close → problems with context menupopup cause a crash on window close [@ nsFrameManager::CaptureFrameStateFor ]

Comment 3

13 years ago
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]
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.
Whiteboard: [xul frame construction]

Updated

12 years ago
Blocks: 272578

Updated

12 years ago
Depends on: 279703

Updated

12 years ago
Blocks: 336662
Created attachment 230562 [details] [diff] [review]
fixes the crash

This fixes the crash, though I'm not quite sure whether we want to take this
kind of fix.
What's the caller of the Destroy() method in question?  And why is this caller not removing the menu from its parent?
Flags: blocking1.9a2?
Blocks: 345659

Comment 7

12 years ago
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.
So perhaps we should simply disallow that?
Created attachment 230638 [details] [diff] [review]
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.
Attachment #230638 - Flags: review?(enndeakin)
No longer blocks: 345659
Do context menus on items in the bookmarks menu still work with that?
(In reply to comment #10)
> Do context menus on items in the bookmarks menu still work with that?
> 

yes
We've seen the same crash when using a <xul:tooltip> as the child, see https://bugzilla.mozilla.org/attachment.cgi?id=230294
(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
Created attachment 230712 [details] [diff] [review]
allow using submenus as context menus and tooltips

this is an alternative patch. Bit hackish though.
Created attachment 230714 [details] [diff] [review]
allow using submenus as context menus and tooltips, less hackish
Attachment #230712 - Attachment is obsolete: true

Comment 16

12 years ago
(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 on attachment 230714 [details] [diff] [review]
allow using submenus as context menus and tooltips, less hackish

Sorry, this is not good either
Attachment #230714 - Attachment is obsolete: true

Comment 18

12 years ago
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?
(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

12 years ago
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.
Attachment #230638 - Flags: review?(enndeakin) → review-
Created attachment 231617 [details] [diff] [review]
Prevent using submenus as context menus and tooltips, v2
Assignee: nobody → Olli.Pettay
Attachment #230638 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #231617 - Flags: review?(enndeakin)

Updated

12 years ago
Attachment #231617 - Flags: review?(enndeakin) → review+
Attachment #231617 - Flags: superreview?(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.
Attachment #231617 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #22)
> 
> Is it ok if we have an ancestor menu that's not a direct parent?
> 
It is.

Created attachment 231722 [details] [diff] [review]
I'll check in this
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.9a2?

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets
Crash Signature: [@ nsFrameManager::CaptureFrameStateFor ]
You need to log in before you can comment on or make changes to this bug.