7

I'm trying to pass block arguments to a NSInvocation, but the app crashes. The invocation makes a network request and calls the success or failure blocks. I think the problem is that blocks are dealloced before the network request finishes. I managed to get it to work with some Block_copy hackery and it doesn't report any leaks using Instruments.

Questions: - Is it possible that the leak is there even though the Static Analyzer or Instruments is not reporting it? - Is there a better way to "retain" the block?

// Create the NSInvocation
NSMethodSignature *methodSignature = [target methodSignatureForSelector:selector];
NSInvocation* invoc = [NSInvocation invocationWithMethodSignature:methodSignature];
[invoc setTarget:target];
[invoc setSelector:selector];

// Create success and error blocks.
void (^successBlock)(id successResponse) = ^(id successResponse) {
    // Some success code here ...
};

void (^errorBlock)(NSError *error) = ^(NSError *error) {
    // Some failure code here ...
};

/*
Without the two Block_copy lines, the block gets dealloced too soon
and the app crashes with EXC_BAD_ACCESS
I tried [successBlock copy] and [failureBlock copy] instead,
but the app still crashes.
It seems like Block_copy is the only way to move the block to the heap in this case.
*/
Block_copy((__bridge void *)successBlock);
Block_copy((__bridge void *)errorBlock);
// Set the success and failure blocks.
[invoc setArgument:&successBlock atIndex:2];
[invoc setArgument:&errorBlock atIndex:3];

[invoc retainArguments]; // does not retain blocks

// Invoke the method.
[invoc invoke];

Update: I updated the code to below. The blocks are NSMallocBlocks, but the app still crashes.

// Create success and error blocks.
int i = 0;
void (^successBlock)(id successResponse) = ^(id successResponse) {
    NSLog(@"i = %i", i);
    // Some success code here ...
};

void (^errorBlock)(NSError *error) = ^(NSError *error) {
    NSLog(@"i = %i", i);
    // Some failure code here ...
};

/*** Both blocks are NSMallocBlocks here ***/
// Set the success and failure blocks.
void (^successBlockCopy)(id successResponse) = [successBlock copy];
void (^errorBlockCopy)(NSError *error) = [errorBlock copy];

/*** Both blocks are still NSMallocBlocks here - I think copy is a NoOp ***/

// Set the success and failure blocks.
[invoc setArgument:&successBlockCopy atIndex:2];
[invoc setArgument:&errorBlockCopy atIndex:3];

[invoc retainArguments]; // does not retain blocks

// Invoke the method.
[invoc invoke];

The blocks are passed down in the chain as follows:

NSInvocationNSProxy (NSInvocation using forwardInvocation:) → method1methodN

methodN eventually calls the success or failure block depending on the HTTP response.

Do I need to copy the block at every stage? The example above was talking about the first NSInvocation. Do I also need [invocation retainArguments]; at every appropriate step? I'm using ARC.

gklka
  • 2,233
  • 1
  • 23
  • 50
pshah
  • 1,954
  • 1
  • 19
  • 39

1 Answers1

8

Block_copy, and indeed [block copy] return copies. They don't magically switch the original with a copy at the same location. So at the very least I think you want:

successBlock = Block_copy((__bridge void *)successBlock);
errorBlock = Block_copy((__bridge void *)errorBlock); 

(or, equivalently, successBlock = [successBlock copy]; ...)

Otherwise you're creating copies, doing nothing with them and still passing the originals off to the invocation.

EDIT: so, I put the following code into a project:

@interface DummyClass: NSObject
@end

typedef void (^ successBlock)(id successResponse);
typedef void (^ failureBlock)(NSError *error);

@implementation DummyClass

- (id)init
{
    self = [super init];

    if(self)
    {
        SEL selector = @selector(someMethodWithSuccess:failure:);
        id target = self;

        // Create the NSInvocation
        NSMethodSignature *methodSignature = [target methodSignatureForSelector:selector];
        NSInvocation* invoc = [NSInvocation invocationWithMethodSignature:methodSignature];
        [invoc setTarget:target];
        [invoc setSelector:selector];

        // Create success and error blocks.
        void (^successBlock)(id successResponse) = ^(id successResponse) {
            // Some success code here ...
            NSLog(@"Off, off, off with %@", successResponse);
        };

        void (^errorBlock)(NSError *error) = ^(NSError *error) {
            // Some failure code here ...
            NSLog(@"Dance, dance, dance till %@", error);
        };

        successBlock = [successBlock copy];
        errorBlock = [errorBlock copy];

        // Set the success and failure blocks.
        [invoc setArgument:&successBlock atIndex:2];
        [invoc setArgument:&errorBlock atIndex:3];

        [invoc retainArguments]; // does not retain blocks

        // Invoke the method.
        double delayInSeconds = 2.0;
        dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delayInSeconds * NSEC_PER_SEC));
        dispatch_after(popTime, dispatch_get_main_queue(),
        ^{
            [invoc invoke];

        });
    }

    return self;
}

