Avoid dangling weak references to deallocating objects.

[Graham--M]

Fixes #198

The previous checks for a deallocating object with negative reference count did not work because:

1. the sign bit has to be recreated as was happening in objc_delete_weak_refs()
2. there was no distinction between a saturated count and a negative reference count

refcount_max now indicates when the refcount has saturated and should no longer be mutated. An underflow to -1 still maps to refcount_mask, allowing us to detect when an object is supposed to be deallocated. Neither objc_release_fast_no_destroy_np() nor objc_retain_fast_np() mutate the refcount when it is one of those values, so the comment in objc_delete_weak_refs() was adjusted.

[Graham--M]

Hmm. The Windows-2016 Debug-32 job seems to be stuck on the ManyManySelectors test. Every other job completed under 6 minutes.

[davidchisnall]

The Win32 builds sometimes run out of memory on those tests, I should probably disable them there.

Would you mind adding a test so that we catch regressions here?

[Graham--M]

Sure, I'll give it a shot. Presumably it's acceptable to directly set the reference count in order to check the behaviour?

[Graham--M]

I've added a test for the expected behaviours. There is a little overlap with WeakRefLoad.m but I think it's better to keep the simple checks in that test.

The pre-emption case can't be directly tested for in a reasonable manner but it's less likely to happen if the functions involved pass the testing. The behaviour around the count saturating is tested by directly setting the refcount to avoid doing a ridiculous number of retains.

I went to add a check that you can't init a weak ref to a deallocating object then load it but then I noticed that objc_initWeak() and objc_storeWeak() are supposed to set the pointer to nil if the object is deallocating, but it's currently possible to init a weak ref when this is the case.

Should I try handle this? It would mean either moving the persistent object check out of setObjectHasWeakRefs() or returning the deallocation status from it by reference.

[triplef]

I think this change may have caused the following failures in GNUstep Base tests:

NSFileHandle/singleton:
Failed test:     (2021-03-17 16:59:18.408 +0000) singleton.m:24 ... stdin persists
Failed test:     (2021-03-17 16:59:18.408 +0000) singleton.m:25 ... stdout persists
Failed test:     (2021-03-17 16:59:18.408 +0000) singleton.m:26 ... strerr persists

GSFileHandle will resurrect objects by calling -retain in -dealloc: https://github.com/gnustep/libs-base/blob/753c907938c2a8c4d00cf0fbe01b7e0d020f0064/Source/GSFileHandle.m#L354

Could this have been affected by this change?

[davidchisnall]

@triplef, would you be able to prepare a reduced test case for this?

[triplef]

I tried, but unfortunately wasn’t able to repro this in a straightforward way in libobjc2 tests. I’m guessing that’s because Base overriding retain/release/retainCount in NSObject is affecting this, but I haven’t figured out how exactly.

[davidchisnall]

Looking at the GNUstep code, it appears that it's relying on undefined behaviour: Resurrection is not permitted in Objective-C. I don't know what macOS does if you try to deallocate a singleton, I'd imagine that it aborts the program. With ARC and weak references, the weak references should have all gone away by the time that -dealloc is called and so if you'll end up zeroing the weak references but not destroying the object, which is a very odd behaviour.

[triplef]

That makes sense. I already changed the GSFileHandle implementation to no longer resurrect in dealloc.

However, I think I found another issue with this change: self no longer works inside a block that is run from within dealloc when building with ARC.

- (void)ping
{
  NSLog(@"%@ ping", self);
}

- (void)dealloc
{
  [self ping];

  void (^block)() = ^{
    NSLog(@"%@ block", self);
    [self ping];
  };
  block();
}

The above will print the following (note that there should be a second "ping"):

<Test: 0xb855aef4> ping
(null) block

This happens even if self is referenced again in dealloc after calling the block.

We found this because YapDatabase is doing things along the lines of the above.

[Graham--M]

However, I think I found another issue with this change: self no longer works inside a block that is run from within dealloc when building with ARC.

I'll try make a PR that always returns the instance pointer when it's not a retain for a weak reference.