From f9e06ac19208dd7c02c1a9144df6218453196132 Mon Sep 17 00:00:00 2001 From: Andrew Overton Date: Fri, 24 Apr 2020 09:54:34 -0400 Subject: [PATCH] Add call to -invalidateAccessibleElements to -dealloc. (#725) This is an example of a stack trace from a crash in a subclass of `NIAttributedLabel` that is used in a few internal apps: Screen Shot 2020-04-22 at 7 07 20 PM `NIAttributedLabel` deallocation appears to initiate an un-registering of the label's accessibility elements, which are instances of a `UIAccessibilityElement` subclass called `NIViewAccessibilityElement`. This un-registering involves calls to the method `-setAccessibilityContainer:` on each element. The subclasses of `NIAttributedLabel` that this crash is occurring in have the `accessibleElementsRememberLastValidContainer` flag set by default. This flag tells the label's accessibility elements to remember old values for the accessibility container in their implementations of `-setAccessibilityContainer:`. The elements remember the old values by assigning them to the `lastValidContainer` weak property before calling the superclass implementation of the setter. Assigning the old accessibility container values to the weak property is usually not a problem. However, it appears that it can be when the call to `-setAccessibilityContainer:` is resulting from a label deallocation, because in this case the old value being saved is the label that is being deallocated. I think this results in a call to `crashIfDeallocating()` in the `weak_register_no_lock` function that is present in our stack trace. See [this StackOverflow post](https://stackoverflow.com/questions/35991363/why-setting-object-that-is-undergoing-deallocation-to-weak-property-results-in-c) for more info. Assuming we're not willing to revert the addition of this flag, the most obvious approach to mitigating the crash is to try to make it so that this code block in the `-setAccessibilityContainer:` method is not entered during deallocation: ``` if (self.isContainerDataValid && self.rememberLastValidContainer) { self.lastValidContainer = self.accessibilityContainer; } ``` There are a few potential ways to go about doing this, but it's hard to prove that any of them work because it's hard to reproduce the bug. The approach I chose for this PR is to call `-invalidateAccessibleElements` from the `-dealloc` method in `NIAttributedLabel`. `-invalidateAccessibleElements` sets the `accessibleElements` ivar array to `nil`, which results in the `UIAccessibilityContainer` protocol methods all returning either 0 or `nil`, depending on the method. My guess is that element-by-element un-registering will not even take place with these methods returning these values. Another approach would be to set the `accessibleElementsRememberLastValidContainer` flag to `NO` in the `-dealloc` method in `NIAttributedLabel`. This will traverse the elements and set `rememberLastValidContainer` to `NO` on each one, which will prevent the problematic code block from being entered. I chose the approach I chose because it does less, and I try to have `dealloc` methods not do anything too fancy. If this approach doesn't end up working, I think we should try the other approach. --- src/attributedlabel/src/NIAttributedLabel.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/attributedlabel/src/NIAttributedLabel.m b/src/attributedlabel/src/NIAttributedLabel.m index a3e39e157..5ded33c6f 100644 --- a/src/attributedlabel/src/NIAttributedLabel.m +++ b/src/attributedlabel/src/NIAttributedLabel.m @@ -342,6 +342,8 @@ @implementation NIAttributedLabel - (void)dealloc { [_longPressTimer invalidate]; + [self invalidateAccessibleElements]; + // The property is marked 'assign', but retain count for this CFType is managed here and via // the setter. if (NULL != _textFrame) {