Capturing NSStackBlock leads to EXC_BAD_ACCESS

[fleupold]

OCHamcrest/Source/Library/Object/HCArgumentCaptor.m

Lines 33 to 40 in 11dcba8

	- (void)capture:(id)item
	{
	if (self.captureEnabled)
	{
	id value = item ?: [NSNull null];
	[self.values addObject:value];
	}
	}

Let's say we want to test code like this:

- (void)foo {
  __weak typeof(self) weakSelf = self;
  [_asyncWorker doWorkWithCompletion:^(){
    [weakSelf _onCompletion];
  }];
}

The completion block will be of type NSStackBlock since it is never assigned to a variable. If we try to test this code using a mocked asyncWorker like this:

MKTArgumentCaptor *captor = [MKTArgumentCaptor new];
[verify(mockedWorker) doWorkWithCompletion:captor.capture];

[objectUnderTest foo];

dispatch_block_t completion = captor.value;
completion();

we run into a EXC_BAD_ACCESS, because the foo's stack has gone out of scope before completion is called and therefore we are accessing the block after it's been released.

NSStackBlocks get copied on the heap lazily (upon first copy). Usually ARC inserts block copy statements automagically across method boundaries. However, it doesn't do so if the method signature specifies a non-block type (which - (void)capture:(id)item does).

If, on the other hand, you pass a block pointer as an argument to a function parameter whose declared compile-time type is a non-block object pointer type, then that function wouldn't be taking responsibility for any block copying, because for all it knows it's just a regular object, that just needs to be retained if stored in a place that outlives the current scope. In this case, if you think that the function may possibly store the value beyond the end of the call, you should copy the block before passing it, and pass the copy instead. (https://stackoverflow.com/questions/35822645/understand-one-edge-case-of-block-memory-management-in-objc)

Currently, my workaround is to either in the production code assign the block to a local variable, explicitly call copy before passing, or use NSInvocation in the test code to get the argument from the invocation and manually copy it. Both solutions are not ideal.
Can we add a check for NSStackBlocks in capture: and call copy if we are dealing with a NSStackBlock? Or is there another solution?

[jonreid]

I tried to reproduce the problem, but the following code successfully runs (and logs YO):

@interface AsyncWorker : NSObject
@end

@implementation AsyncWorker
- (void) doWorkWithCompletion:(void (^)(void))completion {
}
@end


@interface Worker : NSObject
@property (nonatomic, strong) AsyncWorker *asyncWorker;
@end

@implementation Worker

- (void)foo {
    __weak typeof(self) weakSelf = self;
    [self.asyncWorker doWorkWithCompletion:^(){
        [weakSelf onCompletion];
    }];
}

- (void)onCompletion
{
    NSLog(@"YO!");
}

@end


@interface UngaTests : XCTestCase
@end

@implementation UngaTests

- (void)testFoo
{
    AsyncWorker *mockedWorker = mock([AsyncWorker class]);
    Worker *worker = [[Worker alloc] init];
    worker.asyncWorker = mockedWorker;
    HCArgumentCaptor *captor = [[HCArgumentCaptor alloc] init];
    
    [worker foo];
    [verify(mockedWorker) doWorkWithCompletion:(id)captor];
    
    dispatch_block_t completion = captor.value;
    completion();
}

@end

Are you on iOS, or something else?

[fleupold]

Thanks for taking the time to look into this. I am on iOS and you are right that neither mine nor your example repros the issue. I made a mistake distilling this down from the much more complex test case where I noticed the issue.

It turns out this only happens with class mocks. Take the following example that is very similar to yours (it turns doWorkWithCompletion into a class method and uses OCMClassMock/OCMExpect instead of mock/verify).

@interface AsyncWorker : NSObject
@end

@implementation AsyncWorker
+ (void) doWorkWithCompletion:(void (^)(void))completion {
}
@end


@interface Worker : NSObject
@end

@implementation Worker

- (void)foo {
  __weak typeof(self) weakSelf = self;
  [AsyncWorker doWorkWithCompletion:^(){
    [weakSelf onCompletion];
  }];
}

- (void)onCompletion
{
  NSLog(@"YO!");
}

@end


@interface UngaTests : XCTestCase
@end

@implementation UngaTests

- (void)testFoo
{
  id mockedWorker = OCMClassMock([AsyncWorker class]);
  Worker *worker = [[Worker alloc] init];
  HCArgumentCaptor *captor = [[HCArgumentCaptor alloc] init];
  
  OCMExpect([mockedWorker doWorkWithCompletion:(id)captor]);
  [worker foo];
  
  dispatch_block_t completion = captor.value;
  completion();
}

@end

This crashes for me.

[jonreid]

Reproduced.

[jonreid]

Fixed in 4a40c4a. Turns out the problem was that HCArgumentCaptor needs to copy its arguments, not just retain them. Blocks are kind of funny about how long they live.

Thank you for reporting this!

[jonreid]

Now released in v7.1.0