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.

Sign up for GitHub

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

Jump to bottom

Fixed crash on binding to NSTableView #442

Closed
wants to merge 1 commit into from
Closed

Fixed crash on binding to NSTableView #442

wants to merge 1 commit into from

Conversation

nayzak
Copy link

@nayzak nayzak commented Jul 28, 2017 

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x775400656e68)
    frame #0: 0x00007fffca525f31 libobjc.A.dylib`objc_retain + 33
  * frame #1: 0x00000001000b5c4e Bond`__42-[BNDProtocolProxyBase forwardInvocation:]_block_invoke.21((null)=<unavailable>, buffer=0x00007fff5fbfdd40) at BNDProtocolProxyBase.m:79
    frame #2: 0x0000000100130940 Bond`thunk at ProtocolProxy.swift:0
    frame #3: 0x00000001001303d7 Bond`thunk at ProtocolProxy.swift:0
    frame #4: 0x000000010013dc01 Bond`partial apply for thunk at ProtocolProxy.swift:0
    frame #5: 0x0000000100130560 Bond`thunk at ProtocolProxy.swift:0
    frame #6: 0x0000000100132783 Bond`ProtocolProxy.(extractor=0x000000010013dc50 Bond`partial apply forwarder for reabstraction thunk helper from @callee_owned (@in (Swift.Int, Swift.Optional<Swift.UnsafeMutableRawPointer>)) -> (@out ()) to @callee_owned (@unowned Swift.Int, @unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () at ProtocolProxy.swift, setReturnValue=0x000000010013dce0 Bond`partial apply forwarder for reabstraction thunk helper from @callee_owned (@in Swift.Optional<Swift.UnsafeMutableRawPointer>) -> (@out ()) to @callee_owned (@unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () at ProtocolProxy.swift, block=0x000000010013d550 Bond`partial apply forwarder for Bond.ProtocolProxy.(signal <A, B, C, D, E> (for : ObjectiveC.Selector, dispatch : (ReactiveKit.PublishSubject<D, ReactiveKit.NoError>, A, B, C) -> E) -> ReactiveKit.Signal<D, ReactiveKit.NoError>).(closure #1).(closure #1) at ProtocolProxy.swift) -> D) -> Disposable).(closure #1) at ProtocolProxy.swift:113
    frame #7: 0x000000010013c2e8 Bond`partial apply for ProtocolProxy.(registerInvoker<A, B, C, D> (for : Selector, block : (A, B, C) -> D) -> Disposable).(closure #1) at ProtocolProxy.swift:0
    frame #8: 0x00000001001304fa Bond`thunk at ProtocolProxy.swift:0
    frame #9: 0x000000010013c421 Bond`partial apply for thunk at ProtocolProxy.swift:0
    frame #10: 0x0000000100130385 Bond`thunk at ProtocolProxy.swift:0
    frame #11: 0x0000000100130793 Bond`ProtocolProxy.invoke(selector="tableView:objectValueForTableColumn:row:", argumentExtractor=0x00000001001378c0 Bond`partial apply forwarder for reabstraction thunk helper from @callee_unowned @convention(block) (@unowned Swift.Int, @unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () to @callee_owned (@unowned Swift.Int, @unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () at ProtocolProxy.swift, setReturnValue=0x0000000100137950 Bond`partial apply forwarder for reabstraction thunk helper from @callee_unowned @convention(block) (@unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () to @callee_owned (@unowned Swift.Optional<Swift.UnsafeMutableRawPointer>) -> () at ProtocolProxy.swift, self=0x0000600000074480) -> (), setReturnValue : (UnsafeMutableRawPointer?) -> ()?) -> () at ProtocolProxy.swift:71
    frame #12: 0x00000001001308c8 Bond`@objc ProtocolProxy.invoke(with : Selector, argumentExtractor : (Int, UnsafeMutableRawPointer?) -> (), setReturnValue : (UnsafeMutableRawPointer?) -> ()?) -> () at ProtocolProxy.swift:0
    frame #13: 0x00000001000b595c Bond`-[BNDProtocolProxyBase forwardInvocation:](self=0x0000600000074480, _cmd="forwardInvocation:", invocation=0x000060000006c1c0) at BNDProtocolProxyBase.m:76
    frame #14: 0x00007fffb569654a CoreFoundation`___forwarding___ + 538
    frame #15: 0x00007fffb56962a8 CoreFoundation`__forwarding_prep_0___ + 120
    frame #16: 0x00007fffb32a80bb AppKit`-[NSTableRowData _addViewToRowView:atColumn:row:] + 536
    frame #17: 0x00007fffb32a7d1b AppKit`-[NSTableRowData _addViewsToRowView:atRow:] + 204
    frame #18: 0x00007fffb32a64bf AppKit`-[NSTableRowData _initializeRowView:atRow:] + 390
    frame #19: 0x00007fffb32a4ac5 AppKit`-[NSTableRowData _addRowViewForVisibleRow:withPriorView:] + 398
    frame #20: 0x00007fffb32a4880 AppKit`-[NSTableRowData _addRowViewForVisibleRow:withPriorRowIndex:inDictionary:withRowAnimation:] + 316
    frame #21: 0x00007fffb32a35b3 AppKit`-[NSTableRowData _unsafeUpdateVisibleRowEntries] + 1647
    frame #22: 0x00007fffb32a2ea6 AppKit`-[NSTableRowData updateVisibleRowViews] + 232
    frame #23: 0x00007fffb32a27ea AppKit`-[NSTableView layout] + 178
    frame #24: 0x00007fffb3a12246 AppKit`_NSViewLayout + 450
    frame #25: 0x00007fffb3243513 AppKit`-[NSView _doLayout] + 126
    frame #26: 0x00007fffb3243098 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 497
    frame #27: 0x00007fffb3243366 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 1215
    frame #28: 0x00007fffb3243366 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 1215
    frame #29: 0x00007fffb3243366 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 1215
    frame #30: 0x00007fffb3243366 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 1215
    frame #31: 0x00007fffb3243366 AppKit`-[NSView _layoutSubtreeWithOldSize:] + 1215
    frame #32: 0x00007fffb3a1895b AppKit`-[NSView _layoutSubtreeIfNeededAndAllowTemporaryEngine:] + 1516
    frame #33: 0x00007fffb3261869 AppKit`-[NSWindow(NSConstraintBasedLayout) _layoutViewTree] + 163
    frame #34: 0x00007fffb32d0c2e AppKit`-[NSWindow(NSConstraintBasedLayout) layoutIfNeeded] + 269
    frame #35: 0x00007fffb32d0b08 AppKit`-[NSWindow _setUpFirstResponderBeforeBecomingVisible] + 63
    frame #36: 0x00007fffb32cff3d AppKit`-[NSWindow _doWindowWillBeVisibleAsSheet:] + 169
    frame #37: 0x00007fffb3a3663e AppKit`-[NSWindow _reallyDoOrderWindowAboveOrBelow:relativeTo:findKey:forCounter:force:isModal:] + 1514
    frame #38: 0x00007fffb32ce6ce AppKit`-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 1041
    frame #39: 0x00007fffb32ce263 AppKit`-[NSWindow orderWindow:relativeTo:] + 153
    frame #40: 0x00007fffb3380bab AppKit`-[NSWindow makeKeyAndOrderFront:] + 111
    frame #41: 0x00007fffbb156519 QuickLookUI`-[QLSeamlessDocumentOpener showWindow:contentFrame:withBlock:] + 108
    frame #42: 0x00007fffb337fd37 AppKit`-[NSWindowController showWindow:] + 689
    frame #43: 0x00007fffb3152d2e AppKit`NSApplicationMain + 1013
    frame #44: 0x0000000100002b8d BondFix`main at AppDelegate.swift:12
    frame #45: 0x00007fffcae1a235 libdyld.dylib`start + 1

