Set the ivar to nil after release it in dealloc method.

[endless7]

As #37666 (comment) mentioned, the correct pattern is the set the ivar to nil after release it in dealloc method.

Related issue: flutter/flutter#118217

Pre-launch Checklist

• [✓] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [✓] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [✓] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• [✓] I signed the [CLA].
• [✓] I listed at least one issue that this PR fixes in the description above.
• [✓] I updated/added relevant documentation (doc comments with ///).
• [✓] I added new tests to check the change I am making, or this PR is [test-exempt].
• [✓] All existing and new tests are passing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[endless7]

Hi, @cyanglaz. I have some difficulties writing the test. Is there any way to retain the ivar in the outside of FlutterEngine?

[cyanglaz]

Yes there are hacks to do that in objective-C. https://medium.com/@AdamRoberts/accessing-private-properties-and-private-ivars-in-objective-c-from-another-class-bc59edcc36cb

[jmagman]

@cyanglaz I'm surprised this is necessary per your comment in #37666 (comment), it's being dealloc'd so none of these ivars should ever be used after this point. Even in MRC code I've worked in before we didn't nil out ivars in dealloc...

[hellohuanlin]

it's been a while since I wrote MRC code, but iirc you only need to nil out things like delegates to prevent dangling pointers:

- (void) dealloc {
  [super dealloc]
  _tableView.delegate = nil 
  _tableView.dataSource = nil
}

You don't need to nil out _tableView here, because when object is deallocated, its _tableView pointer will be gone.

[cyanglaz]

@cyanglaz I'm surprised this is necessary per your comment in #37666 (comment), it's being dealloc'd so none of these ivars should ever be used after this point. Even in MRC code I've worked in before we didn't nil out ivars in dealloc...

It is to prevent tricky dangling pointers. Examples like:

- (void) dealloc {
 [_propertyA release];
 [self aCleanUpMethodDoesSomethingWithPropertyA];

 [super dealloc];
}

or a pointer pointing to this variable outside of the class.

Even for ivars that only defined in .mm files, there is no guarantee that they won't be exposed in a public getter later. When that happens, it is easy for the author to forget to nil out the ivar in dealloc.

[jmagman]

 [self aCleanUpMethodDoesSomethingWithPropertyA];

That's the pattern that justifies "never call self in init or dealloc" which seems like the real bug, not that anything needs to get nild. Definitely when this is migrated to ARC we won't have to do this.

[knopp]

Even for ivars that only defined in .mm files, there is no guarantee that they won't be exposed in a public getter later. When that happens, it is easy for the author to forget to nil out the ivar in dealloc.

Not sure I follow. How would you access these ivars through public getter after calling dealloc?

Also as @jmagman pointed out, calling methods on self within dealloc goes against google objc guidelines. I had a brief look through dealloc methods within engine, and there are couple of instances of this, but it's usually just methods that clear/deregister single ivar.

[knopp]

I would think that accessing ivar after releasing it is generally a bug, and setting it to nil just hides it so it won't be discoverable even with NSZombieEnabled.

[chinmaygarde]

This almost always indicates there is some other issue. If the instance owning the variable has been dealloc-ed, then someone managed to get a reference to that variable with incorrectly retaining it or reasoning about its lifecycle.

[jmagman]

Let's close this, and stop using this niling out pattern going forward. @endless7 Thanks for following up on the comment from the other review and sitting through this discussion 🙂