pull request #392 results in a regression

[twitterkb]

i had updated to tip-of-master and discovered a regression in a test that performs

id mock = [OCMockObject niceMockForClass:[MyClass class]]; [[[mock stub] andReturn:mock] sharedInstance];

based on the comment in #392

Previous code would only catch references to self when using the OCMStub/OCMExpect macros. Now we avoid self referential retain loops if you use the [[[foo stub] andReturn:foo] sharedInstance] API.

i reverted this patch locally, and the failing test no longer fails.

i have put a comment in pull request #392 that i would like to revert it. also opening this issue to go with it.

[dmaclach]

Hey there.. sorry you are having issues. Can you expand on how the test is failing? I'd love to see a reduced test case if possible.

[twitterkb]

a reduced test-case is probably technically "possible", though we have a fairly large code-base, and reducing this down would be pretty time-consuming.

our test is failing during cleanup, typically with an EXC_BAD_ADDRESS either in the stopMocking that gets called from dealloc, or in the .cxx_destruct for something that at first appears unrelated.

i didn't dig much deeper other than to basically git bisect and discovered which past caused our regression, and then tried simply reverting the patch on the tip of master and finding that it resolved the problem.

[dmaclach]

what that implies to me off the top of my head then is that your test is "passing" because the mock is being leaked. I assume your codebase is ARC based. Would you be able to see if patching in #391 fixes it for you as well?

[twitterkb]

i tried applying patch #391, and end up with a crash in the exact same place:

`Thread 1: EXC_BAD_INSTRUCTION (code=EXC_i386, INVOP, subcode 0x0)

#0 0x00007fff23d0517e in _CFRelease.cold.3 ()
#1 0x00007fff23bd5ee0 in _CFRelease ()
#2 0x00007fff23c28537 in __CFURLDeallocate ()
#3 0x00007fff23bd5a9f in _CFRelease ()
#4 0x00007fff5140d4c0 in _object_remove_assocations ()
#5 0x00007fff5140a4c2 in objc_destructInstance ()
#6 0x00007fff23bd5dcd in _CFRelease ()
#7 0x00007fff23c28537 in __CFURLDeallocate ()
#8 0x00007fff23bd5a9f in _CFRelease ()
#9 0x0000000102c8bfbd in _Block_byref_object_dispose at …
#10 0x00007fff522d0ca5 in _Block_object_dispose ()
#11 0x00000001028e71ae in __destroy_helper_block_e8_32r at …
#12 0x00007fff522d09c2 in _Block_release ()
#13 0x00000001054ed101 in -[OCMBlockCaller dealloc] at …/Frameworks/ThirdParty/OCMock/Source/OCMock/OCMBlockCaller.m:34
#14 0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#15 0x00007fff23b86f7f in -[__NSArrayM dealloc] ()
#16 0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#17 0x00000001054eca28 in -[OCMInvocationStub dealloc] at …/Frameworks/ThirdParty/OCMock/Source/OCMock/OCMInvocationStub.m:34
#18 0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#19 0x00007fff23b86f7f in -[__NSArrayM dealloc] ()
#20 0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#21 0x00000001054e5418 in -[OCMockObject dealloc] at …/Frameworks/ThirdParty/OCMock/Source/OCMock/OCMockObject.m:117
#22 0x00000001054e81bf in -[OCClassMockObject dealloc] at …/Frameworks/ThirdParty/OCMock/Source/OCMock/OCClassMockObject.m:40
#23 0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#24 0x00007fff5141275b in AutoreleasePoolPage::releaseUntil(objc_object**) ()
#25 0x00007fff5141267a in objc_autoreleasePoolPop ()
#26 0x00000001006da9b9 in -[XCTestCase(Failures) performFailableBlock:testCaseRun:shouldInterruptTest:] ()
#27 0x00000001006da8d7 in -[XCTestCase(Failures) _performTurningExceptionsIntoFailuresInterruptAfterHandling:block:] ()
#28 0x0000000100681b04 in __26-[XCTestCase performTest:]_block_invoke.334 ()
#29 0x00000001006ee302 in +[XCTContext runInContextForTestCase:block:] ()
#30 0x0000000100681273 in -[XCTestCase performTest:] ()
#31 0x00000001006c6807 in -[XCTest runTest] ()
#32 0x000000010067b84a in __27-[XCTestSuite performTest:]_block_invoke ()
#33 0x000000010067af74 in -[XCTestSuite _performProtectedSectionForTest:testSection:] ()
#34 0x000000010067b271 in -[XCTestSuite performTest:] ()
#35 0x00000001006c6807 in -[XCTest runTest] ()
#36 0x000000010067b84a in __27-[XCTestSuite performTest:]_block_invoke ()
#37 0x000000010067af74 in -[XCTestSuite _performProtectedSectionForTest:testSection:] ()
#38 0x000000010067b271 in -[XCTestSuite performTest:] ()
#39 0x00000001006c6807 in -[XCTest runTest] ()
#40 0x000000010067b84a in __27-[XCTestSuite performTest:]_block_invoke ()
#41 0x000000010067af74 in -[XCTestSuite _performProtectedSectionForTest:testSection:] ()
#42 0x000000010067b271 in -[XCTestSuite performTest:] ()
#43 0x00000001006c6807 in -[XCTest runTest] ()
#44 0x00000001006fd28e in __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke ()
#45 0x00000001006fd391 in __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke.84 ()
#46 0x0000000100695593 in -[XCTestObservationCenter _observeTestExecutionForBlock:] ()
#47 0x00000001006fd04d in -[XCTTestRunSession runTestsAndReturnError:] ()
#48 0x000000010065edd7 in -[XCTestDriver runTestsAndReturnError:] ()
#49 0x00000001006ea554 in _XCTestMain ()
#50 0x0000000100312c1b in main ()
#51 0x00007fff5227ec25 in start ()
#52 0x00007fff5227ec25 in start ()
`

[dmaclach]

That implies to me that you have and "andDo" block in there somewhere? In your "andDo" block do you extract Arguments or set return values on the invocation?

For extract arguments you need to be sure to use something like:

__unsafe_unretained FooObject *object;
[invocation getArgument:&object atIndex:2];

and for returns, I typically make them __autoreleasing.

Note that the __unsafe_unretained applies for not just objective c object types, but CF types, blocks, dispatch_queues, etc. Anything that could get retained/released.

I would also try running with NSZombieEnabled either via the environment variable, or Instruments.

[twitterkb]

not sure about the provenance, but it appears the stack-crawl above was produced based on bad DerivedData … 

i was able to apply patch #391 properly, clean, and run the tests without failure.

for informational purposes, we do use andDo:, and also use __unsafe_unretained for objects we use when retrieving arguments.

and for pragmatic purposes … i am sort of out of spare time for debugging the problem. i had been inclined to move forward with a locally reverted version of #392, and may do so until #391 is merged, or also if further "regressions" pop up.

[dmaclach]

Thanks @twitterkb for spending the time on it. FWIW I'm busy attempting to move Google's codebase from OCMock 3.4 to OCMock 3.6 hence all the recent patches. I agree that #391 is a crucial fix to OCMock.

[erikdoe]

It sounds like this will be fixed when #391 is merged. I'll mark it as blocked for now. Please retest when #391 is merged.

[erikdoe]

Assume fixed with #391.