Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deadlock while selecting an object #1583

Closed
tiennou opened this issue Aug 28, 2013 · 2 comments
Closed

Deadlock while selecting an object #1583

tiennou opened this issue Aug 28, 2013 · 2 comments

Comments

@tiennou
Copy link
Member

@tiennou tiennou commented Aug 28, 2013

I just the crash below while testing the ndhotkeyevent-module branch (which looks sufficiently unrelated that I care to report it). FYI, I'm just triggering the Global Selection trigger using ⌘⎋ with some text selected in Xcode.

So, relevant threads in the crash log are "Thread_677405" (A), "Thread_677550" (B) and "Thread_673763" (main thread, a.k.a 0). The deadlock arises because thread A executes -[QSInterfaceController showMainWindow:], which calls -[QSWindow makeKeyAndOrderFront:] without switching to the main thread. Meanwhile, thread 0 tries to do its drawing stuff, but gets suspended on a cond variable because thread A is touching stuff in the view hierarchy. After a bunch of calls, thread A tries to access the object's icon (which thanks to our craftfully written GCD code) trigger a background load on thread B, but sets the quick icon through a block scheduled on thread 0, which blocks. Thread B, after valiantly loading the requested icon, schedules the same block on thread 0. Beachballs (and hilarity, perhaps) ensues.

The fix's easy : add a runOnMainQueueSync in -[QSInterfaceController showMainWindow:] (I was thinking in -[QSObjectActions selectObjectInCommandWindow:] at first but we'd kill more birds with less stones if we do it later).

Anyways, I'm afraid it might not be the only place where this mistake is done (as we all know, getting threading right is hard). We should thrive to never, ever, access AppKit classes from another thread than the main one (except if the documentation says you can, but I know it loves to be ambiguous ;-)). Sorry if I'm pointing out the obvious, but I think it's the first time I look at a stacktrace and my brain just yells "AppKit => main thread !!" and I know there are a few random/weird/baffling crashes still left and those are a category by themselves.

There's the log, though I hope my version was funnier ;-). I snipped the binary images and some profile stuff because, well, who cares about that anyway : http://pastie.org/pastes/8278296/text

Sorry for depending on you guys to handle it, but I'm just totally overwhelmed in work and about to get some sleep, in like, 2 seconds. Cheers !

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 28, 2013

Sounds good Etienne.

Always worth remembering AppKit=main thread. I know I've committed a couple of changes in the past and forgot about this. I'm sure one of us (me or Rob! :D) will get round to adding this.
A good story on Mr A and Mr B. I don't quite follow it at the moment, but hey - it's early in the morning for me ;-)

Another idea: would setting allowsConcurrentViewDrawing to NO on the QS interface have an affect? (See NSWindow docs). Although the docs are pretty sparse so I have no idea what the advantages/disadvantages of multi-threaded drawing is.

Bonne nuit!

On 29 Awst 2013, at 06:48, Etienne Samson notifications@github.com wrote:

I just the crash below while testing the ndhotkeyevent-module branch (which looks sufficiently unrelated that I care to report it). FYI, I'm just triggering the Global Selection trigger using ⌘⎋ with some text selected in Xcode.

So, relevant threads in the crash log are "Thread_677405" (A), "Thread_677550" (B) and "Thread_673763" (main thread, a.k.a 0). The deadlock arises because thread A executes -[QSInterfaceController showMainWindow:], which calls -[QSWindow makeKeyAndOrderFront:] without switching to the main thread. Meanwhile, thread 0 tries to do its drawing stuff, but gets suspended on a cond variable because thread A is touching stuff in the view hierarchy. After a bunch of calls, thread A tries to access the object's icon (which thanks to our craftfully written GCD code) trigger a background load on thread B, but sets the quick icon through a block scheduled on thread 0, which blocks. Thread B, after valiantly loading the requested icon, schedules the same block on thread 0. Beachballs (and hilarity, perhaps) ensues.

The fix's easy : add a runOnMainQueueSync in -[QSInterfaceController showMainWindow:](I was thinking in -[QSObjectActions selectObjectInCommandWindow:] at first but we'd kill more birds with less stones if we do it later).

Anyways, I'm afraid it might not be the only place where this mistake is done (as we all know, getting threading right is hard). We should thrive to never, ever, access AppKit classes from another thread than the main one (except if the documentation says you can, but I know it loves to be ambiguous ;-)). Sorry if I'm pointing out the obvious, but I think it's the first time I look at a stacktrace and my brain just yells "AppKit => main thread !!" and I know there are a few random/weird/baffling crashes still left and those are a category by themselves.

There's the log, though I hope my version was funnier ;-). I snipped the binary images and some profile stuff because, well, who cares about that anyway : http://pastie.org/pastes/8278296/text

Sorry for depending on you guys to handle it, but I'm just totally overwhelmed in work and about to get some sleep, in like, 2 seconds. Cheers !


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 28, 2013

Sounds good Etienne.

Always worth remembering AppKit=main thread. I know I've committed a couple of changes in the past and forgot about this. I'm sure one of us (me or Rob! :D) will get round to adding this.
A good story on Mr A and Mr B. I don't quite follow it at the moment, but hey - it's early in the morning for me ;-)

Another idea: would setting allowsConcurrentViewDrawing to NO on the QS interface have an affect? (See NSWindow docs). Although the docs are pretty sparse so I have no idea what the advantages/disadvantages of multi-threaded drawing is.

Bonne nuit!

P.S. search for 'application kit thread safety' on https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Multithreading/CreatingThreads/CreatingThreads.html#//apple_ref/doc/uid/10000057i-CH15-SW2
some good/conclusive info :)

On 29 Awst 2013, at 06:48, Etienne Samson notifications@github.com wrote:

I just the crash below while testing the ndhotkeyevent-module branch (which looks sufficiently unrelated that I care to report it). FYI, I'm just triggering the Global Selection trigger using ⌘⎋ with some text selected in Xcode.

So, relevant threads in the crash log are "Thread_677405" (A), "Thread_677550" (B) and "Thread_673763" (main thread, a.k.a 0). The deadlock arises because thread A executes -[QSInterfaceController showMainWindow:], which calls -[QSWindow makeKeyAndOrderFront:] without switching to the main thread. Meanwhile, thread 0 tries to do its drawing stuff, but gets suspended on a cond variable because thread A is touching stuff in the view hierarchy. After a bunch of calls, thread A tries to access the object's icon (which thanks to our craftfully written GCD code) trigger a background load on thread B, but sets the quick icon through a block scheduled on thread 0, which blocks. Thread B, after valiantly loading the requested icon, schedules the same block on thread 0. Beachballs (and hilarity, perhaps) ensues.

The fix's easy : add a runOnMainQueueSync in -[QSInterfaceController showMainWindow:](I was thinking in -[QSObjectActions selectObjectInCommandWindow:] at first but we'd kill more birds with less stones if we do it later).

Anyways, I'm afraid it might not be the only place where this mistake is done (as we all know, getting threading right is hard). We should thrive to never, ever, access AppKit classes from another thread than the main one (except if the documentation says you can, but I know it loves to be ambiguous ;-)). Sorry if I'm pointing out the obvious, but I think it's the first time I look at a stacktrace and my brain just yells "AppKit => main thread !!" and I know there are a few random/weird/baffling crashes still left and those are a category by themselves.

There's the log, though I hope my version was funnier ;-). I snipped the binary images and some profile stuff because, well, who cares about that anyway : http://pastie.org/pastes/8278296/text

Sorry for depending on you guys to handle it, but I'm just totally overwhelmed in work and about to get some sleep, in like, 2 seconds. Cheers !


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants