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

Tack on a (nil) argument if the selector is asking for one #1724

Merged
merged 2 commits into from Jan 15, 2014
Merged

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 18, 2013

Seems like pre-ARC would tack on a nil argument when calling performSelector: on a selector that required an argument, but for which none was provided.
Now, we have to explicitly check if the selector requires an argument, and tack one on (nil in this case) if none is provided.

Fixes @philostein's bug found Rob mentioned

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 18, 2013

With this change, I just get a crash in sendMessage: now instead of on toggle:. (Although the panel is made visible first.)

In any case, if the problem is that not enough arguments are being passed to the selector, why does it get through showClipboard: OK, but then crash on toggle:.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 15, 2014

Hmm... I see your point Rob. I think this gives the answer. When argument is nil, ARC is trying to retain junk, and causing a crash. @craigotis - have you come across this in your ARC days?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 15, 2014

From what I gather we have two ways we can go:

  • In QSObjCMessageSource, if argument is nil then we can instead send self (or maybe NSNull):
    result = [target performSelector:selector withObject:argument ? argument : self];
  • Add a new - (void)toggle; action to QSDockingWindow that accepts no argument, and revert my last commit. This would only fix that one particular method though (toggle:). My guess is that there are may more that were silently accepting nil arguments before, but now crash under ARC

Personally I think we should go with option 1, but am open to discussion

@craigotis
Copy link
Contributor

@craigotis craigotis commented Jan 15, 2014

You can use, in ARC, without error:

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification
{
    // Insert code here to initialize your application
    [self performSelector:@selector(test:) withObject:nil];
}

- (void)test:(id)something {
    NSLog(@"Testing");
}

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 15, 2014

True, that doesn't crash, but this does:

@interface ISAppDelegate (extras)
@property id thing;
@end

@implementation ISAppDelegate

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification
{
    // this crashes
    [self performSelector:@selector(test:)];
}

- (void)test:(id)sender {
    // retain 'thing'
    self.thing = sender;
    // release 'thing' (and 'sender' - which is GARBAGE!)
    self.thing = nil;

}

@end

Basically, trying to do something with the object (i.e. retain/release it) causes a crash. As for the 2nd crash - it is because in our current code we're trying to get a 'return value' from performSelector, whereas toggle: is an action of type void. This crashes in ARC as well:

@implementation ISAppDelegate

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification
{
    // this crashes
    id result = [self performSelector:@selector(test:) withObject:nil];
}

- (IBAction)test:(id)sender {
    // retain 'thing'
    self.thing = sender;
    // release 'thing' (and sender. In this case it's fine, because we've explicitly sent `nil` above)
    self.thing = nil;
    // nothing is returned... we're void
}

The use of result was never actually implemented anyway, and was causing a crash
skurfer added a commit that referenced this issue Jan 15, 2014
Tack on a (nil) argument if the selector is asking for one
@skurfer skurfer merged commit fc37101 into master Jan 15, 2014
1 check passed
@skurfer skurfer deleted the sendMessage branch Jan 15, 2014
skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 21, 2014
skurfer added a commit that referenced this issue Jan 25, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 5, 2014
skurfer added a commit that referenced this issue Feb 11, 2014
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