Regression on delegates calls

[RuiAAPeres]

It seems #706 broke the way the delegate calls were being made (table:didSelectRowAtIndexPath: is not being called anymore). I am not sure why, but the previous one should work as well, since the target is just being replaced. I will test with the DataSource methods later, as it is probably happening the same there.

I also had a look at the selectors that were hitting ASTableProxy's respondsToSelector and table:didSelectRowAtIndexPath: wasn't. I tested with ASTableView, but I am guessing the ASCollectionView will have the same behaviour.

[appleguy]

@RuiAAPeres Does this resolve the issue without breaking the nil-delegate use case that prompted the original change? Bug also reported here: #721

[RuiAAPeres]

It does. The cell sizes are respected.

[appleguy]

Interesting...the intermittent test failures may actually be related to this area!

Application Specific Information:
objc_msgSend() selector name: _accessibilityValueForKey:
CoreSimulator 110.4 - Device: iPhone 6 - Runtime: iOS 8.1 (12B411) - DeviceType: iPhone 6
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libobjc.A.dylib 0x000000010490900b objc_msgSend + 11
1 UIKit 0x00000001168adc9f -[UIScrollViewAccessibility _axCleanupDelegateClearer] + 89
2 UIKit 0x00000001168adbc6 -[UIScrollViewAccessibility clearDelegateClearer] + 30
3 UIKit 0x00000001168adece -[UIScrollViewAccessibility setDelegate:] + 36
4 UIKit 0x0000000104c12653 -[UITableView setDelegate:] + 122
5 AsyncDisplayKitTests 0x00000001101f8a20 -[ASTableView setAsyncDelegate:] + 688 (ASTableView.mm:317)
6 AsyncDisplayKitTests 0x000000010ffb4c2c -[ASTableViewTests testReloadData] + 508 (ASTableViewTests.m:170)

[RuiAAPeres]

@appleguy I was thinking about if instead of reseting these to nil:

   if (asyncDelegate == nil) {
     // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes
     // in UIScrollViewAccessibility.
    super.delegate = nil;
     _asyncDelegate = nil;
    _proxyDelegate = nil;
}

Maybe to:

_proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; 
super.delegate = (id<UITableViewDelegate>)_proxyDelegate;

[appleguy]

@RuiAAPeres could you give it a try and see if that allows the tests to pass? I think they are crashing every time with this diff, as I did re-run them.

[RuiAAPeres]

@appleguy tried multiple combinations with that code, but the tests always fail. A quick search for clearDelegateClearer revealed some people having that specific crash in iOS8 simulator only, but I don't think it's worth the trouble.

[RuiAAPeres]

@appleguy apparently my last commit did it! ✨

[appleguy]

@RuiAAPeres so I also reverted my original change, and that may have had an impact because I think your new submitted diff will rebase onto master. Can you pull in the other elements of my original change into this diff, that would enable the nil delegate to work? Namely the -init changes.

#726

[levi]

@appleguy, @RuiAAPeres any word on the status of this fix? Ran into some issues in my app internally that would benefit from having nil delegate supported.

[RuiAAPeres]

@levi from my tests, it works when you got an initial delegate nilled and then you set a new one. What I saw was when you do something: nil, delegate, nil. Things weren't working.

[appleguy]

@levi just confirming, do those issues still exist in latest master? File a task with details if you get a chance.

[levi]

I'm seeing some strange nil delegate crashes in my production app, but I'm too far removed right now to be able to draw a relationship to this ticket. I just want to clarify, the support for nil delegates was merged and then rolled back, and now we're pending a fix to merge it back in?

On Oct 26, 2015, at 23:09, appleguy notifications@github.com wrote:

@levi just confirming, do those issues still exist in latest master? File a task with details if you get a chance.

—
Reply to this email directly or view it on GitHub.

[appleguy]

@levi I'm nearly positive it was merged in again after changes. @RuiAAPeres am I right? Do you remember for sure?

[appleguy]

@levi as such, posting full data about the crashes would probably be useful when you come across them again.

[levi]

I'll collect what we've gathered with ASDK master in our production app and send you some logs this week. Still assessing the source in regards to ASDK, but there's enough of them that I'm confident we'll be able to fix a bug or two in ASCollectionView and ASTableView.

[RuiAAPeres]

@appleguy from what I remember it was reverted. So I was using my fork.

[appleguy]

@RuiAAPeres I will figure out what happened exactly, but I'm nearly certain I intended to merge this PR and then close it — but somehow just closed it. ha.

[appleguy]

OK, I don't understand WTF is going on with this PR. In the files changed, it is just @nguyenhuy's awesome test additions for static layout spec. And it shows that the PR can be merged, even though this has already been landed.

@RuiAAPeres would you be able to re-submit your fixed version of the nil delegate handling patch? Naturally the discussion was about a mysterious part of the change that no one could explain the necessity of, but I'm willing to land that change and later investigate the details, as clearly it will improve the behavior for a few apps.

[RuiAAPeres]

Ok, let me close this, and re-submit.