Crash when calling mocks from dealloc that are not called from elsewhere

[cerihughes]

Hi Jon,

We're running into some issues where mocks injected into some code under test cause problems if the mock is called from the dealloc method AND ONLY the dealloc method.

Consider the following code under test:

@interface CodeUnderTest : NSObject <ObjectObserver>

@property (nonatomic, strong, readonly) ObservableObject *observableObject1;
@property (nonatomic, strong, readonly) ObservableObject *observableObject2;

@end

@implementation CodeUnderTest

- (instancetype)initWithObservableObject1:(ObservableObject *)observableObject1
                        observableObject2:(ObservableObject *)observableObject2
{
    self = [super init];
    if (self) {
        _observableObject1 = observableObject1;
        _observableObject2 = observableObject2;
        [_observableObject1 addObserver:self];
    }
    return self;
}

- (void)doSomethingLater
{
    [self.observableObject2 addObserver:self];
}

- (void)dealloc
{
    [_observableObject1 removeObserver:self];
    [_observableObject2 removeObserver:self];
}

@end

Note that in the init method, we observe the 1st observable, but not the 2nd. In dealloc, we stop observing both observables as we're not sure whether doSomethingLater will have been called at this point.

In a test, if I mock both observables and pass them in, the order in which I call stopMocking() suddenly becomes very important:

• observableObject2 then observableObject1 works just fine, as dealloc won't be called until observableObject1 is stopped, so by the time we call dealloc, all mocks are stopped, and the calls to removeObserver: on them do nothing.
• observableObject1 then observableObject2 causes a crash as dealloc is called as soon as observableObject1 is stopped and the call to removeObserver: on observableObject2 then creates a strong reference from the mock to the object being disposed. When we stop observableObject2, the crash manifests as we'll try to release an already released object.

Of course, this can be solved by making sure we stop the mocks in the correct order, but the test then has to be written with explicit knowledge of the implementation, and as the implementation changes, we may have to review the "stop order" to prevent further crashes. This doesn't strike me as a good thing!

I've created a PR (#140) which addresses the problem by separating the process of preventing the mock from receiving any more invocations (I call this "disabling" the mock in the PR), and stopping the mock. The downside is that tests now have to first disable, then stop all mocks to guarantee that this kind of crash won't happen.

Another option would be for OCMockito to ignore all invocations on a mock that come from the dealloc method.

WDYT?

[cerihughes]

FYI, we've actually written an OCMockito helper that tracks all mocks that are created and automatically calls stopMocking() on them during tearDown. I wonder if this would be something that we could add to OCMockito? We could then adapt it to call disableMocking() on all mocks, followed by stopMocking() on all mocks.

i.e. as well as stopMocking(mock), we could add stopAllMocks() which would perform the operations on all mocks created up to that point.

I guess it would have to be an "add-on" though - some users may like the flexibility of stopping mocks at certain points in their tests.

[jonreid]

@cerihughes I'd be interested in stopAllMocks(). But I'm also curious… what does your automatic solution look like? How does it work?

[cerihughes]

@jonreid: Agreed - I think stopAllMocks() is a much nicer approach. It means that:

• disableMocking(mock) and stopMocking(mock) don't need to be part of the public API any more, they'd just be implementation details - stopAllMocks() would make sure to call them in the correct order on all tracked mocks.
• we get a nice point to also make sure we also clean up the static instances in the OCMockito runtime (I've been playing around with unit tests that assert that all objects created during the test are also deallocated, and noticed that some of the static OCMockito instances are being reported as leaks).

Our automatic solution for tracking mocks isn't that smart... we just define our own XCTestCase subclass and duplicate the mock creation methods on there. Those methods just call the mock() equivalents and put the object in a collection before returning it. OCMockito could implement a similar solution without having to redefine the API. I'll see if I have time this week to create a PR.

Thanks!

Cer

[cerihughes]

I've spent some time investigating this more thoroughly and have realised that even with the PR #140 the problem hasn't gone away. I wrote a rough test case that captures all of the different scenarios for mocks being used in dealloc and couldn't find a way to get them all to pass.

There's 1 simple scenario that even with the current codebase, is impossible to prevent from crashing:

• We have some code under test that calls a property's method in dealloc, passing self into that method, e.g. [_observableObject removeObserver:self];
• We inject a mock instance of _observableObject into the code under test
• We don't interact with that mock in any way during the test
• We deallocate the code under test before disabling and stopping the mock.

In this test, there's no way I can see of guaranteeing that the mock object is disabled and stopped before deallocation of the code under test without the tester explicitly knowing about this problem, understanding the problem, and writing the test in such a way that the mock is stopped before the code under test is deallocated. IMO, that's too big an ask for a test writer.

The crux of the problem is that [_observableObject removeObserver:self]; triggers the following method invocations from dealloc (from bottom up):

[NSInvocation retainArguments]
[NSInvocation mkt_retainArgumentsWithWeakTarget]
[MKTInvocationContainer setInvocationForPotentialStubbing:]
[MKTBaseMockObject prepareInvocationForStubbing:]
[MKTBaseMockObject forwardInvocation:]
...................
. (NSProxy magic) .
...................
[_observableObject removeObserver:]
[DeallocationTestsRunner dealloc]

In this scenario, one of the arguments in the invocation is the object being deallocated, so we increase the retain count on this object during deallocation. When we then go to tear down the mock, we release it again, triggering a deallocation of an already deallocated object.

So, how do we get around this? I had a couple of ideas:

• Update mkt_retainArgumentsWithWeakTarget to figure out that it's been called directly from a dealloc call. Figure out what the object being deallocated is, and if it's one of the arguments, don't retain it.
• Take more control over memory management - e.g. swizzle [NSObject alloc] so that we can keep track of all objects created in a OCMockito "session", and make sure they're not released until we've disabled and stopped all mocks.

I made a quick and dirty attempt at the 1st solution using the existing logic in MKTCallStackElement. It kinda worked (I got it to detect that it was being called from a dealloc method) but the runtime performance was horrible (inspecting the call stack for every invocation of a mock is very time consuming).

The second solution seems more achievable but we'd need to introduce the concept of a OCMockito "session". This could be done with functions similar to the existing API, e.g. startMockito() (which could swizzle alloc) and stopMockito() (which would unswizzle alloc). Calling these functions in a test would be optional (so not really a change to the existing API) and tests that don't use them should still work (assuming that don't reference mocks in dealloc, but this case is broken anyway). Note that this solution has quite a lot of overlap with another PR I've submitted (which is also flawed for the same reasons) : #143 One downside of this is that we can't use any mocks to test dealloc logic in code under test, but that's already a restriction we have.

Of course, this also opens up lots of potential for things to go very badly wrong. What if a test calls startMockito() but doesn't call stopMockito(). We can't detect that and it would cause untold havoc :)

At this point, it might be worth reverting #140 as it only solves part of the problem, and also introduces a change to the API.

I'd be very interested in working with you on this, but it's probably best to talk about the direction you want the API to take first. Are you comfortable with the concept of "starting" and "stopping" mockito (i.e. a more memory-managed solution), or do you think there's more value trying to figure out a solution that can detect if dealloc is being called? Or are there any other ways to solve this that you can think of?

Finally, some good news: I ran the same tests using OCMock and it failed in exactly the same way :)

