Avoid retaining objects that are being deallocated.

[dmaclach]

We do not want to retain objects that are being deallocated because this will cause
a crash later when the retaining object releases them.

An example of this is when a delegate (that is mocked) is called in a dealloc for an
object. The mock retains the deallocating object as part of an invocation and then
things crash later when the invocation is released as part of the mock deallocating
itself.

[dmaclach]

Any thoughts on this one?

[imhuntingwabbits]

isDeallocating is a racey lie.

It's maybe useful as a debugging check but in that case it should be a harder failure (like a process abort / trap).

IMO this is programmer error. What have you done in your test suite that it is so unpredictable you're sending unretained objects to mocks?

I would suggest:

1. Wrap this by an environment variable or process argument (i.e. OCMOCK_CHECK_DEALLOCATING_ARGUMENTS, ocmock.check.isDeallocatingArguments)
2. Make it a hard failure with a suitable logging message (use __builtin_trap())

The stack trace should include something that looks awful like:

__ocmock_someOneSentAMockAnUnretainedArgumentThatIsNowDeallocating_allThatIsLeftToUsIsHonor

Which would mean your PR should invoke a function like this:

void __ocmock_someOneSentAMockAnUnretainedArgumentThatIsNowDeallocating_allThatIsLeftToUsIsHonor(void) {
  NSLog(@"...");
  __builtin_trap();
}

However this won't catch all occurrences. The real fix here is for clients to run their test suites with Address Sanitizer enabled. So... 🤷‍♂️

[dmaclach]

I don't disagree that isDeallocating is a potentially racey lie, but I'd appreciate suggestions out of the trap I'm in where a mock is being called with self as an argument in dealloc. See testHandlesCallingMockWithSelfAsArgumentInDealloc as an example. People do call methods with self as an argument in dealloc somewhat regularly for example:

-(void)dealloc {
  [[NSNotificationCenter defaultCenter] removeObserver:self];
}

If in this case the notificationcenter was a mock, self would be being retained by it in the mock's invocations and would crash when the mock was released.

[carllindberg]

Passing references to self to other methods in dealloc is inherently dangerous. If you are calling ARC code, it will automatically try to retain and then release the argument, but it's already deallocing so it's too late and that will lead to a crash. In almost all cases, don't do that :-). If you have to do this in APIs of your own making, it's probably best to make the API you are calling take a void *, so that it is less likely that other parts of systems will try to retain or release it.

It works with NSNotificationCenter because that is (or was) non-ARC code and is only using "self" as an __unsafe_unretained pointer, probably, to clear out its observing tables. Supposedly from iOS9 and MacOS 10.11 and up, you don't need that call in dealloc anymore.

That all said, calling that method on NSNotificationCenter from dealloc used to be required so there will be lots of code that does it, and it shouldn't crash mocks if there is a way to avoid it. Same with removing KVO observers. Any other code that passes references to self (such as in the test case example) is broken by design, in most cases.

