520044 - Crash [@ ChildViewMouseTracker::MouseMoved]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x51000057
0x922c268c in objc_msgSend ()
(gdb) bt
#0  0x922c268c in objc_msgSend ()
#1  0x1132d13a in ChildViewMouseTracker::MouseMoved (aEvent=0x280f0df0) at /Users/smaug/mozilla/hg/mozilla/widget/src/cocoa/nsChildView.mm:6584
#2  0x9249d731 in -[NSWindow sendEvent:] ()
#3  0x1131fe78 in -[ToolbarWindow sendEvent:] (self=0x14376d60, _cmd=0x922c84b8, anEvent=0x280f0df0) at /Users/smaug/mozilla/hg/mozilla/widget/src/cocoa/nsCocoaWindow.mm:1938
#4  0x92469d91 in -[NSApplication sendEvent:] ()
#5  0x923c6fe7 in -[NSApplication run] ()
#6  0x1131cbea in nsAppShell::Run (this=0x536ba0) at /Users/smaug/mozilla/hg/mozilla/widget/src/cocoa/nsAppShell.mm:766
#7  0x11e9c7d7 in nsAppStartup::Run (this=0xbfffe758) at /Users/smaug/mozilla/hg/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:182
#8  0x00048e0a in XRE_main (argc=3, argv=0xbfffede0, aAppData=0x50e520) at /Users/smaug/mozilla/hg/mozilla/toolkit/xre/nsAppRunner.cpp:3418
#9  0x00001d97 in main (argc=3, argv=0x804600) at /Users/smaug/mozilla/hg/mozilla/browser/app/nsBrowserApp.cpp:15

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 404098 [details] [diff] [review]
fix v1.0

I don't think this crash is caused by a null-pointer. Sending messages to nil is defined and supported in Objective C: it's a no-op and returns a 0-like value.
Moreover, this patch would cause prevent us from sending exit events when the mouse leaves the window.

Olli, can you reproduce this crash?

I think what's more likely to have caused this crash is a bad sLastMouseEventView pointer that wasn't cleared when the object was freed. Is it possible that widgetDestroyed sometimes isn't called before dealloc?

Comment 3

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

(In reply to comment #2)
> Olli, can you reproduce this crash?
No I can't and I unfortunately quit gdb already.

But I'm running FF in gdb all the time, so I'll report if this happens again.

(I wish I knew how to create a core dump in OSX gdb)

Comment 4

[Markus Stange [:mstange]]

Attachment: v2

This moves the clearing back to where we cleared sLastViewEntered before bug 515003. It might also fix bug 520871.

Comment 5

[Josh Aas]

Markus - please explain how you expect this patch to fix crashes by writing out the course of events that leads to a crash without this patch. I only glanced at the patch but it seems like you're setting the pointer to nil later (iirc dealloc should happen after widgetDestroyed), and if it was already a bad value then why would clearing out the bad value later help?

If widgetDestroyed is getting called after dealloc then I think we have a different bug and this patch will just cover that up.

Comment 6

[Markus Stange [:mstange]]

Attachment: v3

This is just a guess, but I think the following event flow is possible:

- There's a valid pointer to a view stored in sLastMouseEventView.
- Gecko destroys this view by calling nsChildView::Destroy
   - This calls -[ChildView widgetDestroyed], which calls ChildViewMouseTracker
     ::OnDestroyView, which resets sLastMouseEventView to nil.
   - In nsChildView::TearDownView, we queue a delayedTearDown.
- We process a mouse move event over the view and sLastMouseEventView is set
  to the destroyed view again.
- delayedTearDown is called, which releases the view and makes
  sLastMouseEventView bad.

Of course this requires a mouse move event to be already waiting in the event queue before we do [mView performSelectorOnMainThread:@selector(delayedTearDown) withObject:nil waitUntilDone:false].

And I think I've found another, though very unlikely, way for sLastMouseEventView to get bad, which is why I've uploaded a new patch:

- sLastMouseEventView is view A.
- The user moves the mouse from view A to view B.
- A mouse exit event is sent to view A.
   - During the processing of this event, view B is destroyed.
     sLastMouseEventView still points to view A, so nothing happens there.
- sLastMouseEventView is set to view B.
- A mouse move event is set to view B, which is being destroyed, but not
  deallocated yet.
- view B is deallocated, sLastMouseEventView becomes bad.

Comment 7

[Josh Aas]

Comment on attachment 405638 [details] [diff] [review]
v3

We should take this for now but it would be nice to come up with a more robust and transparent scheme for handling destruction and globals.

Comment 8

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/f9ea7e1fce8d