Demo project to reproduce the bug https://github.com/nayzak/BondBugDemo.

@codecov-io
Copy link

Codecov Report

Merging #442 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #442   +/-   ##
=======================================
  Coverage   60.46%   60.46%           
=======================================
  Files          52       52           
  Lines        4619     4619           
=======================================
  Hits         2793     2793           
  Misses       1826     1826

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06cc0b2...d2feda2. Read the comment docs.

srdanrasic
srdanrasic reviewed Sep 22, 2017
View reviewed changes
Sources/Bond/AppKit/NSTableView.swift
@@ -196,7 +196,7 @@ public extension SignalProtocol where
property: dataSource,
to: #selector(NSTableViewDataSource.tableView(_:objectValueFor:row:)),
map: { (dataSource: DataSource?, _: NSTableView, _: NSTableColumn, row: Int) -> Any? in
return dataSource?[row]
return dataSource?[row] as AnyObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonyarnold would you know how this affects things?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not sure what the intent here is. The closure still returns Any, and it's not a conditional downcast… I don't think it's necessary?

@nayzak do you recall why you needed this?

Copy link
Author

@nayzak nayzak Sep 23, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @srdanrasic, @tonyarnold! Did you look at my demo (https://github.com/nayzak/BondBugDemo) that reproduces the bug? I've just updated it to Swift 4 and latest Bond release an it still crashes without casting to AnyObject.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a look now - wow, that's… special.

I'm honestly not sure where this issue is coming from - I'm not saying we shouldn't fix it, but I'd like to know what knock-on effects we're going to see from casting absolutely everything to AnyObject (which is essentially saying "this is a reference type now", right?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was surprising for me too :) But obvious thing is that something goes wrong on a "glue level between Swift and ObjC". I've just made an assumption that objc definitely needs object, however Apple made Any same thing as id. Anyway I'm not that guy who knows what's actually going on in this case.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonyarnold Can you post a link to your PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DivineDominion I'm not sure it was me who ended up fixing this. I think @srdanrasic ended up rewriting some of the protocol proxy to address similar issues to this one. What specifically did you want to see?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hunting down a similar crash in another situation (and without Bond), so I am curious what you did to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DivineDominion This issue was very specific to Bond and how it implements protocol proxies by intercepting NSInvocations. The issue was related to Swift -> ObjC type bridging. Here is a first commit from the fix: 64f8e95

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton!

@srdanrasic
Copy link
Contributor

Thanks @nayzak for bringing this up!

@srdanrasic srdanrasic closed this Sep 30, 2017
@nayzak
Copy link
Author

nayzak commented Sep 30, 2017

@srdanrasic, @tonyarnold, thank you guys for spending your time for this helpful project 👍

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

@tonyarnold tonyarnold tonyarnold left review comments

@DivineDominion DivineDominion DivineDominion left review comments

@srdanrasic srdanrasic srdanrasic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nayzak @codecov-io @srdanrasic @tonyarnold @DivineDominion