I thought that Apple had a check which calls _objc_fatal if you call _isDeallocating directly. If it works, there should be no harm, but it may not catch all cases, and maybe on some versions of the OS it may crash directly. (See https://opensource.apple.com/source/objc4/objc4-532/runtime/NSObject.mm.auto.html , which is old, but...)

The comments in there suggest that versions of objc_storeWeak do know about deallocting objects though and won't allow a weak reference to be made to them. What happens if you change the code in question from:

id target = [self target];
to
__weak id target = [self target];

(and similar for the other changes in this PR). If that theory holds true, the weak target value should be nil if the object is deallocating, and the != nil check would then suffice to avoid retaining it.

[imhuntingwabbits]

__weak is a bit of a trap in that it autoreleases all strong references to the pointer. So anywhere you use it you need to include a local pool if you want fine grained control over the lifecycle of the referenced object.

I think it's the right change here, you would end up with a predictable 'nil' in the places where you're currently checking for isDeallocating. It's worth noting that in that world, nil often represents a coordination bug between the test and executing code, so it may be worth making it a hard failure.

[imhuntingwabbits]

I'd appreciate suggestions out of the trap I'm in where a mock is being called with self as an argument in dealloc.

Academically this trap happens because we are hiding from the event loops we've implemented and confused -dealloc with -tearDown.

Are you concerned about specific references in the OCMock framework? If so it would be useful to enumerate them and consider making them part of stopMocking.

I feel like a number of recent changes have essentially pushed us towards making stopMocking mandatory, and maybe we want to bite that bullet now and eat the bin-compat headache of everyone having to update their verify calls?

[carllindberg]

If the __weak is a problem perhaps we could use the internal function objc_storeWeakOrNil explicitly to check (and call it with with nil right after). That function is (privately) available since iOS 9 and OS X 10.11, and sounds like exactly the behavior we would want there. But the __weak may be good enough (and would probably need to use that on older OSes anyways).

[dmaclach]

Hey @carllindberg,

I totally agree with what you are saying.

Unfortunately __weak does cause issues. We get the following log at runtime:

Cannot form weak reference to instance (0x7f9ab2c02520) of class TestClassThatCallsDelegateOnDealloc. It is possible that this object was over-released, or is in the process of deallocation.

followed by a SIGABRT.

// From objc/runtime/objc-internal.h
// Only available on macOS 10.11/iOS 9.
extern id objc_initWeakOrNil(id *location, id newObj) __attribute__((weak_import));
extern void objc_destroyWeak(id _Nullable * _Nonnull location) __attribute__((weak_import));

BOOL OCMIsDeallocating(id anObject)
{
    if (!objc_initWeakOrNil)
    {
        // Pre iOS9/macOS10.11 we just assume all is well since we can't check.
        return NO;
    }
    id deallocatingObject = nil;
    BOOL isDeallocating = objc_initWeakOrNil(&deallocatingObject, anObject) == nil;
    if (deallocatingObject)
    {
        objc_destroyWeak(&deallocatingObject);
    }
    return isDeallocating;
}

appears to work. Sounds like this is preferred to _isDeallocating.

[imhuntingwabbits]

Cannot form weak reference to instance (0x7f9ab2c02520) of class TestClassThatCallsDelegateOnDealloc. It is possible that this object was over-released, or is in the process of deallocation.

That looks like the ideal behavior no? Your code is broken.

[carllindberg]

The code that is "broken" is Apple-required code before iOS 9, and a code pattern which is still supported by Apple. It works fine with the real calls, which are valid, so really the mock should not be the thing that breaks it. Mocks are not production code, so using hacks like this to avoid problems should be fine. It sounds like __weak uses the regular objc_storeWeak call still, which is coded to still crash unfortunately. So... sounds like the objc_initWeakOrNil is the way to go.

I might put a comment on the test case to note that it is not a valid coding pattern, but is used to demonstrate the issue with legacy calls to NSNotificationCenter.

[imhuntingwabbits]

I don't know what this means:

The code that is "broken" is Apple-required code before iOS 9, and a code pattern which is still supported by Apple.

The client's code that resulted in that warning is broken. They're attempting to get OCMock to establish a strong reference to an object that's dead.

I think we're saying the same thing here? We want deallocating objects to be detectable? There are a couple of ways to do this.

1. Invent OCMIsDeallocating
2. Switch to using weak references in OCMock in places where these races occur and add appropriate failure (exceptions? faults? trap()? halt_catch_fire?)

Clients who race in this way won't be helped by OCMIsDeallocating. In fact in a number of places we still continue to use the object even after this check.

For example this:

id target = [self target];
if((target != nil) && (target != objectToExclude) && !object_isClass(target))
//...

Should really just use a weak reference, __weak id target, then target will be nil.

Also this is completely broken and was never a valid paradigm:

id action;
-if(anObject == mockObject)
+if(anObject == mockObject || [anObject _isDeallocating])
{
     action = [[[OCMNonRetainingObjectReturnValueProvider alloc] initWithValue:anObject] autorelease];
}

So now we will init the return value provider with a deallocating reference? That's terrible. The caller of anObject: should fail as their call site has raced (without a capturing retain) with some other part of their code.

Part of this boils down to thread safety and clients not understanding the confinement practices in their tests. But we shouldn't make this worse by establishing references to objects that are in the process of being destroyed.

[carllindberg]

@imhuntingwabbits It used to be required to call [[NSNotifcationCenter defaultCenter] removeObserver:self] in your dealloc method. That was correct code which likely still exists in many many codebases, and still works fine if called. That particular use case is always OK when not mocking, and OCMock should still support it when mocking if possible. If that means checking for deallocating objects in a private-ish way, so be it.

The same goes with KVO's -removeObserver:forKeyPath: . That was once also typically required in -dealloc methods.

Yes, that may mean that OCMock may "support" some other broken code patterns out there. If you want to warn when this situation arises when mocking methods other than the above two situations, that would be fine too. But OCMock should not crash simply due to the extra stuff that it has added to the runtime environment, when the real code does not crash. If people are using ARC code and this code general pattern, their stuff will crash on its own without OCMock. If it works, then they have done something crafty if perhaps ill-advised, but know enough to make it work. It's their option -- OCMock shouldn't be taking an opinion on that.

It sounds like using __weak references does not go through the objc_storeWeakOrNil functionality, but rather still crashes directly, so using OCMIsDeallocating would seem to be the best way to go.

I might agree on the -andReturn: case though. How is that ever valid from within a dealloc method? I may be missing something there. Obviously, we *really * don't want the "else" condition so the proposed patch may be better than what we had, but using a deallocating "self" as a return (object) value would seem to be misuse of the OCMock APIs themselves. We could throw in that situation with a better error message too, unless there really is a valid use case I'm not thinking of.

[imhuntingwabbits]

I might agree on the -andReturn: case though. How is that ever valid from within a dealloc method? I may be missing something there. Obviously, we *really * don't want the "else" condition so the proposed patch may be better than what we had, but using a deallocating "self" as a return (object) value would seem to be misuse of the OCMock APIs themselves.

^^^This.

It used to be required to call [[NSNotifcationCenter defaultCenter] removeObserver:self] in your dealloc method.

It still is as far as I know for manual retain / release clients.

The same goes with KVO's -removeObserver:forKeyPath: . That was once also typically required in -dealloc methods.

It still should be AFAIK.

The difference here is that neither of those paradigms require KVO or NSNotificationCenter to establish strong references to the deallocating object; great pains are taken to avoid that. Attempting to acquire a strong reference to a deallocating object is always a bug. The code is broken.

That's exactly what OCMock is doing in some of these examples:

[retainedArguments addObject:target];

Now the array is poisoned.

action = [[[OCMNonRetainingObjectReturnValueProvider alloc] initWithValue:anObject] autorelease];

Now the call site is poisoned and the calling application will crash at some future point, probably at invocation time because OCMNonRetainingObjectReturnValueProvider does this:

    if([anInvocation methodIsInAllocFamily] || [anInvocation methodIsInNewFamily] ||
            [anInvocation methodIsInCopyFamily] || [anInvocation methodIsInMutableCopyFamily])
    {
        // methods that "create" an object return it with an extra retain count
        [returnValue retain];
    }
    else if([anInvocation methodIsInInitFamily])
    {
        // init family methods "consume" self and retain their return value. Do the retain first in case the return value and self are the same.
        [returnValue retain];
        [[anInvocation target] release];
    }
    [anInvocation setReturnValue:&returnValue];

There are two issues here:

1. The lifecycle of the object is controlled by the client, so these are retain bugs in their code
2. These issues are difficult to detect for lay-clients not running ASAN or with memory guards and stack logging, we should help them.

This PR addresses the first issue by passing the puck down the road. I believe it should do more. Anywhere we detect a deallocating reference (by our a private method or __weak) we should throw / abort. The client's code is broken.

Using weak references in place of a private isDeallocating simply provides for nil based detection of that case. __weak is preferred because it is self documenting in the OCMock codebase that "it is an error to run through this call site with a deallocating object". But we can invent other approaches if __weak proves intractable for older platforms.

Older platforms may require a different approach, __weak got a number of notable improvements over the last few years IIRC (memory barrier like behavior, compiler enforced concurrency? maybe?).

I would take this pull request further and replace all the call sites of isDeallocating with something like this:

#define OCMOCK_ASSERT_LIVE_OBJECT(OBJECT) ({
    if (OCMIsDeallocating(OBJECT) {
        NSLog(@"Bug in client of OCMock: %p is deallocating or no longer valid. Use MallocStackLoggingNoCompact / malloc_history to investigate further.");
        abort();
    }
})

I'll take my claims further because I'm pedantic. I posit that there is no place in the OCMock codebase where it is valid to receive / work with a deallocating reference. Methods like verify and stopMocking don't take arguments so things they touch that might be holding unretained references either need to be kept alive by the caller or use __weak and tolerate the object disappearing.

[carllindberg]

It used to be required to call [[NSNotifcationCenter defaultCenter] removeObserver:self] in your dealloc method.

It still is as far as I know for manual retain / release clients.

Apple's docs say this (doesn't mention ARC):

If your app targets iOS 9.0 and later or macOS 10.11 and later, you don't need to unregister an observer in its dealloc method. Otherwise, you should call removeObserver:name:object: before observer or any object passed to this method is deallocated

Anyways, as you say, NSNotificationCenter and KVO both ensure that they do not take a strong reference on the arguments. It is entirely possible for other people to do the same in their own code. Presumably they should be able to unit test their code. Such code would work at runtime since they ensured they were not taking a strong pointer, but introducing OCMock which adds its own code in there could break that. That is not a coding mistake, nor a misuse of OCMock APIs.

For example, are you saying that users should not be allowed to write tests which ensure that notification observers get removed? That could involve mocking NSNotificationCenter itself; I don't see why that should be prohibited. Maybe sometimes the remove is called from dealloc, maybe others call it from some other place, and you just want to verify it's called at some point. If it so happens to be called from dealloc, which would work normally due to careful coding, OCMock should not be the one adding the problems if we can avoid it. And OCMock tends to put in mocks for every method in your classes (not just ones explicitly stubbed/mocked), to record all calls so that verify calls can work after the fact, which means it could causes crashes anywhere someone has careful code like that even if they aren't explicitly testing it, since it's still called during normal execution and OCMock is inserting these NSInvocations all over the place.

