Open sdefresne opened 7 years ago
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:
object1
reference count reach zero and its -dealloc
method is called,object1
invokes -removedFrom:
method of object2
passing self
as argument,object2
is a partial mock, -handleInvocation:
is called,-retainObjectArgumentsExcluding:
is invoked on the NSInvocation
,object1
is added to retainedArguments
as it is the sole argument to -removedFrom:
,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,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
.
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.
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?
Regarding the API, what about the suggestions atop #347?
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.
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
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).
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. https://github.com/erikdoe/ocmock/blob/812b5db86a46db8a4a5d45464fe3229532fd9f29/Source/OCMock/OCMockObject.m#L108-L115
Environment: OCMock 3.4.1 (static library) Xcode 9.2 on macOS 10.13.3 iOS 11.2 SDK
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.
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
See #470
When using
OCMPartialMock
to mock aUIView
that has been inserted in the view hierarchy (passed to-addSubView:
of some other view), then the app crashes inNSInvocation
-dealloc
. This can be reproduced with the following test: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:This used to work with revision https://github.com/erikdoe/ocmock/commit/f03b3cc126edc8d6a2d4466d227fb41a1b2c2a14 but fails with the most recent revision from github. Using
git bisect
, I've found that this was introduced by https://github.com/erikdoe/ocmock/commit/bf6f59ad28e48f804fc063fcd9d7e691144f48cc (reland of https://github.com/erikdoe/ocmock/commit/982c6f7a106bd4dad0ff49e4a281b28c6d2600f5 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 toinvocations
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).