DGElasticPullToRefresh icon indicating copy to clipboard operation
DGElasticPullToRefresh copied to clipboard

Deallocated while observers still registered

Open tgyhlsb opened this issue 8 years ago • 17 comments

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 avatar Oct 29 '15 16:10 tgyhlsb

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 ;)

tgyhlsb avatar Oct 29 '15 17:10 tgyhlsb

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

gontovnik avatar Oct 29 '15 17:10 gontovnik

Great, even better :)

ty !

tgyhlsb avatar Oct 29 '15 17:10 tgyhlsb

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! :)

gontovnik avatar Oct 29 '15 17:10 gontovnik

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

tgyhlsb avatar Oct 29 '15 17:10 tgyhlsb

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

gontovnik avatar Oct 29 '15 17:10 gontovnik

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:

gontovnik avatar Oct 30 '15 08:10 gontovnik

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.

ajinkyashelar avatar Dec 10 '15 13:12 ajinkyashelar

  • 1

shanmarkus avatar Dec 11 '15 05:12 shanmarkus

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

gontovnik avatar Dec 11 '15 14:12 gontovnik

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 avatar Aug 04 '16 02:08 ryanholden8

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 avatar Aug 05 '16 12:08 ryanholden8

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 avatar Aug 05 '16 15:08 ryanholden8

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 avatar Aug 05 '16 17:08 ryanholden8

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

Can we please cut a 1.0.4 release? @gontovnik

ryanholden8 avatar Aug 05 '16 17:08 ryanholden8

deinit { self.tableView.dg_removePullToRefresh() }

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

worked for me in Objective c

jatinderjk avatar Jan 21 '17 07:01 jatinderjk

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()

michaelschwinges avatar Jul 30 '17 20:07 michaelschwinges