I am in agreement on the third part. If OCMIsDeallocating() returns true on an argument passed to andReturn:, that could be an assertion and crash right there with a more helpful error message, since that is an misuse of OCMock APIs with an invalid parameter. But in general OCMock should be able to stub those other carefully-crafted type methods which are using the objects mainly as a pointer, without causing the crashes itself, which I think is basically what the rest of this PR is doing.

Using a __weak reference does not work, as @dmaclach showed. ObjC will simply call _objc_fatal() in that situation rather than returning nil. I.e. it's still using the regular objc_storeWeak or whatever, the one that crashes and not the safer one. So I don't see another way around it.

[imhuntingwabbits]

For example, are you saying that users should not be allowed to write tests which ensure that notification observers get removed?

Not at all. I'm implying that attempting to stub such a method (or invoke any OCMock API at all) with a deallocating object is programmer error. You have to set up the mock with a valid object, what is the point of calling it with one that's already dead?

For example, this is programmer error:

id mock = [OCMock partialMockForObject:[NSNotificationCenter defaultCenter]];
id observer = //some object alloc / init
[observer release];
[[mock expect] removeObserver:observer];
[mock verify]; [mock stopMocking];

That is broken and shouldn't be allowed to continue (OCMock should throw). As far as I know there is no good reason for that order of events to ever be tolerated because:

1. You have a trivial bug in your test
2. You have some undiscovered confinement / concurrency violation in your code that needs to be fixed

However this is valid:

id mock = [OCMock partialMockForObject:[NSNotificationCenter defaultCenter]];
id observer = //some object alloc / init
[[mock expect] removeObserver:observer];
[observer release];
[mock verify]; [mock stopMocking];

observer is alive and valid when the mock is created; the matcher can use pointer equality at invocation to verify the invocation matches the recorded expectation. As far as I can tell this PR doesn't go far enough in testing that because -[OCMInvocationMatcher matchesInvocation:] doesn't check if the recorded argument is deallocating. But I may not be recalling the matching invocation path correctly.

[carllindberg]

I agree with all that. But you say:

However this is valid:

id mock = [OCMock partialMockForObject:[NSNotificationCenter defaultCenter]];
id observer = //some object alloc / init
[[mock expect] removeObserver:observer];
[observer release];
[mock verify]; [mock stopMocking];

But isn't that exactly what this test case is doing?

- (void)testHandlesCallingMockWithSelfAsArgumentInDealloc
 {
     // Note that this test will crash on failure.
     id mock = [OCMockObject mockForClass:[TestDelegate class]];
     [[mock expect] go:OCMOCK_ANY];
     TestClassThatCallsDelegateOnDealloc *foo = [[TestClassThatCallsDelegateOnDealloc alloc] init];
     foo.delegate = mock;
     foo = nil;
     [mock verify];
 }

