TextureGroup / Texture

Smooth asynchronous user interfaces for iOS apps.
https://texturegroup.org/
Other
8k stars 1.29k forks source link

[ASCellNode] Retain Cycle with Backing UIViewController #1720

Open DreamingInBinary opened 4 years ago

DreamingInBinary commented 4 years ago

Texture Version - Master

If you use ASCellNode's initializer to provide a view controller block, it seems to hang on to that controller when, in my own testing, it no longer needs it. This also appears to cause a retain cycle where both the cell node and controller still hang around, even if you change your data source to new view controllers instances and reload its data.

Here's what I did to fix this:

- (void)didLoad
{
  [super didLoad];

  if (_viewControllerBlock != nil) {

    _viewController = _viewControllerBlock();
    _viewControllerBlock = nil;

    if ([_viewController isKindOfClass:[ASViewController class]]) {
      ASViewController *asViewController = (ASViewController *)_viewController;
      _viewControllerNode = asViewController.node;
      [_viewController loadViewIfNeeded];
    } else {
      // Careful to avoid retain cycle
      UIViewController *viewController = _viewController;
      _viewControllerNode = [[ASDisplayNode alloc] initWithViewBlock:^{
        return viewController.view;
      }];
    }
    [self addSubnode:_viewControllerNode];

    // Since we just loaded our node, and added _viewControllerNode as a subnode,
    // _viewControllerNode must have just loaded its view, so now is an appropriate
    // time to execute our didLoadBlock, if we were given one.
    if (_viewControllerDidLoadBlock != nil) {
      _viewControllerDidLoadBlock(self);
      _viewControllerDidLoadBlock = nil;
    }

    // ADDED: Release the controller since we've got a view controller node now
     _viewController = nil;
  }
}

Since the controller appears to only be used to setup the _viewControllerNode, nilling it out fixes the issue for me - but I'm unsure of the impact. If it sounds reasonable, I'm happy to open a PR.

rahul-malik commented 4 years ago

Mind putting up a PR for this?

DreamingInBinary commented 4 years ago

Got one right here

rahul-malik commented 4 years ago

Thanks for the PR, will merge soon. Did you see the retain cycle in the memory debugger? Just wanted to make sure we were making the proper fix here