Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TyphoonFactoryProvider : 2nd request blows up with dangling pointer #174

Closed
jasperblues opened this issue Feb 11, 2014 · 9 comments
Closed
Labels

Comments

@jasperblues
Copy link
Member

Steps to reproduce:

Inject a TyphoonFactoryProvider built factory. Use it as follows:

- (void)checkoutPressed
{
    UIViewController* userDataController = [_userInfoControllerFactory controllerWithDelegate:self];
    [userDataController.view setFrame:CGRectMake(0, 0, self.view.width, 400)];
    [self presentOverlayController:userDataController inNavigationController:NO];
}

This presents an "ActionSheet" like view over the main view. . . 1st invocation is ok. 2nd invocation blows up with dangling pointer.

Workaround:
Get a new factory from the assembly each time. (Though this defeats the purpose of having the factory).

@jasperblues
Copy link
Member Author

Daniel, if you have time to look at this, can you please assign it to yourself?

@drodriguez
Copy link
Contributor

Wow! Never have this problem. I will have a look at it.

@drodriguez drodriguez self-assigned this Feb 11, 2014
@jasperblues
Copy link
Member Author

My guess is it's related to the new lazy stuff?

I don't have a failing test yet though I could make one if that helps. Meanwhile, I have a project here that you can look at if you like.

@jasperblues
Copy link
Member Author

Doh! Please excuse that the variable names don't match up above ;) (the factory is called userInfo and the controller is userData )

@drodriguez
Copy link
Contributor

It was so easy to reproduce that I’m embarrassed. I still testing the fix in some of the platforms, but it will be ready soon.

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Feb 11, 2014
Seems that classes created during runtime are treated as MRC, so there weren't
no retain/release for the ivars backing up the dependencies.

I choose to go back the dictionary backing, which is what has been working for
months in my code.

A new test simulate autorelease pools being drained between the first and the
second use of the factory.

See appsquickly#174.
@drodriguez
Copy link
Contributor

Sorry about this. I am still using the old version in my project, so I didn’t test the lazy dependencies in a real project.

Seems that classes created at runtime are not ARC (and I haven’t found a way to turn them to ARC), so setting an ivar was not retaining anything (when I wrote the proof of concept for the system I used a real class, so I didn’t see the problem, and the test were not releasing the object until the end, so I didn’t see the problem in the tests).

I decided to bring back the dictionary storage we were using before the lazy dependencies, since I can say for sure that it works. Other option is moving TyphoonAssistedFactoryCreator (or
parts of it) to MRC, and do the retain dance ourselves (but we will need to also do some kind of dealloc… I really don’t want to think about it).

I added a test with a couple of nested autorelease pools, but I have no time to update Typhoon in my project. I’m pretty sure it will work, but please, tell me if it works for you (and close the ticket if it does).

@jasperblues
Copy link
Member Author

Ok I will give it a try when I start work later today. If works will close. If not will workaround and leave open.

Thanks so much for your help!

Sent from my iPhone

On Feb 12, 2014, at 2:09 AM, Daniel Rodríguez Troitiño notifications@github.com wrote:

Sorry about this. I am still using the old version in my project, so I didn’t test the lazy dependencies in a real project.

Seems that classes created at runtime are not ARC (and I haven’t found a way to turn them to ARC), so setting an ivar was not retaining anything (when I wrote the proof of concept for the system I used a real class, so I didn’t see the problem, and the test were not releasing the object until the end, so I didn’t see the problem in the tests).

I decided to bring back the dictionary storage we were using before the lazy dependencies, since I can say for sure that it works. Other option is moving TyphoonAssistedFactoryCreator (or
parts of it) to MRC, and do the retain dance ourselves (but we will need to also do some kind of dealloc… I really don’t want to think about it).

I added a test with a couple of nested autorelease pools, but I have no time to update Typhoon in my project. I’m pretty sure it will work, but please, tell me if it works for you (and close the ticket if it does).


Reply to this email directly or view it on GitHub.

@jasperblues
Copy link
Member Author

That fixed it.

I had another problem where the FactoryProvider didn't seem to pick up the method on the controller despite following the 'by-convention' rules. . . maybe it was a user error. Will raise a new issue anyway.

@jasperblues
Copy link
Member Author

Pushing Typhoon 1.7.9 to CoocaPods for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants