I've had various crashes when right clicking. Many times when right clicking at the top of a Pandora tab, once right clicking on a Twitter link, once right clicking on a Hacker News link. Dave Herman has also had some crashing when right clicking.

This bug was spun off from bug 699457. There is more detail there, starting with bug 699457 comment #22 and continuing through bug 699457 comment #42.

Note that the crash log from comment #0 appears to show an infinite recursion. I assume that's what we're going to need to fix. In other words, a patch that prevents that infinite recursion from ever happening should also fix this bug.

Here's a possible fix. Please try it, Andrew! And whoever else sees this bug. If you can't build yourself, you can wait a few hours for my tryserver build, which I just started.

I haven't crashed with this since I rebooted for the Safari update that somehow disabled right clicking (which I re-enabled). If I hit it again I'll try the fix.

Still no crashes for me. Kind of odd.

> Still no crashes for me. Kind of odd. Apple might silently have changed OS behavior to fix a bug in Safari. But do keep trying. However it turns out, though, I think your crashlog shows the need for my patch.

> Apple might silently have changed OS behavior to fix a bug in Safari. There's another possibility. Turning off right-click support might not have been the only settings change made by the Safari updater. Can you think of any other system settings that you might have changed, which the Safari updater might have restored to their defaults?

Not off the top of my head. I haven't customized very much. Is there any way to check? I also updated my nightly in addition to applying the Safari fix, so I suppose something in last night's nightly could have done it. I looked at the patch notes for Safari, but they weren't very detailed: http://support.apple.com/kb/DL1070

> Not off the top of my head. I haven't customized very much. Is there any way to > check? I don't think there's any way to check. > I also updated my nightly in addition to applying the Safari fix, so I suppose > something in last night's nightly could have done it. That's possible, but it's a real long shot. > I looked at the patch notes for Safari, but they weren't very detailed: > http://support.apple.com/kb/DL1070 I don't expect that documentation to help :-)

>> I also updated my nightly in addition to applying the Safari fix, so I suppose >> something in last night's nightly could have done it. > > That's possible, but it's a real long shot. Please go back to testing with whatever nightly you were previously testing with, to see if that makes a difference.

For what it's worth, here's the tryserver build made from my patch from comment #3: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-452fb98f7069/try-macosx64/firefox-11.0a1.en-US.mac.dmg

Oh, I got the crash just now on nightly. Inspired by bug 707814, I have some steps to reproduce: 1. Primary monitor is set to my external monitor (File etc menu is on that), Firefox window is on my laptop monitor, which is the secondary display. 2. Unplug the external monitor, wait for display to settle out. 3. Right click on top of browser tab. Crash! Main browser window has to have focus when you unplug maybe? Left clicking to get focus first prevented it from crashing, but then plugging and unplugging my external monitor a few times, then right clicking, caused it to crash. I guess the crash stopped because I hadn't unplugged my monitor in a while.

This is the OSX crash log produced when I right clicked on a tab after unplugging the monitor. Just so you can look and see if it seems like the same issue. I can try your patched version tomorrow.

Came in this morning, plugged in my external monitor, right clicked a few times on the title bar above the tabs, no problem. Right clicked once on the top part of a tab, instant crash.

What are your results with my tryserver build?

Using the try build in comment 11, I am not able to reproduce the crash, even after unplugging and plugging in the external monitor a number of times.

Good to hear it! If you don't mind, please keep testing for a while with my tryserver build. I very much doubt you'll see any problems (caused by my patch), but I'd like to hear about them if you do. If there are problems, they'd most likely have to do with window movement not being tracked properly. I'll also test the tryserver build. But I currently don't have an external monitor, so I'm unlikely to be able to reproduce this bug.

Comment on attachment 579735 [details] [diff] [review] Possible fix (prevent recursion in ReportMoveEvent()) I've now lived with my tryserver build for a day, and didn't see any problems.

