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

Crash #1401

Closed
wants to merge 11 commits into from
Closed

Crash #1401

wants to merge 11 commits into from

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 20, 2013

Fixes the crash discussed on the dev groups

return label;

return nil;
label = [[meta objectForKey:kQSObjectAlternateName] retain];
Copy link
Member

@tiennou tiennou Feb 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leak.

Copy link
Member Author

@pjrobertson pjrobertson Feb 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why's this a leak?

this line is only ever called if label == nil, and as such, there's nothing to release

Copy link
Member

@tiennou tiennou Feb 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah. This ivar looked to me like a local variable, so I was misled into thinking it's a unbalanced retain (sometimes those diffs really miss context). The return line below should read return [[label retain] autorelease] though. The one above should to the same ;-).

Here's how I'd write it :

- (NSString *)label {
    if (!label)
        label = [[meta objectForKey:kQSObjectAlternateName] retain];

    return [[label retain] autorelease];
}

@tiennou
Copy link
Member

@tiennou tiennou commented Feb 20, 2013

Just a thought, we should nil the ivars in dealloc too.

@tiennou
Copy link
Member

@tiennou tiennou commented Feb 20, 2013

Also, most of those accessors aren't thread-safe (they lack @synchronized(self)). I know there's duplication between ivars and data/meta but maybe we could take the time to straighten all this ?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 20, 2013

I've pushed a cleaner getter.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 21, 2013

After making the QSPureStringRanking changes, my searching/mnemonics were up the spout. So clearly it is used for something. I've reverted that commit.

@skurfer
Copy link
Member

@skurfer skurfer commented Feb 21, 2013

Closing this, as I'm going to merge it manually.

@skurfer skurfer closed this Feb 21, 2013
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 21, 2013

Can you delete this branch when you're done as well (I don't want to do it incase you haven't cherry-picked yet)

Ta

skurfer added a commit that referenced this issue Feb 21, 2013
skurfer
Copy link
Member

@skurfer skurfer commented on c068eb2 Feb 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm troubleshooting a weird problem that this introduced. Still looking into it, but for at least one proxy object (maybe all of them) the noIdentifier flag is YES, causing the identifier method to always return nil and breaking mnemonics. Just FYI. I'll probably figure it out before you see this. :-)

skurfer
Copy link
Member

@skurfer skurfer commented on c068eb2 Feb 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Pull request after lunch.

pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented on c068eb2 Feb 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjrobertson pjrobertson deleted the crash branch Feb 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants