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

Crash in -[NSAlert buttonPressed:] #2297

Open
dmoagx opened this Issue Oct 19, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@dmoagx
Member

dmoagx commented Oct 19, 2015

Another popular issue.

objc_msgSend() selector name: retain
Performing @selector(buttonPressed:) from sender NSButton 0x10d8d3c10

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                 0x0000000103182710 objc_msgSend_vtable13 + 16
1   com.apple.AppKit                0x00000001015e3678 -[NSAlert didEndAlert:returnCode:contextInfo:] + 47
2   com.apple.AppKit                0x00000001012a3ac6 -[NSApplication endSheet:returnCode:] + 267
3   com.apple.AppKit                0x00000001015e39a6 -[NSAlert buttonPressed:] + 265
4   com.apple.AppKit                0x0000000101496959 -[NSApplication sendAction:to:from:] + 342
5   com.apple.AppKit                0x00000001014967b7 -[NSControl sendAction:to:] + 85
6   com.apple.AppKit                0x00000001014966eb -[NSCell _sendActionFrom:] + 138
7   com.apple.AppKit                0x0000000101494bd3 -[NSCell trackMouse:inRect:ofView:untilMouseUp:] + 1855
8   com.apple.AppKit                0x0000000101494421 -[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:] + 504
9   com.apple.AppKit                0x0000000101493b9c -[NSControl mouseDown:] + 820
10  com.apple.AppKit                0x000000010148b50e -[NSWindow sendEvent:] + 6853
11  com.apple.AppKit                0x0000000101487644 -[NSApplication sendEvent:] + 5761
12  com.apple.AppKit                0x000000010139d21a -[NSApplication run] + 636
13  com.apple.AppKit                0x0000000101341bd6 NSApplicationMain + 869
14  com.sequelpro.SequelPro         0x0000000100001994 start + 52

(or similar)

Reports:
http://log.sequelpro.com/view/4978
http://log.sequelpro.com/view/3724
(and others)

@dmoagx

This comment has been minimized.

Show comment
Hide comment
@dmoagx

dmoagx Oct 19, 2015

Member

The debug code added in f38ffcb shows this as one culprit:

-[NSAlert(SPAlertDebug) sp_buttonPressed:] of <NSAlert: 0x11a38fc90> title=
Error
text=
The table data couldn't be loaded.

MySQL said: MySQL server has gone away

This is in -[SPTableContent loadTableValues].
Stepping through the code suggests the retain comes from Cocoa trying to retain the modalDelegate (which is SPTableContent *) to call the selector.
So, yes, the modalDelegate is not retained during the general lifetime of the alert, and SPTableContent could very likely be released at this stage.

There seems to be exactly one mention of this in Apples doc:

Unlike other delegates in Cocoa, modal delegates in sheets are temporary and the relationship lasts only until the sheet is dismissed. The sheet’s modal delegate is not retained by any document-modal method or function.

https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Sheets/Concepts/AboutSheets.html

Buuuut, in this case we didn't supply a callback selector anyway, so passing a modalDelegate was completely unnecessary in the first place since it would have never been called.

Member

dmoagx commented Oct 19, 2015

The debug code added in f38ffcb shows this as one culprit:

-[NSAlert(SPAlertDebug) sp_buttonPressed:] of <NSAlert: 0x11a38fc90> title=
Error
text=
The table data couldn't be loaded.

MySQL said: MySQL server has gone away

This is in -[SPTableContent loadTableValues].
Stepping through the code suggests the retain comes from Cocoa trying to retain the modalDelegate (which is SPTableContent *) to call the selector.
So, yes, the modalDelegate is not retained during the general lifetime of the alert, and SPTableContent could very likely be released at this stage.

There seems to be exactly one mention of this in Apples doc:

Unlike other delegates in Cocoa, modal delegates in sheets are temporary and the relationship lasts only until the sheet is dismissed. The sheet’s modal delegate is not retained by any document-modal method or function.

https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Sheets/Concepts/AboutSheets.html

Buuuut, in this case we didn't supply a callback selector anyway, so passing a modalDelegate was completely unnecessary in the first place since it would have never been called.

@dmoagx

This comment has been minimized.

Show comment
Hide comment
@dmoagx

dmoagx Feb 25, 2016

Member

This issue still appears in 1.1.1 stable.

Member

dmoagx commented Feb 25, 2016

This issue still appears in 1.1.1 stable.

@abhibeckert

This comment has been minimized.

Show comment
Hide comment
@abhibeckert

abhibeckert Feb 25, 2016

Member

The method we're using to present the alert was deprecated in 10.10, perhaps because of bugs like this. Unfortunately the replacement requires 10.9.

But modal alerts suck anyway. What do you think of switching to a non-modal NSPanel instead?

Member

abhibeckert commented Feb 25, 2016

The method we're using to present the alert was deprecated in 10.10, perhaps because of bugs like this. Unfortunately the replacement requires 10.9.

But modal alerts suck anyway. What do you think of switching to a non-modal NSPanel instead?

@abhibeckert

This comment has been minimized.

Show comment
Hide comment
@abhibeckert

abhibeckert Feb 25, 2016

Member

I may have been able to reproduce this crash by stopping the server in the middle of editing a row.

After trying to save the edit, I get the "Connection Lost" sheet, with "close connection" or "reconnect" options.

Reconnect doesn't work, since the server is gone.

Close connection closes the Window and presents "Unable to write row .. server has gone away" with "discard changes" or "edit row" buttons. Clicking either will crash.

Member

abhibeckert commented Feb 25, 2016

I may have been able to reproduce this crash by stopping the server in the middle of editing a row.

After trying to save the edit, I get the "Connection Lost" sheet, with "close connection" or "reconnect" options.

Reconnect doesn't work, since the server is gone.

Close connection closes the Window and presents "Unable to write row .. server has gone away" with "discard changes" or "edit row" buttons. Clicking either will crash.

@abhibeckert

This comment has been minimized.

Show comment
Hide comment
@abhibeckert

abhibeckert Feb 25, 2016

Member

In my opinion neither of those errors should be modal.

If the connection goes away, the behaviour should be more like the Slack app - you're notified that the connection is gone and it's trying to reconnect, but you can still access the UI normally:

screen shot 2016-02-26 at 07 48 16

Second, if changes to a row can't be saved, instead of asking the user if they want to continue editing or discard, it should perform the "continue editing" code path automatically and present a non-modal dialog offering to discard changes.

Member

abhibeckert commented Feb 25, 2016

In my opinion neither of those errors should be modal.

If the connection goes away, the behaviour should be more like the Slack app - you're notified that the connection is gone and it's trying to reconnect, but you can still access the UI normally:

screen shot 2016-02-26 at 07 48 16

Second, if changes to a row can't be saved, instead of asking the user if they want to continue editing or discard, it should perform the "continue editing" code path automatically and present a non-modal dialog offering to discard changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment