181158 - crash when trying to access context menu [@ -[BrowserWindowController getContextMenu]]

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect, P2)

Description

[kaldari]

Haven't been able to reproduce this one yet, but I got a crash on the following
page:
http://www.jigzone.com/ms/z.php?q=im
when control-clicking on the jigsaw puzzle to pull up the context menu using
Chimera 2002111704 in 10.2.2. Will attach stack trace.

Comment 1

[kaldari]

Attachment: stack trace

Comment 2

[kaldari]

When control-clicking on the jigsaw puzzle, 1 of 4 things will happen:

1. Nothing at all.
2. I get an HTML page context menu.
3. I get a frame context menu.
4. Chimera crashes.

The behavior seems rather random. Perhaps it is a race condition or a caching issue.

Comment 3

[Greg K.]

WorksForMe using Chimera/2002111204. I loaded the URL ten times and contextually
clicked on the puzzle. I encountered conditions 1 and 2 once each, and condition
3 the remaining times.

[e-mail], what percentage of the time do you experience condition 4?

Comment 4

[kaldari]

Actually, I've only gotten it to crash once, so may last post was a bit misleading.

Comment 5

[Simon Fraser [no longer active]]

I think this is fixed by plugin context menu fixes.

Comment 6

[Greg K.]

Crashed at -[BrowserWindowController getContextMenu] using 2004050308 (v0.8b)
while testing attachment 103143 [details] in relation to bug 185714. Stack is not
identical, though. Same issue?

Comment 7

[Greg K.]

Attachment: Crash report showing crash at -[BrowserWindowController getContextMenu] using 2004050308 (v0.8b)

Comment 8

[Greg K.]

Attachment: HTML example that caused the crash

In fact, I was testing a *variant* of that attachment to check Camino’s context
menu behavior on images embedded using IMG, EMBED and OBJECT. This is the
precise HTML file I was using. I had reloaded and force-reloaded it several
times, and had just closed the tab I was viewing it in, then loaded it in yet
another tab and was testing again (right-clicking on the images, that is).

Comment 9

[Mike Pinkerton (not reading bugmail)]

so what are the steps to repro? i can't.

Comment 10

[Greg K.]

Not sure yet. Sure wish we had TalkBack... ;)

Comment 11

[Greg K.]

(See also bug 242906, follow-on to bug 185714.)

Comment 12

[Josh Aas]

Tried reproducing this for about 10 minutes and couldn't. Until we can repro or
get some evidence that this is a more widespread problem, marking it 0.9. I
can't access talkback to see how common the crash is, but I don't hear about it
much.

Comment 13

[Greg K.]

I get 14 hits searching for "BrowserWindowController getContextMenu" using
TalkBack FastFind. Do you see that, Josh?

Comment 14

[Greg K.]

FWIW, each of them is attributed to nsCLiveconnect.cpp, line 300.

Comment 15

[Josh Aas]

That feature of Talkback find wasn't available when I tried. Anyway, 14 reports
given the thousands of testers... Is this a lot of reports? I'm not sure how
many reports a "common" crash gets. We need a consistent repro.

Comment 16

[Simon Fraser [no longer active]]

The only way I can see that this might happen in -getContextMenu is if
mContextMenuNode goes bad.

Comment 17

[Simon Fraser [no longer active]]

This crash is almost certainly caused by the dubious handling of
mContextMenuNode in the BrowserWindowController. It has an ill-defined lifetime,
and we don't hold a ref to it.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This accounts for most of my context menu-related crashes, some on Flash content
(when I don't get bug 242906 or bug 192276 instead) and others on
MediaWiki-produced pages when ctrl-clicking on headings to edit
(oncontextmenu='document.location="url"'), and the line is always in the crashed
thread somewhere on cm-crashes when the top of the stack is something else.... 
Camino usually has to have been running for "a while" before the oncontextmenu
triggers this crash.

Comment 19

[L. Perone]

Filed Talkback incident TB7853151M for this bug.
Was right-clicking on an overloaded context menu (related to bug 298458 ?)

Comment 20

[Simon Fraser [no longer active]]

The problem here is that BrowserWindowController has a raw pointer to
mContextMenuNode, which is not cleared after the context menu has been handled.
Combine that with the fact that onShowContextMenu: is not called for plugins,
and you can easily be left with stale pointers.

Comment 21

[Simon Fraser [no longer active]]

Steps to reproduce:
1. Open one tab with a page with an image, and a second tab with the HTML
   testcase from this bug.
2. Right-click on an image in the first tab, but don't choose anything from
   the menu.
3. Close the first tab.
4. In the second tab, right-click on one of the EMBEDs or OBJECTs. Choose one
   of the image-related items from the menu.
5. Note crash.

Comment 22

[Simon Fraser [no longer active]]

Attachment: Patch fragment

This is the actual fix for the bug (I'll post a bigger patch shortly). It's not
very nice. I could find no way to get called after the context menu has been
handled, either in the widget code, or the BWC code. I ended up using the
autorelease of an object to know that we've got back to the main event loop,
which is a good time to clear the context menu node etc.

Comment 23

[Simon Fraser [no longer active]]

Attachment: Full patch

Full patch. This does two other things:
1. Embellish the NSMenu extension code to fire out notificaions when menus
close,
   as well as when they show. I don't actually need this now, but it seems
useful
   to have.

2. Move raw XPCOM data member pointers in BrowserWindowController into a little

   C++ class (BWCDataOwner) that can use nsCOMPtr, and makes refcounting
easier.

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 194124 [details] [diff] [review]
Full patch

skankadellic, but reasonable.

r=pink

Comment 25

[Simon Fraser [no longer active]]

Fixed on trunk and branch.