lexrus / LTMorphingLabel

[EXPERIMENTAL] Graceful morphing effects for UILabel written in Swift.
MIT License
8.07k stars 782 forks source link

memory leak #86

Closed zhengbomo closed 6 years ago

zhengbomo commented 7 years ago

LTMorphingLabel.swift displayLink does not invalidate to release target and remove from runloop

Igor-Palaguta commented 6 years ago

deinit is never called as LTMorphingLabel leaks. So invalidate in deinit will not help

lexrus commented 6 years ago

@Igor-Palaguta Thank you for your feedback. After spend a couple of minutes in my demo with Instruments, I can't find any leak. A specific line of code would be super helpful.

Igor-Palaguta commented 6 years ago

Retain cycle here. LTMorphingLabel always lives. LTMorphingLabel retains CADisplayLink, and CADisplayLink retains label as target, my recommendation to make this:

    private var displayLink: CADisplayLink?

    deinit {
        stop()
    }

    private func stop() {
        displayLink?.remove(from: .current, forMode: .commonModes)
        displayLink?.invalidate()
        displayLink = nil
    }

    public func start() {
        let displayLink = CADisplayLink(target: self, selector: #selector(displayFrameTick))
        displayLink.add(to: .current, forMode: .commonModes)
        self.displayLink = displayLink
    }

    public override func didMoveToWindow() {
        super.didMoveToWindow()

        if window == nil {
            stop()
        } else {
            start()
        }
    }

    @objc
    private func displayFrameTick(displayLink: CADisplayLink) {
zhengbomo commented 6 years ago

add a WeakWrapper class for target

lexrus commented 6 years ago

@Igor-Palaguta Thank you again for address out this stupid bug. deinit is never called. According to your code, I just did some change. Please have a look. https://github.com/lexrus/LTMorphingLabel/commit/ffa8ed58f0c395a3f63456f452dc1f40010f6ae7

@zhengbomo Just read a blog post about weak wrapper, and found a tiny OC class which forward invocations from the wrapper to the inner weak target. Here are the links: https://medium.com/fueled-engineering/memory-management-in-swift-common-issues-90dd7c08b77 https://github.com/datwelk/RDRIntermediateTarget/blob/master/RDRIntermediateTarget/RDRIntermediateTarget.m I had to say it's nifty and kind of tricky to do such a general task. I'd prefer the straightforward way. Thank you all the same.

Igor-Palaguta commented 6 years ago

@lexrus looks fine. But I also recommend to start/stop it if label becomes visible/not visible (e.g. in didMoveToWindow). As really library eats a lot of CPU and not need to load processor when label is not on the screen.

lexrus commented 6 years ago

@Igor-Palaguta Yes, I did stop the animation in didMoveToSuperview when superview is nil. But this does not affect the isHidden property. Too much override I guess.

ownmas commented 3 years ago

@lexrus Similar problem. LTMorphingLabel just doesn't deallocates when dismiss viewController. ViewController was called deinit, all other views were called deinit, but LTMorphingLabel was not called deinit

Update: I found a problem. All this closures:

var startClosures = [String: LTMorphingStartClosure]()
var effectClosures = [String: LTMorphingEffectClosure]()
var drawingClosures = [String: LTMorphingDrawingClosure]()
var progressClosures = [String: LTMorphingManipulateProgressClosure]()
var skipFramesClosures = [String: LTMorphingSkipFramesClosure]()

are capture strong reference to LTMorphingLabel object and it creates a retain cycle.

A possible solution is to use [unowned self] in every closure, but it is dangerous, because of possible force unwrap nil, better to use [weak self]