[cerihughes]

... or of course, the other option is to just document these corner cases and make sure testers know how to fix their tests when the crash occurs.

[jonreid]

That's an interesting piece of information about OCMock! That helps nudge my opinion toward your last comment: "fix with documentation" rather than trying to avoid the problem in OCMockito.

[jonreid]

Would you be willing to add a section to https://github.com/jonreid/OCMockito/wiki/OCMockito-FAQ ?

[jonreid]

I would like to roll another release. @cerihughes how do you feel about disableMocking…

• Still a keeper?
• Unending edge cases, not worth it?

[cerihughes]

Hey @jonreid! I think the latter... it hasn't made the problem go away, so let's get rid of it.

I'll write something for the FAQ when I get a chance, but it might not be for a week or so.

Thanks!

[RuudPuts]

hey @cerihughes! Any news on progress of this issue / pull request?
I'm working with a large iOS unit test suite (+/- 9500 tests) and am experiencing the same issues you describe.
For my making a build of OCMockito with your proposed changes included, along with a system to keep track of all mocks seems to improve it greatly!
Unfortunately I also see crashes when running in an CI environment, but wasn't able yet to reproduce on my development machine.

If you've made any progress that would be awesome!

[jonreid]

I'm interested in the idea of something that can register when mocks are created. Here's my main design goal:

It should be optional. That is, existing tests should work fine without this mock-cleaner-upper.

One way of achieving this would be to create a subclass of XCTestCase. When mocks are created, they can notice their context and register themselves. Cleanup would happen automatically on tearDown.

Thoughts?

[cerihughes]

Hey @RuudPuts ! No progress from me I'm afraid. We've pretty much decided to not use mocks to test any scenarios showing the dealloc problem. It's quite an edge case for us.

[cerihughes]

@jonreid - I'm a little hesitant about putting too much logic in XCTestCase subclasses. There are sometimes cases when you want to work with multiple test frameworks, e.g. KIF for functional UI testing and FBSnapshotTestCase for visual UI testing, and if all these frameworks mandate that you use an XCTestCase subclass, we run into problems.

Maybe a better compromise would to to provide all the logic outside of the XCTestCase, but also provide a XCTestCase subclass that uses it, and does all the necessary work in setUp and tearDown for those that want it (like KIF does IIRC). Best of both worlds :)

[jonreid]

@cerihughes That makes sense. I wonder if we can make something so that: when a new mock is created, it can determine whether or not this list of of managed mocks even exists. If there is no list, do nothing. If there is a list, register itself.

Something like that.

[RuudPuts]

I do agree on the mock tracker being optional, although I do think any test suite can benefit from it. Placing it in XCTestCase itself could cause problems when combining with other frameworks. An approach I've taken now is in a XCTestCase subclass define new my_mock() my_mockProtocol() etc methods, which create the mock using OCMockto's mock() mockProtocol() and store the mock created in the mock-cleaner-upper.

From the teardown of the XCTestCase then the mock-cleaner-upper is called upon to cleanup all mocks. I've tried to achieve the same with a test observer, although I didn't get that to work unfortunately.

@cerihughes I'm going to see if I can discover and fix the crashes still present after this pull request, although I can't seem to reproduce them always. Any insights you might still have I'd appreciate!

[jeremy-w]

One way of achieving this would be to create a subclass of XCTestCase. When mocks are created, they can notice their context and register themselves. Cleanup would happen automatically on tearDown.

As of Xcode 9, this could maybe be done without subclassing XCTestCase by using -addTeardownBlock:. Teardown blocks stack up during a test run, then they get run in last-in, first-out order before the -tearDown method gets called on the test case instance.