379512 - Right-clicking sourceforge.net menu crashes Camino

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Leander Koornneef]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4pre) Gecko/20070501 Camino/1.1b+
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4pre) Gecko/20070501 Camino/1.1b+

Right-clicking the (sub)menu items on sourceforge.net crashes (recent nightly builds of) Camino reproducably. Tested and reproduced with the nightly builds 2007050104 and 2007043001 from caminobrowser.org. Also reproduced with a clean profile with the current nightly (2007050104), using http://pimpmycamino.com/parts/troubleshoot-camino

Reproducible: Always

Steps to Reproduce:
1. go to http://sourceforge.net
2. hover over menu
3. right click
Actual Results:  
crash (see attachment)

Expected Results:  
show context menu

Unable to reproduce with Firefox on OSX:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

Comment 1

[Leander Koornneef]

Attachment: OSX Crash reporter output

Comment 2

[Stuart Morgan]

Confirming. Oddly enough, control-click works fine.

Still trying to figure out if this crashes 1.0.4.

Comment 3

[Stuart Morgan]

The answer seems to be "yes", so at least it's not a regression.

Comment 4

[Stuart Morgan]

Happens on trunk too. mGeckoChild is null, so we are crashing trying to give it a gecko version of the menu event. I don't have time to look into whether that's expected at the moment; nsChildView sometimes null-checks that member and sometimes doesn't, and it's not immediately obvious whether that's sloppiness or it's only supposed to be possible for it to be null in certain cases.

Taking for investigation.

Comment 5

[Stuart Morgan]

Hm, our right-click/control-click handling strikes me as wrong; Josh, can you look at this and see if it makes sense?

Here's a control-click's path to menuForEvent:
#0  -[ChildView menuForEvent:] ...
#1  0x9335f33a in -[NSWindow sendEvent:] ...
...
mouseDown:, interestingly enough, never appears to be called. menuForEvent: pushes a context-menu click event into Gecko:
  nsMouseEvent geckoEvent(PR_TRUE, NS_CONTEXTMENU, nsnull, nsMouseEvent::eReal);
  [self convertEvent:theEvent toGeckoEvent:&geckoEvent];
  geckoEvent.button = nsMouseEvent::eRightButton;
  mGeckoChild->DispatchMouseEvent(geckoEvent);


Here's a right-click's path:
#0  -[ChildView menuForEvent:] ...
#1  0x9373a80e in -[NSView rightMouseDown:] ...
#2  0x1d33bc01 in -[ChildView rightMouseDown:] ...
#3  0x9335fbe1 in -[NSWindow sendEvent:] ...
...
rightMouseDown: pushes a right mouse click event into Gecko:
  nsMouseEvent geckoEvent(PR_TRUE, NS_MOUSE_BUTTON_DOWN, nsnull, nsMouseEvent::eReal);
  [self convertEvent:theEvent toGeckoEvent:&geckoEvent];
  geckoEvent.button = nsMouseEvent::eRightButton;
  ...
  geckoEvent.clickCount = [theEvent clickCount];
  PRBool handled = mGeckoChild->DispatchMouseEvent(geckoEvent);
Then immediately calls super's rightMouseDown: if gecko didn't handle it, causing us to fall into menuForEvent:

The fact that we appear not to send a click event in one case but do in the other doesn't seem right.


That right click event is triggering something that causes the ChildView to be gone before we get to menuForEvent: (mGeckoChild is valid before the DispatchMouseEvent, and null immediately after it), so the lack of that click is what's making control-clicking safe (although why it isn't considered handled if it's doing something, I don't know). I'll look into that part of it more tomorrow; probably I'll put together a short-term safeguard fix and file the rest as a longer-term issue is Cocoa Widget.

Comment 6

[Stuart Morgan]

For my reference when I get to this, bug 384448's patch is pretty much exactly what I was planning to do, but for trunk.

Comment 7

[Stuart Morgan]

Attachment: fix

Well, the site seems to have changed, since I can't crash it anymore. Still, here's the safety patch for any other similar cases.

Josh, I think you'll find this pretty familiar.

Comment 8

[Josh Aas]

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

   if ([ChildView  areHandScrollModifiers:[theEvent modifierFlags]]) {
     return;
   }
+  if (!mGeckoChild)
+    return;
+

Kill off the extra curly brackets on the code above so the format is the same.

   // if the handscroll flag is set, steal this event
   if (mInHandScroll) {
     [self updateHandScroll:theEvent];
     return;
   }
+  if (!mGeckoChild)
+    return;
+

Note that you might as well do mGeckoChild first because updateHandScroll just bails without mGeckoChild. I don't really care how you do it though. This is fine.

+  if (!mGeckoChild)
+    [super rightMouseUp:theEvent];

Nice catch instead of just bailing. Do we have to do something like this on trunk?

Comment 9

[Leander Koornneef]

Have the above patches been added to the code tree already? 

I may have overlooked this earlier, but current 1.6a1pre knightlies still/also crash on right-clicking sub-items of the project-specific menu on SourceForge. 
I.e., the blue/gray-ish horizontal menu containing Project, Tracker, Mailing Lists, etc. I couldn't say if this is new or if this has always been the case.

Leander

Comment 10

[Stuart Morgan]

It has not, no; it still needs a final review.

Comment 11

[Leander Koornneef]

Ok, thanks. 

On a related sidenote: I started testing the most recent knightly of Camino 2.0a1pre today. It seems that this version does not crash on the SourceForge menu. However, you must right-click twice on an item in the submenu before the 
context menu shows up. I'd assume that this is a manifestation of the same Camino 1.6 bug?

Leander

Comment 12

[Stuart Morgan]

No, the trunk already has essentially the same fix as the one attached here, so that would be a different bug.

Comment 13

[Leander Koornneef]

Ok, new bug submitted: https://bugzilla.mozilla.org/show_bug.cgi?id=386208

Leander

Comment 14

[Mark Mentovai]

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

This is Camino-only on the branch.

Comment 15

[Stuart Morgan]

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

This is branch only, so Camino only.

Comment 16

[Daniel Veditz [:dveditz]]

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

approved for 1.8.1.5, a=dveditz

Comment 17

[Stuart Morgan]

Landed on MOZILLA_1_8_BRANCH with josh's comments addressed.

Comment 18

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

Since this is in widget/src/cocoa, 1.5.1 will automatically pick it up :)