It's named a "delegate" there, but if that delegate was presumably written with the same carefulness of NSNotificationCenter (knowing that it can be called with a deallocating pointer, and allowing that usage), then it would work in regular execution but apparently will crash when mocked like the above. The NSInvocation code in question would seem to be invoked during the actual execution of the real methods, not just at mock setup time.

The difference there may be the usage of OCMOCK_ANY. In your example, OCMock would retain the object and prevent its dealloc from being called until the mock was cleaned up (which would also prevent testing of this do-I-clean-up-in-dealloc situation, as the verify would fail since the dealloc would not be called until stopMocking). But in the PR's test case, the last reference to foo is in the test itself, so nilling it out starts the dealloc process, which should call the "delegate" or "notification center" or whatever to clear itself out, which is exactly what it's trying to test. But at that point, at runtime, the mock will be called with a deallocating object, and apparently OCMock is currently crashing rather than allowing it the way the real code would.

[imhuntingwabbits]

But isn't that exactly what this test case is doing?

Trivially yes. What does this have to do with not establishing strong references to deallocating objects?

- (void)testHandlesCallingMockWithSelfAsArgumentInDealloc
 {
     // Note that this test will crash on failure.
     id mock = [OCMockObject mockForClass:[TestDelegate class]];
     [[mock expect] go:OCMOCK_ANY];
     TestClassThatCallsDelegateOnDealloc *foo = [[TestClassThatCallsDelegateOnDealloc alloc] init];
     foo.delegate = mock;
     foo = nil;
     [mock verify];
 }

Here the declaration of the expectation happens with a totally valid object (OCMOCK_ANY). Pedantically this test is actually broken. The usage of OCMOCK_ANY here is wrong; it should be passing foo to the expectation.

[[mock expect] go:OCMOCK_ANY];
//should be
[[mock expect] go:foo];

Foo is still alive and completely valid here. Establish a strong reference away! But that breaks the test as you point out.

Now... how do we make that deallocate? That's technically an orthogonal topic but we'd need an API surface that holds a pointer or a weak reference, whatever "it's fine". Off the top of my head one easy way to do that test is with a block based verifier:

