577567 - Cm 2.0.3 crashes [@ objc_msgSend | -[NSWindow makeKeyWindow] ] with full-screen Flash and Exposé or window cycling; Take 2: Flash 10.1!

Status: RESOLVED FIXED

(Camino Graveyard :: Plug-ins, defect)

Description

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

Starting mid-cycle for 2.0.3, we're seeing new [@ objc_msgSend | -[NSWindow makeKeyWindow] ] crashes.  They're currently the #2 2.0.3 crash.

The signatures are the same or very similar to what we saw in bug 465178, except all (but one) of these crashes are with 10.1.something.

0  	libobjc.A.dylib  	objc_msgSend  	
1 	AppKit 	-[NSWindow makeKeyWindow] 	
2 	AppKit 	-[NSWindow makeKeyAndOrderFront:] 	
3 	Camino 	-[BrowserWindow makeKeyAndOrderFront:] 	mozilla/camino/src/browser/BrowserWindow.mm:66
4 	AppKit 	_prepareToRestore 	
5 	AppKit 	_NSRestoreFromDock 	
6 	AppKit 	_NSCoreDockMessageReceive 	

0  	libobjc.A.dylib  	objc_msgSend  	
1 	AppKit 	-[NSWindow makeKeyWindow] 	
2 	AppKit 	-[NSWindow _makeKeyRegardlessOfVisibility] 	
3 	AppKit 	-[NSWindow makeKeyAndOrderFront:] 	
4 	Camino 	-[BrowserWindow makeKeyAndOrderFront:] 	mozilla/camino/src/browser/BrowserWindow.mm:66
5 	AppKit 	_prepareToRestore 	
6 	AppKit 	_NSRestoreFromDock 	
7 	AppKit 	_NSCoreDockMessageReceive 	

There's also a crash that looks like

0  	libobjc.A.dylib  	objc_msgSend  	
1 	AppKit 	-[NSWindow makeKeyWindow] 	
2 	AppKit 	-[NSWindow _makeKeyRegardlessOfVisibility] 	
3 	AppKit 	-[NSWindow makeKeyAndOrderFront:] 	
4 	Camino 	-[BrowserWindow makeKeyAndOrderFront:] 	mozilla/camino/src/browser/BrowserWindow.mm:66
5 	AppKit 	-[NSApplication _cycleWindowsReversed:] 	
6 	AppKit 	_NSKeyboardUIHandleSymbolicHotKey 	
7 	AppKit 	-[NSApplication _handleKeyEquivalent:] 	
8 	AppKit 	-[NSApplication sendEvent:] 	
9 	AppKit 	-[NSApplication run] 	
10 	AppKit 	NSApplicationMain 	

that's slightly different, but it's using Cmd-` to cycle windows rather than Exposé.

There are also some comments mentioning sleeping the Mac or sleeping the display.

The vast majority of the crashes are with 10.1 "final" (aka rc7) or 10.1rc2.  There are also a few with Gala Preview 2, and one each with Gala Preview 1, 10.1rc4, and some version that no-one has ever seen before (module id: A761C9FCC2551568696107F1811B8F010).  All are Intel, mostly 10.6 or 10.5, but 2 10.4, too.

I can easily repro either Exposé Show All Windows + click on origin window or Cmd-` with 2.0.3, but not at all with 2.1a1pre (I suspect Core focus changes changed something yet again?) using the URL in the location field.

STR:

1) Load http://www.fastcashphoto.com/2010/06/final-cut-pro-7-unscripted-using-alpha-transitions/

2) Make the video full-screen

3a) Exposé "Show All Windows" and click on the original window, OR
3b) Cmd-` to cycle windows.

4) BOOM!

Comment 1

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

To summarize from irc:  

With Flash 10.1, it's now a FP_FPWindow that's being deallocated too early, per zombies (-[FP_FPWindow _setFrameNeedsDisplay:]: message sent to deallocated instance 0x12a7ff10), and also checking for that window class in the work-around for bug 465178 seems to stop the crashing.  Stuart says the real fix would be a bit more complex, though.

Comment 2

[Stuart Morgan]

The "slightly more complex" part is that I don't think we want to pass nil into isMemberOfClass, so we need to do the string-to-class lookup and check the return before using it.

Comment 3

[Stuart Morgan]

Attachment: fix

I don't have a 2.0 tree post-hard-drive-death; can you take this for a spin to make sure I didn't botch it?

Comment 4

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

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

f+; I can Exposé and cycle windows with impunity.  Sometime please explain to me why we don't need the more complex method for NSCarbonWindow, too, though.

Comment 5

[Stuart Morgan]

NSCarbonWindow is a core system class, so it'll always be there. (I guess for future-proofing we could do the same check in case it changes in 10.x, since it's not public, but we'd have plenty of time to find that out). This string is only going to resolve to a class if a version of Flash with that class in it has been loaded, which is a whole lot less dependable.

Comment 6

[Mike Pinkerton (not reading bugmail)]

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

suck. sr=pink.

Comment 7

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

Stuart, I assume we want this on 1.9.2 even though the crash appears not to happen there, just in case the focus code changes under us?  It won't hurt any more than the code to stop the 10.0 crashes does, which appears to be "not at all", right?

Landed on CAMINO_2_0_BRANCH for Stuart in advance of 2.0.4; leaving open pending an answer to the previous question, though.

Comment 8

[Stuart Morgan]

It won't hurt, but since it's an ugly hack I'd rather we not carry it forward until/unless we have evidence that it's actually necessary.

Comment 9

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

OK, then FIXED it is!

I guess that also means we should test Flash 10.0 and see if we can remove that part of the hack from 2.1?

Comment 10

[Stuart Morgan]

Couldn't hurt (although I'd be happy just pulling the code regardless; nobody running 2.1 should still be running 10.0)

Comment 11

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

(In reply to comment #10)
> Couldn't hurt (although I'd be happy just pulling the code regardless; nobody
> running 2.1 should still be running 10.0)

Since we left this thread hanging, the removal happened in bug 583187.

-------------------------

There are also two reports of this crash with 2.0.5 and Flash 10.2:
bp-dc24eb30-f0b6-4709-8b4c-594a32101113 (Preview 1 and 10.5.8)
bp-76cac4c3-9a4c-4503-8185-2343b2101109 (Preview 2 and 10.6.4)

I can't reproduce the crash with 10.2 using the normal steps, so either there's a new STR or a new edge case, or some new condition to trigger that I don't meet.  We should keep an eye on it, but hopefully we can get 2.1 out before 10.2 ;)