I was getting this crash all the time. I tried Andrew's STRs and they totally make me crash every time. I'm now using your try server build, and I can't reproduce the crash any more.

My tryserver build from comment #11 has disappeared, even though it was only created five days ago! Here's another: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-011eb6a9ee19/try-macosx64/firefox-11.0a1.en-US.mac.dmg

Builds are temporarily not being saved for as long (4 days or something?) due to some disk problems, or something along those lines.

Comment on attachment 579735 [details] [diff] [review] Possible fix (prevent recursion in ReportMoveEvent()) Looks like Markus is unavailable, or very busy. Could you review this, Benoit?

Comment on attachment 579735 [details] [diff] [review] Possible fix (prevent recursion in ReportMoveEvent()) Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/b0115f8271e1

Sorry for the delay; I'm fine with the fix as an additional safety net, but I think we should also fix the bug that makes this necessary. Neil, what is the expectation about the widget's behavior here? Is it supposed to fire move events when moved via an explicit call to nsIWidget::Move? If so, we really need to fix the nsMenuPopupFrame side of things so that it doesn't keep adding the context menu offset. Note that the Linux widget backend has run into the same problem: http://hg.mozilla.org/mozilla-central/annotate/fd6ab19f312c/widget/src/gtk2/nsWindow.cpp#l2448 They worked around it by just not firing NS_MOVE events for context menus at all.

As far as I can tell, nsMenuPopupFrame::SetPopupPosition starts with mScreenX/YPos (if they're set) and calculates the widget position from that, and nsMenuPopupFrame::MoveTo tries to do the inverse calculation in order to update mScreenX/YPos and gets it wrong. In fact, nsMenuPopupFrame::SetPopupPosition uses so many inputs into its calculation that I don't know if accounting for the context menu offset was all that's needed to fix the reverse calculation. Probably not, but I think it will fix this bug. Perhaps nsMenuPopupFrame::MoveTo should only fix mScreenX/YPos for popups which it expects to be movable? Then for example anchored menus would stay anchored (and not screen-pinned) even if NS_MOVE come from their widgets.

> Is it supposed to fire move events when moved via an explicit call to nsIWidget::Move? It can do, but checks in nsXULPopupManager::PopupMoved should be preventing anything from happening if the coordinates are the same. Comment 12 suggests that this bug is a regression from 668437. Likely, the client offset computation is causing an issue and is probably the same problem as 707814. > Perhaps nsMenuPopupFrame::MoveTo should only fix mScreenX/YPos for popups > which it expects to be movable? Then for example anchored menus would stay > anchored (and not screen-pinned) even if NS_MOVE come from their widgets. All popups can be moved. Calling nsMenuPopupFrame::MoveTo makes the popup screen positioned and removes its anchoring. Normal context menus are always opened screen positioned.

(In reply to Neil Deakin from comment #26) > > Is it supposed to fire move events when moved via an explicit call to nsIWidget::Move? > > It can do, but checks in nsXULPopupManager::PopupMoved should be preventing > anything from happening if the coordinates are the same. Comment 12 suggests > that this bug is a regression from 668437. Likely, the client offset > computation is causing an issue and is probably the same problem as 707814. Oh, I see. Makes sense. I was also seeing this bug when I worked on async widget geometry in bug 598482, but that was on top of bug 668437, too, so it might well be the cause. > > Perhaps nsMenuPopupFrame::MoveTo should only fix mScreenX/YPos for popups > > which it expects to be movable? Then for example anchored menus would stay > > anchored (and not screen-pinned) even if NS_MOVE come from their widgets. > > All popups can be moved. Calling nsMenuPopupFrame::MoveTo makes the popup > screen positioned and removes its anchoring. Normal context menus are always > opened screen positioned. Okay, thanks.

Comment on attachment 581617 [details] [diff] [review] take context menu offset into account when calculating mScreenX/YPos from widget position This isn't necessary, then, and the real bug is being fixed in bug 707814.