Crash in NSInvocation -dealloc when using OCMPartialMock

[sdefresne]

When using OCMPartialMock to mock a UIView that has been inserted in the view hierarchy (passed to -addSubView: of some other view), then the app crashes in NSInvocation -dealloc. This can be reproduced with the following test:

- (void)testDealloc
{
    UIView* view1 = [[UIView alloc] initWithFrame:CGRectZero];
    UIView* view2 = [[UIView alloc] initWithFrame:CGRectZero];
    [view1 addSubview:view2];
    id viewMock = OCMPartialMock(view2);
    NSLog(@"silence warning: Unused variable 'viewMock', %p", (__bridge void*)viewMock);
}

To test, just paste this in iOS9ExampleTests.m, build with Xcode 9.0, and run the tests on iPhone 6s 10.0 simulator, and you'll have a crash with the following callstack:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x000000010f7d6d2b libobjc.A.dylib`objc_release + 11
    frame #1: 0x000000010fdb920e CoreFoundation`common_removeAllObjects + 254
    frame #2: 0x000000010fc968c3 CoreFoundation`-[__NSArrayM dealloc] + 19
    frame #3: 0x000000010f7d7b12 libobjc.A.dylib`objc_object::sidetable_release(bool) + 212
    frame #4: 0x000000010f7d20c3 libobjc.A.dylib`_object_remove_assocations + 375
    frame #5: 0x000000010f7cd4ea libobjc.A.dylib`objc_destructInstance + 141
    frame #6: 0x000000010f7cd510 libobjc.A.dylib`object_dispose + 22
    frame #7: 0x000000010fce911b CoreFoundation`-[NSInvocation dealloc] + 139
    frame #8: 0x000000010f7d7b12 libobjc.A.dylib`objc_object::sidetable_release(bool) + 212
    frame #9: 0x000000010fdb920e CoreFoundation`common_removeAllObjects + 254
    frame #10: 0x000000010fc968c3 CoreFoundation`-[__NSArrayM dealloc] + 19
    frame #11: 0x000000010f7d7b12 libobjc.A.dylib`objc_object::sidetable_release(bool) + 212
  * frame #12: 0x000000011e9fb2c1 iOS9ExampleTests`-[OCMockObject dealloc](self=0x000064800009b120, _cmd=<unavailable>) at OCMockObject.m:113 [opt]
    frame #13: 0x000000011e9f92e0 iOS9ExampleTests`-[OCClassMockObject dealloc](self=0x000064800009b120, _cmd=<unavailable>) at OCClassMockObject.m:40 [opt]
    frame #14: 0x000000011e9fdda0 iOS9ExampleTests`-[OCPartialMockObject dealloc](self=0x000064800009b120, _cmd=<unavailable>) at OCPartialMockObject.m:44 [opt]
    frame #15: 0x000000010f7d7b12 libobjc.A.dylib`objc_object::sidetable_release(bool) + 212
    frame #16: 0x000000010f7d81d1 libobjc.A.dylib`(anonymous namespace)::AutoreleasePoolPage::pop(void*) + 715
    frame #17: 0x000000011b70e999 XCTest`__24-[XCTestCase invokeTest]_block_invoke + 671
    frame #18: 0x000000011b756f45 XCTest`-[XCUITestContext performInScope:] + 183
    frame #19: 0x000000011b70e6ef XCTest`-[XCTestCase invokeTest] + 141
    frame #20: 0x000000011b70f6b0 XCTest`__26-[XCTestCase performTest:]_block_invoke.369 + 42
    frame #21: 0x000000011b75bc4b XCTest`+[XCTContext runInContextForTestCase:block:] + 163
    frame #22: 0x000000011b70f04c XCTest`-[XCTestCase performTest:] + 608
    ...

This used to work with revision f03b3cc but fails with the most recent revision from github. Using git bisect, I've found that this was introduced by bf6f59a (reland of 982c6f7 with compilation fixes, this version also fails with the same error when the compilation is fixed).

