Memory leak in PageboyViewController+Management.swift

[RamblinWreck77]

Hello! Thanks for a great tool!

When doing a memory leak audit I found one inside your Pod.

I believe the fix is to use [weak self] on lines 71 and 76 of PageboyViewController+Management.swift

Here's a screenshot from xcode showing the leak:

https://imgur.com/a/V5mprcD

[msaps]

@RamblinWreck77 hey! Thanks for finding this, will get it fixed in Pageboy 3 (which should hopefully be out soon 😄)

[RamblinWreck77]

@msaps Hey, think it's safe to try out Pageboy 3 beta 2? This memory leak has popped back up in a new feature and it's kind of killing my implementation atm. Any news on Beta 3/release? I'll go ahead and implement the beta if you think it's close/stable.

[msaps]

@RamblinWreck77 hey! Yeah the API should be pretty stable and I'm not expecting many changes to things - so Beta 2 should be good to try.

The main reason I haven't released 3.0 just yet is I'm also rewriting Tabman which relies heavily on Pageboy. 😄

[RamblinWreck77]

@msaps Great! I will convert everything today and report back how it went.

[RamblinWreck77]

@msaps, just a heads up there still appears to be a leak in PageboyAutoScroller.

I'm using

pod 'Pageboy', git: 'https://github.com/uias/Pageboy.git', branch: 'pageboy3'

https://imgur.com/a/tJlQnGi

PageboyViewController line 178

[msaps]

@RamblinWreck77 okay cheers will try and find it! 👍 1 down at least 😄

[RamblinWreck77]

@msaps Thanks for the quick response! I'm testing a few potential fixes now, if I squash it I'll post my results here

[RamblinWreck77]

@msaps While I haven't been able to fix the leak, I have been able to switch it on and off.

Commenting out

public let autoScroller = PageboyAutoScroller()

and anything that uses it and poof, no more leak!

[RamblinWreck77]

It's ugly, but I can confirm that this fork has absolutely zero leaks.

https://github.com/RamblinWreck77/Pageboy.git

While I'm far from an expert, in our app we've added a compile-time-error lint rule banning "self." and "unowned", so I applied those rules and applied weak references where possible.

It appears this project uses a ton of block callbacks with strong references to self, and while it's possible to do it right more often then not leaks pop up.

What's nice about banning "self." is:

-any time you're accessing something that needs self the compiler will yell at you, forcing you to add [weak self] to the closure and making the programmer think long and hard about why it's needed.

-[weak self] prevents the memory leak, but it also forces the programmer to handle the null case. So like, when you do a UIView.animate and that animation completes... how do you handle self being nil? In most cases you're safe to just ignore the result since if the view controller has been nuked the animation change is useless anyways.

-By forcing weak instead of unowned, you again force the programmer to handle nil cases and further reduces the "WTF" factor of weird crashes in prod.

Note that a side effect of this is changing the init naming pattern slightly, for example

class Car {
   let tires: Tires

   init(wTires: Tires) {
      tires = wTires
   }
}

Which IMO makes since because when you call Car(wTires: myTiresObject) it's like "initialize Car with tires object"

For a simple broadcast to a delegate after a callback:

SomeClass.fetch(input: String, callback: { [weak self] result in
   self?.delegate?.gotResult(result)
})

If you have tons of branching logic and want to safely handle self not exiting

SomeClass.fetch(input: String, callback: { [weak self] result in
   if let exists = self {
      // Use "exists" instead of self because we know it exists inside this callback
   } else {
      // We're dead, so handle it. If we needed something to happen here we need to handle it properly
   }
})

From what I've found, any time we've really needed to use strong references inside nested closures it was because we were designing it wrong in the first place, so these rules helped us structure problems better.

Just my 2c, best of luck!

[msaps]

@RamblinWreck77 thanks for all the feedback and detail!

I managed to fix the leak eventually - was something weird with PageboyViewController.Transition, Swift didn't seem to like it being a struct?! 😕 I also took your feedback onboard and removed a lot of the usages of self., and cleaned up a bit.

So, finally there should be no memory leaks. Published a new beta release to celebrate: Beta 3 🎉

[RamblinWreck77]

@msaps Weird! I know I've ran into issues with structs vs classes when doing lots of heavy multi-threading (structs are on the stack, classes are on the heap... oof)

I'm building this now to test out, will report back soon. Thanks for the lightning fast turn around!

[RamblinWreck77]

@msaps Can confirm, no more leaks as of beta 3. Nice work!