- (void)someMethodWithSuccess:(successBlock)successBlock failure:(failureBlock)failureBlock
{
    NSLog(@"Words:");
    successBlock(@[@"your", @"head"]);
    failureBlock([NSError errorWithDomain:@"you're dead" code:0 userInfo:nil]);
}

@end

And added the following to the end of application:didFinishLaunchingWithOptions::

DummyClass *unusedInstance = [[DummyClass alloc] init];

The result is that two seconds after launching my program the following appears on the console:

2013-06-02 20:11:56.057 TestProject[3330:c07] Words:
2013-06-02 20:11:56.059 TestProject[3330:c07] Off, off, off with (
    your,
    head
)
2013-06-02 20:11:56.060 TestProject[3330:c07] Dance, dance, dance till Error Domain=you're dead Code=0 "The operation couldn’t be completed. (you're dead error 0.)"
Tommy
  • 97,164
  • 12
  • 174
  • 193
  • I tried successBlock = [successBlock copy]; and errorBlock = [errorBlock copy]; but I get the same crash with this error: address doesn't contain a section that points to a section in a object file . As I mentioned, adding the Block_copy lines as mentioned prevents the crash, but I'm not sure if they leak memory. – pshah Jun 03 '13 at 02:51
  • The `Block_copy`s that you currently use have no documented effect. All you're seeing is that the undefined results caused by the problematic `invoke` have a different undefined effect. It's not a real solution. And even zombies won't help you debug here, since they can't keep stack objects alive artificially — once the stack grows back up it will overwrite them. – Tommy Jun 03 '13 at 03:01
  • I was under the impression that calling Block_copy forces the block to be saved on the heap instead of the stack. And, I still can't figure out why passing in [successBlock copy] instead of successBlock to the invocation wouldn't work. – pshah Jun 03 '13 at 03:12
  • Assuming you start with one on the stack, `Block_copy` creates a copy on the heap. The block you pass in has already been created on the stack so it's too late to change that. The block it returns will be on the heap. Your code discards the return result and continues to use the one on the stack. – Tommy Jun 03 '13 at 03:14
  • The code in the example DummyClass works even without those copy lines: successBlock = [successBlock copy]; and errorBlock = [errorBlock copy]; I obviously need to understand more about blocks and memory management: when blocks are dealloced – pshah Jun 03 '13 at 03:32
  • Should the example code crash without those two copy lines? I thought that those blocks would be dealloced since they are out of scope. – pshah Jun 03 '13 at 04:57
  • Without copying the blocks you should get undefined results. Maybe do something like a quick tail recursive 100 iteration loop to deliberately wind and unwind the stack? – Tommy Jun 03 '13 at 05:15
  • 2
    If you invoke the `invocation` immediately after creation, there is no need to copy the blocks. It's the responsibility of the called method to copy them if does something asynchronously. If you are not invoking the invocation instance asynchronously or saving it for future use, there's no need to copy the blocks. – Sulthan Jun 03 '13 at 08:29
  • Sulthan, someMethodWithSuccess:failure: does not copy the blocks (actually it's not even invoked before the block is dealloced). Without the copy lines, I thought the app would crash, but it doesn't. – pshah Jun 03 '13 at 19:37
  • 1
    Actually, my blocks probably bad examples because they don't actually capture any state so it's not surprising that they're safe to use after deallocation — the code in a block is compiled by the compiler at compile time, with only the captured state being owned by the block. – Tommy Jun 03 '13 at 19:49
  • Further to this, if you `NSLog` the two blocks shown, you'll see they're `__NSGlobalBlock__`s. Those are ones that don't capture any state. Unexpectedly adding some local state converted them straight to `__NSMallocBlock__`s (ie, on the heap) rather than the expected `__NSStackBlock__`s. The point that needs to be made is that you want to store malloc blocks, not stack blocks. What do you get if you log the actual blocks in the actual original code? – Tommy Jun 03 '13 at 20:02
  • If I log the blocks before the [invoc invoke]; statement, they are __NSMallocBlock__ s (even without the copy or Block_copy statements). – pshah Jun 03 '13 at 20:56
  • 1
    Then whatever crash you're seeing definitely isn't related to your block copying or not copying. Malloc blocks are already on the heap, and retaining them is good enough. – Tommy Jun 03 '13 at 21:08
  • I tried commenting out the contents of the blocks, so that they become __NSGlobalBlock__s and the crash goes away. So, now with the original block contents, they are __NSMallocBlock__s (and retained, but I copied them anyways using [block copy] anyways), but the app still crashes. – pshah Jun 03 '13 at 21:34
  • Updated the question to include sample blocks that crash. – pshah Jun 03 '13 at 21:43
  • 1
    One question is successBlock = [successBlock copy]; errorBlock = [errorBlock copy]; will cause memory leak ... do i need to release them anywhere ?? – Mihir Mehta Jun 20 '13 at 10:55