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

Fix a possible crash when releasing a CF obj #1775

Merged
merged 2 commits into from Feb 11, 2014
Merged

Fix a possible crash when releasing a CF obj #1775

merged 2 commits into from Feb 11, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 11, 2014

I think this crash just now only occurred because of a caches problem, but if you see that when fileURL == nil then urlRef == NULL. We all know that trying to CFRelease(NULL) is bad.

@@ -299,7 +299,7 @@ - (NSArray *)validIndirectObjectsForAction:(NSString *)action directObject:(QSOb
if ([action isEqualToString:kFileOpenWithAction]) {
NSURL *fileURL = nil;
// comma trick - get a list of apps based on the 1st selected file
fileURL = [NSURL fileURLWithPath:[[dObject validPaths] objectAtIndex:0]];
fileURL = [NSURL fileURLWithPath:[[dObject validPaths] objectAtIndex:0]];

CFURLRef urlRef = NULL;
if (fileURL) LSGetApplicationForURL((__bridge CFURLRef) fileURL, kLSRolesAll, NULL, &urlRef);
Copy link
Member

@tiennou tiennou Feb 11, 2014

Choose a reason for hiding this comment

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

Maybe also :

NSURL *appURL = nil;
if (fileURL) {
    CFURLRef urlRef = NULL;
    LSGetApplicationForURL((__bridge CFURLRef) fileURL, kLSRolesAll, NULL, &urlRef);
    if (urlRef) {
        appURL = (__bridge_transfer NSURL*)urlRef; // Ta-daa !
    }
}

Let's make ARC handle memory management, right ;-) ?

@tiennou
Copy link
Member

@tiennou tiennou commented Feb 11, 2014

LGTM, but the indentation is wrong ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 11, 2014

Nah, that’s what Xcode says. Ignore GitHub. It’s wrong ;-)

On 12 Chwef 2014, at 01:41, Etienne Samson notifications@github.com wrote:

LGTM, but the indentation is wrong ;-).


Reply to this email directly or view it on GitHub.

@tiennou
Copy link
Member

@tiennou tiennou commented Feb 11, 2014

God Bless Xcode then.

tiennou added a commit that referenced this issue Feb 11, 2014
Fix a possible crash when releasing a CF obj
@tiennou tiennou merged commit 9298da9 into master Feb 11, 2014
1 check passed
@tiennou tiennou deleted the smallcrash branch Feb 11, 2014
@skurfer
Copy link
Member

@skurfer skurfer commented Feb 11, 2014

D’oh! You guys snuck this in while I wasn’t paying attention. 😃

It’s included in today’s dev preview, but was not mentioned in the release notes. Not that big a deal.

Update

I’ve added it to the release notes and pushed an updated copy to qsapp.com, so it’s just the in-app release notes that will be wrong until the next preview.

skurfer added a commit that referenced this issue Feb 11, 2014
@tiennou
Copy link
Member

@tiennou tiennou commented Feb 11, 2014

Heh, sorry about that. I think that can be called a "submarine merge" ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 12, 2014

Great work Etienne, us Europeans foiled the Americans! ;-)

On 12 Chwef 2014, at 05:01, Etienne Samson notifications@github.com wrote:

Heh, sorry about that. I think that can be called a "submarine merge" ;-).


Reply to this email directly or view it on GitHub.

skurfer added a commit that referenced this issue Mar 19, 2014
skurfer added a commit that referenced this issue Apr 14, 2014
skurfer added a commit that referenced this issue May 13, 2014
skurfer added a commit that referenced this issue May 30, 2014
skurfer added a commit that referenced this issue Aug 7, 2014
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