//just spitball'ing, not sure this compiles but in MRR this dance works like this, maybe arc needs __weak here?
id fooRef = foo;
[[mock expect] go:[OCMArg checkWithBlock:^BOOL(id obj) {
   return (fooRef == obj);
}];
[foo release]; foo = nil;
[mock verify];

Note in this trivial test none of the references to foo are ever actually invalid, nor do we try to acquire strong references to foo during dealloc.

1. At [moc expect] time foo is alive and well
2. At [self.delegate go:self] time foo is deallocating but is totally sane for pointer equality an no one tries to retain it
3. At verify / stopMocking time no one cares about foo anymore. The block check was invoked and passed.

So all you're really fighting is the implicit retain in expect. Which is fine, the block constraint method will work as long as you don't accidentally incur an implicit block_copy. Alternatively we could invent a new constraint for that: [OCMArg nonRetainedArgument:foo] or similar.

and apparently OCMock is currently crashing rather than allowing it the way the real code would.

I mean... that's a bug, we should definitely fix that which I think this PR does? But remember that matching / fulfilling an expectation is not the same thing as creating one.

Matchers / verifiers can generally work just fine with deallocating arguments if they're written to use pointer equality. If they have to call isEqual all bets are off and the test should fail, don't do that.

My main point is that expect / stub can't work with deallocating references. That's totes broken, what were you trying to do that the object is invalid before you set up the expectations for it?

I'm implying that attempting to stub such a method (or invoke any OCMock API at all) with a deallocating object is programmer error.

Really is the meat of my concern. Nowhere in the above test case does the test attempt to hand the OCMock framework an invalid object. I still feel like that should be a hard failure.

[carllindberg]

//just spitball'ing, not sure this compiles but in MRR this dance works like this, maybe arc needs __weak here?
id fooRef = foo;
[[mock expect] go:[OCMArg checkWithBlock:^BOOL(id obj) {
   return (fooRef == obj);
}];
[foo release]; foo = nil;
[mock verify];

The block would retain the object here, as would the local variable of fooRef since you don't nil it out. (In optimized code, fooRef may be immediately released, but in unoptimized, the way most unit tests are run, I believe it will main in scope and retained until the end of the method). Changing it to __weak means it could be nil by checking time (there are no guarantees), and break the test that way. Perhaps making fooRef __unsafe_unretained would work. We may need a new OCMArg method of -isEqualToObjectPointer:(__unsafe_unretained id)pointer or -isPointer:(void *)pointer or something like that, which would explicitly just keep the pointer around in a void * reference and not retain it directly, and compare pointer values (perhaps just wrapping the implementation you have there).

In this case, they have scoped the test and mock such that there should only be one call to the method in question, so OCMOCK_ANY was "close enough" while still allowing the test to work.

1. At [moc expect] time foo is alive and well

Correct.

2. At [self.delegate go:self] time foo is deallocating but is totally sane for pointer equality an no one tries to retain it.

Incorrect. Because the go: method is stubbed, it calls into the forwardInvocation implementation of OCMock at this point, and thus its handleInvocation: methods. OCMock wants to keep the information around for later use by verify. Therefore, it adds all object arguments to a retainedArguments array. This is where OCMock is adding the retain to a deallocating object where the real code is not. It's possible that the check of !OCMIsDeallocating(argument) on line 87 is the only real change needed here -- the target should already be the objectToExclude argument, and it may be dubious to call a method that returns self from a dealloc method. But they don't really hurt, either, and the check on line 87 is absolutely necessary to me.

This is the main point of this PR to me; in those cases it is now avoiding adding a retain if the argument is deallocating. OCMock was causing the problem where even a normal -invoke call on the NSInvocation would not, since it's doing a version of -retainArguments on the NSInvocation which is not there without OCMock.

3. At verify / stopMocking time no one cares about foo anymore. The block check was invoked and passed.

You might care if you are making comparisons to the original values, instead of OCMOCK_ANY. OCMock tries to support verify-after-the-fact. But that may not be a valid approach in this case, because the object is already deallocated after the fact, so you really need to set up expectations etc. before the call. Unless of course you kept around an __unsafe_unretained pointer yourself, and were verifying after the fact using those -- that may be a valid use case. Unsure if that would work even with this PR. But at least with this PR there is a way to do it, where currently there is not, since OCMock itself will cause the crash either way.

I agree that you should not be setting up expect/stub/andReturn: with deallocating pointers. I am therefore dubious of the change in OCMStubRecorder -- that should probably error out with a better error message, as I think it's invalid use of the API to pass in a deallocating instance there, though there may be a use case I'm not thinking of. But the rest of the PR is valid to me, since OCMock also involves itself during regular execution of the code.

[imhuntingwabbits]

Incorrect. Because the go: method is stubbed, it calls into the forwardInvocation implementation of OCMock at this point, and thus its handleInvocation: methods. OCMock wants to keep the information around for later use by verify.

That is an implementation detail not a requirement of the laws of physics. Your earlier points about block capture are valid but not insurmountable.

You might care if you are making comparisons to the original values, instead of OCMOCK_ANY. OCMock tries to support verify-after-the-fact. But that may not be a valid approach in this case, because the object is already deallocated after the fact, so you really need to set up expectations etc. before the call.

Indeed, these are mutually exclusive (and therefore probably deserve separate API?). You can't do deep object comparison on deallocated objects; some tests want things to deallocate other tests might want verify later.

It sounds like this means there are a couple changes that need to be made here:

1. API for non-retained comparisons (or better examples / tests)
2. A more thorough review of the framework's consumption of deallocating objects (like OCMStubRecorder)

Anything else?

[carllindberg]

The additions to check pointers would avoid the need for OCMOCK_ANY in the test case, which would be nice. But that wouldn't solve the crash either way (just make the test more accurate), so that could be a different PR.

I think this PR likely has dealt with the places that OCMock is adding extra retains during execution -- it's primarily around the invocations. I agree that the andReturn: change probably should be an error condition, but otherwise this PR addresses part 2, I think. I think OCMock needs to retain the arguments in the invocations most of the time; not sure how things would work otherwise. This just avoids retaining them if they are being dealloced, which is a valid runtime situation in rare cases, so may as well handle it.

[carllindberg]

It looks like we are going back to calling _isDeallocating directly, which may crash on older OSes.

[carllindberg]

It looks like we are going back to calling _isDeallocating directly, which may crash on older OSes.

[carllindberg]

It looks like we are going back to calling _isDeallocating directly, which may crash on older OSes.