From a149dd7e2b0356a56cc8c68d0514e8f55b45e209 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Fri, 21 Apr 2017 09:16:11 -0700 Subject: [PATCH 1/3] Fixes a potential deadlock; it's not safe to message likely super nodes while holding the lock. --- Source/ASControlNode.mm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Source/ASControlNode.mm b/Source/ASControlNode.mm index 404567c83..0d1df5739 100644 --- a/Source/ASControlNode.mm +++ b/Source/ASControlNode.mm @@ -420,9 +420,10 @@ - (void)removeTarget:(id)target action:(SEL)action forControlEvents:(ASControlNo - (void)sendActionsForControlEvents:(ASControlNodeEvent)controlEvents withEvent:(UIEvent *)event { + ASDisplayNodeAssertMainThread(); //We access self.view below, it's not safe to call this off of main. NSParameterAssert(controlEvents != 0); - ASDN::MutexLocker l(_controlLock); + _controlLock.lock(); // Enumerate the events in the mask, invoking the target-action pairs for each. _ASEnumerateControlEventsIncludedInMaskWithBlock(controlEvents, ^ @@ -442,10 +443,13 @@ - (void)sendActionsForControlEvents:(ASControlNodeEvent)controlEvents withEvent: responder = [self.view targetForAction:action withSender:self]; } + //It's unsafe to arbitrarily call out to classes which could own this control node while holding the lock. + _controlLock.unlock(); #pragma clang diagnostic push #pragma clang diagnostic ignored "-Warc-performSelector-leaks" [responder performSelector:action withObject:self withObject:event]; #pragma clang diagnostic pop + _controlLock.lock(); } }); } From 1ca5462d88bad11966ad539e7d87bf657b53e7eb Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 24 Apr 2017 10:48:31 -0700 Subject: [PATCH 2/3] I think this is what @nguyenhuy is looking for :) --- Source/ASControlNode.mm | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/Source/ASControlNode.mm b/Source/ASControlNode.mm index 0d1df5739..542245057 100644 --- a/Source/ASControlNode.mm +++ b/Source/ASControlNode.mm @@ -423,35 +423,41 @@ - (void)sendActionsForControlEvents:(ASControlNodeEvent)controlEvents withEvent: ASDisplayNodeAssertMainThread(); //We access self.view below, it's not safe to call this off of main. NSParameterAssert(controlEvents != 0); + NSMutableArray *resolvedEventTargetActionArray = [[NSMutableArray alloc] init]; + _controlLock.lock(); // Enumerate the events in the mask, invoking the target-action pairs for each. _ASEnumerateControlEventsIncludedInMaskWithBlock(controlEvents, ^ (ASControlNodeEvent controlEvent) { - // Use a copy to itereate, the action perform could call remove causing a mutation crash. - NSArray *eventTargetActionArray = [_controlEventDispatchTable[_ASControlNodeEventKeyForControlEvent(controlEvent)] copy]; - // Iterate on each target action pair - for (ASControlTargetAction *targetAction in eventTargetActionArray) { - SEL action = targetAction.action; - id responder = targetAction.target; + for (ASControlTargetAction *targetAction in _controlEventDispatchTable[_ASControlNodeEventKeyForControlEvent(controlEvent)]) { + ASControlTargetAction *resolvedTargetAction = [[ASControlTargetAction alloc] init]; + resolvedTargetAction.action = targetAction.action; + resolvedTargetAction.target = targetAction.target; // NSNull means that a nil target was set, so start at self and travel the responder chain - if (!responder && targetAction.createdWithNoTarget) { + if (!resolvedTargetAction.target && targetAction.createdWithNoTarget) { // if the target cannot perform the action, travel the responder chain to try to find something that does - responder = [self.view targetForAction:action withSender:self]; + resolvedTargetAction.target = [self.view targetForAction:resolvedTargetAction.action withSender:self]; } - //It's unsafe to arbitrarily call out to classes which could own this control node while holding the lock. - _controlLock.unlock(); + if (resolvedTargetAction.target) { + [resolvedEventTargetActionArray addObject:resolvedTargetAction]; + } + } + }); + + _controlLock.unlock(); + + //We don't want to hold the lock while calling out, we could potentially walk up the ownership tree causing a deadlock. #pragma clang diagnostic push #pragma clang diagnostic ignored "-Warc-performSelector-leaks" - [responder performSelector:action withObject:self withObject:event]; + for (ASControlTargetAction *targetAction in resolvedEventTargetActionArray) { + [targetAction.target performSelector:targetAction.action withObject:self withObject:event]; + } #pragma clang diagnostic pop - _controlLock.lock(); - } - }); } #pragma mark - Convenience From 9fb18962dbbebe506c29944c039eede8e4718577 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 24 Apr 2017 10:49:56 -0700 Subject: [PATCH 3/3] Fix up CHANGELOG.md --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bd611b9f..b2fbc11a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,5 @@ ## master * Add your own contributions to the next release on the line below this with your name. -Philipp Smorygo Fix `__has_include` check in ASLog.h \ No newline at end of file +Fix `__has_include` check in ASLog.h [Philipp Smorygo](Philipp.Smorygo@jetbrains.com) +Fix potential deadlock in ASControlNode [Garrett Moon](https://github.com/garrettmoon) \ No newline at end of file