colinta / teacup

This project has been sunset in favor of MotionKit
github.com/motion-kit/motion-kit
Other
602 stars 85 forks source link

Instance variables are leaked when using teacup #160

Closed jsteinberg closed 10 years ago

jsteinberg commented 10 years ago

I have created a sample application here: https://github.com/jsteinberg/teacup-instance-leak

There are two rootViewControllers available. You can switch them out in the appdelegate. They are very simple and just have a button that swaps them out for a new one. Here is a screenshot from instruments when running with RootViewController. screen shot 2014-07-11 at 11 25 05 am

As expected when the controller is deallocated, the button and sub controller are also deallocated.

There is a screenshot from RootTeacupViewController screen shot 2014-07-11 at 11 24 21 am

All of the buttons are still active, and most of the old view controllers are. You can fix the sub view controller by manually nilling it in the dealloc, but the same trick does not work for the button.

I have tried looking through the teacup source and cannot figure out what is going on. The retain count of the sub controller is never more than 1, but it is not deallocated consistently. I understand that teacup has been deprecated, but we dont have the resources to switch everything over to motion-kit. It also seems to be a serious issue.

colinta commented 10 years ago

Ok, I'll definitely take a look. The sample project helps a ton! On Jul 11, 2014 10:03 AM, "Jeremy Steinberg" notifications@github.com wrote:

I have created a sample application here: https://github.com/jsteinberg/teacup-instance-leak

There are two rootViewControllers available. You can switch them out in the appdelegate. They are very simple and just have a button that swaps them out for a new one. Here is a screenshot from instruments when running with RootViewController. [image: screen shot 2014-07-11 at 11 25 05 am] https://cloud.githubusercontent.com/assets/393840/3555674/177364ee-091a-11e4-9c78-5010eb67de24.png

As expected when the controller is deallocated, the button and sub controller are also deallocated.

There is a screenshot from RootTeacupViewController [image: screen shot 2014-07-11 at 11 24 21 am] https://cloud.githubusercontent.com/assets/393840/3555872/59242d7c-091c-11e4-87e8-2baedd328208.png

All of the buttons are still active, and most of the old view controllers are. You can fix the sub view controller by manually nilling it in the dealloc, but the same trick does not work for the button.

I have tried looking through the teacup source and cannot figure out what is going on. The retain count of the sub controller is never more than 1, but it is not deallocated consistently. I understand that teacup has been deprecated, but we dont have the resources to switch everything over to motion-kit. It also seems to be a serious issue.

— Reply to this email directly or view it on GitHub https://github.com/colinta/teacup/issues/160.

colinta commented 10 years ago

Good news is I found the culprit. Bad news: the culprit is the class layout method. I think I'll have to push a new release of Teacup that removes that class method! (O_O)

Changing the code a little to use a teacup_layout method, along with some fixes to the stylesheet cache, resolves this issue. The stylesheet cache fixes are going in 2.3.1, but the layout method is a huge and necessary change, and i'll have to go in a 3.0 release.

To test it, can you install the 3.0 branch? You'll need to follow the directions that it spits out the first time you compile (it will fail if you pass a block to layout). I'd like to know if the error message is sufficient to tell you how to update your project (it should be very straightforward).

colinta commented 10 years ago
gem 'teacup', git: 'git@github.com:colinta/teacup', branch: '3.0'
jsteinberg commented 10 years ago

Just tried it out in my sample project and everything looked good. I will try it with our main app tomorrow.

Thank you

jsteinberg commented 10 years ago

I think the error message is sufficient.

jsteinberg commented 10 years ago

Just got around to trying it in our app and the change was swift and seems to be working. @colinta thanks for looking into this, have a beer on me. @changetip

changetip commented 10 years ago

Hi colinta, jsteinberg sent you a Bitcoin tip worth a beer (5.619 mBTC/$3.50), and I'm here to deliver it ➔ collect your tip at ChangeTip.com.

Is this real?