The crash happens in OCMockObject -dealloc method when sending the -release signal to invocations ivar. This release the object side-table used to store the association created by -retainObjectArgumentsExcluding: so I suspect that the bug is present in that method (or maybe the bug was always there bug never visible due to the object cycle this method is trying to prevent).

[sdefresne]

I've debugged this some more and was able to create a smaller test that reproduce this crash reliably without using UIView (like the previous example, this assumes the code is compiled with ARC but the same error can be reproduced without ARC):

@interface MyObject : NSObject
@end

@implementation MyObject {
  MyObject* _contained;
}
- (void)setContained:(MyObject*)contained {
  if (_contained) {
    [_contained removedFrom:self];
    _contained = nil;
  }
  _contained = contained;
}
- (void)removedFrom:(MyObject*)container {
  NSLog(@"-removedFrom:%@ invoked on @%", container, self);
}
- (void)dealloc {
  [setContained:nil;
}
@end
- (void)testDealloc {
  MyObject* object1 = [[MyObject alloc] init];
  MyObject* object2 = [[MyObject alloc] init];
  [object1 setContained:object2];
  id objectMock = OCMPartialMock(object2);
  NSLog(@"silence warning: Unused variable 'objectMock', %p", (__bridge void*)objectMock);
}

The crash happens when the current autorelease pool is drained because the following succession of events happen:

1. object1 reference count reach zero and its -dealloc method is called,
2. object1 invokes -removedFrom: method of object2 passing self as argument,
3. since object2 is a partial mock, -handleInvocation: is called,
4. the method -retainObjectArgumentsExcluding: is invoked on the NSInvocation,
5. while iterating over the argument, object1 is added to retainedArguments as it is the sole argument to -removedFrom:,
6. the control eventually returns to object1 -dealloc method and the method returns, at this point the object is dead, any pointer to it, including the one stored in the NSInvocation referenced from the partial mock, is a dangling pointer,
7. eventually object2 reference count reaches zero, and its -dealloc method is called, causing the destruction of the captured NSInvocation that tries to send -release message to object1 (via NSMutableArray -dealloc method), since this pointer is dangling, the test crash (well undefined behaviour happens).

With the original test case, the same thing happens, view1 -dealloc method invokes -movedFromSuperview: passing self as parameter, it is captured by the NSInvocation (but too late) and the same double release happens when the NSInvocation is deallocated. It is just a bit more complex because many other methods are invoked on view2 than in the reduced test case.

TL;DR: I think capturing the NSInvocation arguments is unsafe because they could potentially be in the middle of a call to -dealloc when the invocation is handled but the mock. I would recommend not retaining any arguments of NSInvocation.

[sdefresne]

Here is the call stack when the method -retainObjectArgumentsExcluding: is invoked in that last test case:

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 4.1
  * frame #0: 0x000000011a5df34c OCMock`-[NSInvocation(self=0x000062400006f2c0, _cmd="retainObjectArgumentsExcludingObject:", objectToExclude=0x000060000009c160) retainObjectArgumentsExcludingObject:] at NSInvocation+OCMAdditions.m:88
    frame #1: 0x000000011a5e6e43 OCMock`-[OCMockObject handleInvocation:](self=0x000060000009c160, _cmd="handleInvocation:", anInvocation=0x000062400006f2c0) at OCMockObject.m:329
    frame #2: 0x000000011a5de1d9 OCMock`-[OCPartialMockObject forwardInvocationForRealObject:](self=0x0000600000019c20, _cmd="forwardInvocation:", anInvocation=0x000062400006f2c0) at OCPartialMockObject.m:228
    frame #3: 0x00000001015beed8 CoreFoundation`___forwarding___ + 760
    frame #4: 0x00000001015beb58 CoreFoundation`__forwarding_prep_0___ + 120
    frame #5: 0x000000011a5c395f OCMockDebugUITests`-[MyObject setContained:](self=0x0000600000019b60, _cmd="setContained:", contained=0x0000000000000000) at OCMockDebugUITests.m:25
    frame #6: 0x000000011a5c3a3f OCMockDebugUITests`-[MyObject dealloc](self=0x0000600000019b60, _cmd="dealloc") at OCMockDebugUITests.m:37
    frame #7: 0x0000000100fb3a2e libobjc.A.dylib`objc_object::sidetable_release(bool) + 202
    frame #8: 0x000000011a5c3c79 OCMockDebugUITests`-[OCMockDebugUITests testExample](self=0x00006080000394e0, _cmd="testExample") at OCMockDebugUITests.m:74
    frame #9: 0x00000001015c056c CoreFoundation`__invoking___ + 140
    frame #10: 0x00000001015c0440 CoreFoundation`-[NSInvocation invoke] + 320
    frame #11: 0x0000000100856949 XCTest`__24-[XCTestCase invokeTest]_block_invoke + 591
    frame #12: 0x000000010089ef45 XCTest`-[XCUITestContext performInScope:] + 183
    frame #13: 0x00000001008566ef XCTest`-[XCTestCase invokeTest] + 141
    frame #14: 0x00000001008576b0 XCTest`__26-[XCTestCase performTest:]_block_invoke.369 + 42
    frame #15: 0x00000001008a3c4b XCTest`+[XCTContext runInContextForTestCase:block:] + 163
    frame #16: 0x000000010085704c XCTest`-[XCTestCase performTest:] + 608
    frame #17: 0x0000000100853052 XCTest`__27-[XCTestSuite performTest:]_block_invoke + 363
    frame #18: 0x00000001008529b9 XCTest`-[XCTestSuite _performProtectedSectionForTest:testSection:] + 26
    frame #19: 0x0000000100852bb6 XCTest`-[XCTestSuite performTest:] + 239
    frame #20: 0x0000000100853052 XCTest`__27-[XCTestSuite performTest:]_block_invoke + 363
    frame #21: 0x00000001008529b9 XCTest`-[XCTestSuite _performProtectedSectionForTest:testSection:] + 26
    frame #22: 0x0000000100852bb6 XCTest`-[XCTestSuite performTest:] + 239
    frame #23: 0x0000000100853052 XCTest`__27-[XCTestSuite performTest:]_block_invoke + 363
    frame #24: 0x00000001008529b9 XCTest`-[XCTestSuite _performProtectedSectionForTest:testSection:] + 26
    frame #25: 0x0000000100852bb6 XCTest`-[XCTestSuite performTest:] + 239
    frame #26: 0x00000001008ab16d XCTest`__44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 40
    frame #27: 0x0000000100866232 XCTest`-[XCTestObservationCenter _observeTestExecutionForBlock:] + 475
    frame #28: 0x00000001008ab00c XCTest`-[XCTTestRunSession runTestsAndReturnError:] + 281
    frame #29: 0x00000001008426ab XCTest`-[XCTestDriver runTestsAndReturnError:] + 314
    frame #30: 0x00000001008a2eb6 XCTest`_XCTestMain + 619

As you can see, we capture an invocation of -setContained: with as part of -dealloc of a MyObject instance. In that case, the argument of -setContained: is the object currently deallocated and the reference count cannot be increased.

[erikdoe]

I think I understand what is going on, but I don't know what to do. On the one hand there are good reasons to retain the arguments. On the other hand it can, under the conditions you describe, cause a crash.

That said, the case you describe is a real edge case that, arguably, violates an implicit contract in Objective-C. It is normally a fair assumption for a method that it can retain an object argument it receives, but here you're basically creating a situation where that is not true and the receiver is not allowed to hold onto an argument it receives. Somehow the receiver must know that, unlike all "normal" object arguments, it must not hold onto (retain) that argument.

When this tacit "side-contract" is coded into a class, as it is in your example and as it seems with UIView, then it's not a problem, because the class doesn't do what it shouldn't do. However, that special contract is not explicit and OCMock can't know about it.

Interestingly, #347 is also about a problem with mocks retaining arguments of invocations. Maybe we really need some extra API on OCMock to deal with these edge cases, allowing the test case more control over how a mock manages invocations. I'm not sure exactly how such an API would look like. Ideas?

[alanterranova]

Regarding the API, what about the suggestions atop #347?

[sdefresne]

I think an API as described in #347 to disable retaining of the arguments of the captured NSInvocation would work in my case.

I found this "side-contract" in UIView while trying to understand why the some of the Chromium tests where crashing with the latest version of OCMock. Since there are only a few tests that try to mock a class from Apple frameworks, and we do not have such side-contract, it is a sustainable solution for us.

Thank you for looking into this issue.

[nchavez324]

I also agree that there needs to be a way to specify this behavior for testing deallocations, as described in #347 -- there isn't another way to do so outside of making the tests non-ARC, which is pretty unreasonable in 2017. +1

[carllindberg]

Yes, passing a reference to self in a method call from -dealloc is dangerous. You have to be sure that the value is not autoreleased in the calling code (or any other delayed release like these captured NSInvocations) -- even a normal retain/release added by ARC can be a bad idea. I think it's even too late to obtain a weak reference to such an object.

There is a private NSObject _isDeallocating method I believe. It may be possible to use that to avoid capturing the NSInvocation in the first place, or at least don't retain such arguments in the NSInvocation (better would be to invoke the original method with the value but store nil in the NSInvocation).

Another option would be to add the ability to specify that -removedFrom: or movedFromSuperview: (or similar specific methods on a particular class) should not have its NSInvocation recorded.

Or a way to turn off the implicit capturing of all methods completely for a particular test, such that you need to specify all your expects before the code is invoked, and only those specific methods will be mocked (the old OCMock behavior, which is waaaay less invasive runtime-wise for edge cases like these).

[madsolar8582]

I'm encountering the same issue. The test is as follows:

MyController *controller = [[MyController alloc] init];
OCMockObject *mockNotificationCenter = [OCMockObject partialMockForObject:NSNotificationCenter.defaultCenter];
[[mockNotificationCenter expect] postNotificationName:MyControllerViewDidLoadNotification object:controller];
    
[controller viewDidLoad];
[mockNotificationCenter verify];
XCTAssertEqual(controller.edgesForExtendedLayout, UIRectEdgeNone);
XCTAssertFalse(controller.webView.translatesAutoresizingMaskIntoConstraints);
XCTAssertNotNil(controller.activityIndicator);
    
[mockNotificationCenter stopMocking];

Granted, I can switch this particular test to leverage a XCTestExpectation via expectationForNotification:object:handler:, but other tests are experiencing this issue.

I was able to confirm what @sdefresne reported and that it is crashing during the dealloc of the mock object when the release is sent to the mock's array of NSInvocations.

ocmock/Source/OCMock/OCMockObject.m

Lines 108 to 115 in 812b5db

	- (void)dealloc
	{
	[stubs release];
	[expectations release];
	[exceptions release];
	[invocations release];
	[super dealloc];
	}

Environment:
OCMock 3.4.1 (static library)
Xcode 9.2 on macOS 10.13.3
iOS 11.2 SDK

[erikdoe]

I'm super busy at work right now. Will look into #347 in a couple of weeks, hopefully adding the feature. If you have any specific suggestions on the API, please leave a comment in #347.

[MikePendo]

Having the same issue:

    self.analyticMock = OCMPartialMock([PNDAnalyticsMessageManager sharedInstance]);
    void (^theBlock)(NSInvocation *) = ^(NSInvocation *invocation) {
           NSDictionary *value;
           [invocation getArgument:&value atIndex:2];
       };
    
    OCMStub([self.analyticMock put:[OCMArg any]])._andDo(theBlock);
    
- (void) tearDown {
    [self.analyticMock stopMocking];
    self.analyticMock = nil;
}

If I don't call getArgumnet the crash is gone