gontovnik / DGElasticPullToRefresh

Elastic pull to refresh for iOS developed in Swift
https://medium.com/@gontovnik/elastic-view-animation-or-how-i-built-dgelasticpulltorefresh-269a3ba8636e#.9dioekqv6
MIT License
3.77k stars 443 forks source link

Deallocated while observers still registered #5

Open tgyhlsb opened 8 years ago

tgyhlsb commented 8 years ago

Hey !

I am trying to use your beautiful component but I am facing an error I can't solve. One of my view controller uses your DGElasticPullToRefreshwith its circle loading view. But when this view controller is deallocated I got the error :

Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An instance 0x7fd7d4343080 of class UIScrollViewPanGestureRecognizer was deallocated while key value observers were still registered with it

Any idea how to solve this ?

Thanks

tgyhlsb commented 8 years ago

After investigation, I added some code to my view controller

    deinit {
        self.tableView.dg_removePullToRefresh()
    }

You should add to your README that removing pull to refresh manually if necessary ;)

gontovnik commented 8 years ago

Hey! Good point! Completely forgot to call "observing = false" on "dealloc". Updated repo, pushing changes to cocoapods. Thanks!

tgyhlsb commented 8 years ago

Great, even better :)

ty !

gontovnik commented 8 years ago

Version 1.0.3 pushed to cocoapods:

- October 29th, 17:05: Push for `DGElasticPullToRefresh 1.0.3' initiated.
- October 29th, 17:05: Push for `DGElasticPullToRefresh 1.0.3' has been pushed (1.042036202 s).

Let me know if there are any other issues, thanks! :)

tgyhlsb commented 8 years ago

I updated to 1.0.3 but I still have the same issue. Manually removing the pull to refresh in deinit still solves it.

gontovnik commented 8 years ago

Ok, I will look at it today in the evening and will let you know :+1: thanks

gontovnik commented 8 years ago

Added it to the description. What happens is deinit() of pull to refresh is not called. It can be because it is used as an associated object. However, people say that it still should deinit automatically. Really weird. Will investigate more :+1:

ajinkyashelar commented 8 years ago

Hi @gontovnik,

I am using 1.0.3 and have added the deinit code but still my app crashes later on due to deallocated UICollectionView when the instance of that collectionview is already killed.

shanmarkus commented 8 years ago
gontovnik commented 8 years ago

Hm, really weird. Did you have a chance to investigate on that?

ryanholden8 commented 8 years ago

I'm getting this error after my UICollectionViewController subclass deinits:

An instance 0x7fc3661e8200 of class UICollectionView was deallocated while key value observers were still registered with it. Current observation info: <NSKeyValueObservationInfo 0x7fc365664580> (
<NSKeyValueObservance 0x7fc36563b560: Observer: 0x7fc36564d590, Key path: contentInset, Options: <New: YES, Old: NO, Prior: NO> Context: 0x0, Property: 0x7fc36549bd30>
)

I'm calling this in my deinit: (I have verified pullToRefreshView is not nil at this point)

pullToRefreshView?.dg_removePullToRefresh()
ryanholden8 commented 8 years ago

Further investigated revealed that scrollView() was returning nil because by the time deinit was called it had already been removed from the superview.

var observing: Bool = false {
        didSet {
            guard let scrollView = scrollView() else { return }
            if observing {
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentOffset)
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentInset)
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.Frame)
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.PanGestureRecognizerState)
            } else {
                scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentOffset)
                scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentInset)
                scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.Frame)
                scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.PanGestureRecognizerState)
            }
        }
    }

scrollView() relies on the superview being present, which is not a good assumption to rely on when it's needed to deinit properly. Those two events (removeFromSuperView & deinit) are not always the same thing.

private func scrollView() -> UIScrollView? {
        return superview as? UIScrollView
    }
ryanholden8 commented 8 years ago

The fatal error I get always states that contentInset is not deregistered. Thus it's no coincidence that is the only keyPath that is registered for observation outside of the observing didSet (posted above)

Places where its registered else where:

private func resetScrollViewContentInset(shouldAddObserverWhenFinished shouldAddObserverWhenFinished: Bool, animated: Bool, completion: (() -> ())?) {
        guard let scrollView = scrollView() else { return }

        var contentInset = scrollView.contentInset
        contentInset.top = originalContentInsetTop

        if state == .AnimatingBounce {
            contentInset.top += currentHeight()
        } else if state == .Loading {
            contentInset.top += DGElasticPullToRefreshConstants.LoadingContentInset
        }

        scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentInset)

        let animationBlock = { scrollView.contentInset = contentInset }
        let completionBlock = { () -> Void in
            if shouldAddObserverWhenFinished {
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentInset)
            }
            completion?()
        }

        if animated {
            startDisplayLink()
            UIView.animateWithDuration(0.4, animations: animationBlock, completion: { _ in
                self.stopDisplayLink()
                completionBlock()
            })
        } else {
            animationBlock()
            completionBlock()
        }
    }
private func animateBounce() {
        guard let scrollView = scrollView() else { return }

        resetScrollViewContentInset(shouldAddObserverWhenFinished: false, animated: false, completion: nil)

        let centerY = DGElasticPullToRefreshConstants.LoadingContentInset
        let duration = 0.9

        scrollView.scrollEnabled = false
        startDisplayLink()
        scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentOffset)
        scrollView.dg_removeObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentInset)
        UIView.animateWithDuration(duration, delay: 0.0, usingSpringWithDamping: 0.43, initialSpringVelocity: 0.0, options: [], animations: { () -> Void in
            self.cControlPointView.center.y = centerY
            self.l1ControlPointView.center.y = centerY
            self.l2ControlPointView.center.y = centerY
            self.l3ControlPointView.center.y = centerY
            self.r1ControlPointView.center.y = centerY
            self.r2ControlPointView.center.y = centerY
            self.r3ControlPointView.center.y = centerY
            }, completion: { _ in
                self.stopDisplayLink()
                self.resetScrollViewContentInset(shouldAddObserverWhenFinished: true, animated: false, completion: nil)
                scrollView.dg_addObserver(self, forKeyPath: DGElasticPullToRefreshConstants.KeyPaths.ContentOffset)
                scrollView.scrollEnabled = true
                self.state = .Loading
        })

        bounceAnimationHelperView.center = CGPoint(x: 0.0, y: originalContentInsetTop + currentHeight())
        UIView.animateWithDuration(duration * 0.4, animations: { () -> Void in
            self.bounceAnimationHelperView.center = CGPoint(x: 0.0, y: self.originalContentInsetTop + DGElasticPullToRefreshConstants.LoadingContentInset)
            }, completion: nil)
    }
ryanholden8 commented 8 years ago

Looks like its caused by calling db_stopLoading() twice in a row and then after that the deinit will fail to deregister the observer added in resetScrollViewContentInset

ryanholden8 commented 8 years ago

Just pulled from master and it looks like #30 addresses this issue.

Can we please cut a 1.0.4 release? @gontovnik

jatinderjk commented 7 years ago

deinit { self.tableView.dg_removePullToRefresh() }

-(void)dealloc { [self.tblVw dg_removePullToRefresh] }

worked for me in Objective c

michaelschwinges commented 7 years ago

In case it helps anyone, I kept getting this error and it was a pain because I was correctly calling db_stopLoading() in deinit().

A bit of playing around led me to realize I was adding the pull to refresh in viewWillAppear which can get called multiple times, causing multiple observers to be registered.

Moving the code that adds the control to viewDidLoad() solved this crash. I suggestion would be to note explicitly in the "example usage" on the front page to call dg_addPullToRefreshWithActionHandler() in viewDidLoad()