Can no longer observe certain classes due to the weak references

[tonyxiao]

There are several classes in Cocoa that do not support weak references formed to them, including NSWindow, NSWindowController, NSViewController, etc. The latest codebase uses weak references to refer to observer and target, therefore cannot be used to observe the said classes. I will try to modify the code so that __unsafe_unretained references are formed when observing the non-weak classes.

[gwynne]

It should be fairly trivial to modify _MAKVONotificationHelper so it maintains a parallel __unsafe_unretained reference to _observer, which it already does for _target. In theory, it should be able to check +[NSObject allowsWeakReference], but in practice this is unavailable, so I would presume the same tricks used by MAZeroingWeakRef would be necessary.

That brings to mind the thought that using MAZeroingWeakRef instead of the builtin ZWR support might be the easiest solution.

In practice, using __unsafe_unretained references instead of __weak is harmless when automatic deregistration is enabled (the default), and simply returns the caller to the original semantics of Apple's KVO when it isn't, with the necessary caveats for thread-safety kept in mind.

[mikeash]

How about an explicit option to request a weak reference, which is off by default? This would put the responsibility of knowing whether the weak reference is safe to make onto the caller, and would default to the saferish case of being able to observe anything but potentially dying without automatic deregistration.

[tonyxiao]

Hmm, if __unsafe_unretained is indeed guaranteed to be safe with automatic de-registration, it makes sense to default to __unsafe_unretained rather than __weak. When using manual deregistration mode though, I was thinking of making a best-effort attempt to form weak references, to that end I wrote the following method to test whether weak reference is supported by a class, what do you think of the following as well as the approach of forming best-effort weak references falling back to __unsafe_unretained references?

/******************************************************************************/
@implementation NSObject (MAWeakReference)

static NSSet *weakRefUnavailableClasses = nil;

+ (void)load {
    // https://developer.apple.com/library/mac/#releasenotes/ObjectiveC/RN-TransitioningToARC/_index.html
    weakRefUnavailableClasses = [NSSet setWithObjects:
                                 // Classes that don't support zeroing-weak references
                                 @"NSATSTypesetter",
                                 @"NSColorSpace",
                                 @"NSFont",
                                 @"NSFontManager",
                                 @"NSFontPanel",
                                 @"NSImage",
                                 @"NSMenuView",
                                 @"NSParagraphStyle",
                                 @"NSSimpleHorizontalTypesetter",
                                 @"NSTableCellView",
                                 @"NSTextView",
                                 @"NSViewController",
                                 @"NSWindow",
                                 @"NSWindowController",
                                 // In addition
                                 @"NSHashTable",
                                 @"NSMapTable",
                                 @"NSPointerArray",
                                 // TODO: need to add all the classes in AV Foundation
                                 nil];
}

- (BOOL)ma_supportsWeakPointers {
    if ([self respondsToSelector:@selector(supportsWeakPointers)])
        return [[self performSelector:@selector(supportsWeakPointers)] boolValue];

    // NOTE: Also test for overriden implementation of allowsWeakReference in NSObject subclass.
    // We must use a bit of hackery here because by default NSObject's allowsWeakReference causes
    // assertion failure and program crash if it is not called by the runtime
    Method defaultMethod = class_getInstanceMethod([NSObject class], @selector(allowsWeakReference));
    Method overridenMethod = class_getInstanceMethod([self class], @selector(allowsWeakReference));
    if (overridenMethod != defaultMethod)
        return [[self performSelector:@selector(allowsWeakReference)] boolValue];

    // Make sure we are not one of classes that do not support weak references according to docs
    for (NSString *className in weakRefUnavailableClasses)
        if ([self isKindOfClass:NSClassFromString(className)])
            return NO;

    // Finally, all tests pass, by default objects support weak pointers
    return YES;
}

@end

/******************************************************************************/

[mikeash]

I'd recommend using the big table-driven approach used in MAZeroingWeakRef, if the desired approach is to check for weak reference compatibility at runtime:

https://github.com/mikeash/MAZeroingWeakRef/blob/master/Source/MAZeroingWeakRef.m#L484 https://github.com/mikeash/MAZeroingWeakRef/blob/master/Source/MAZeroingWeakRefNativeZWRNotAllowedTable.h

Although it's conceptually much nicer to check at runtime, the unsupported nature of the check causes App Store troubles.

[tonyxiao]

Or is it better to be fool-proof and unambiguous by requiring programmers to explicitly request weak references, and letting the code crash if weak references cannot be formed. That said we can also add a property on MAKVNotification to let the programmer know whether weak references was used.

[tonyxiao]

Wow I was googling for how to check for weak references support for a long time to no avail before finally deciding to roll my own hack, didn't realize it's been sitting in MAZeroingWeakRef the whole time. :) That said, why would the code cause app store trouble though, all the class names are taken right out of Apple's own documentation, or are you referring to the Method comparison?

[tonyxiao]

I was just looking at the big table driven approach in MAZeroingWeakRef, does it check for subclasses of the incompatible classes? Will it work if it's a custom class that implements either allowsWeakReference or supportsWeakPointers?

[mikeash]

allowsWeakReference is a private method, so referring to that in code is asking for trouble. Some of the non-weak-ref-available classes are also private, so if those are in a table, they would also cause trouble. (They probably don't need to be, since the private ones are objects you're likely never to weakly reference, but it's nice to have them for completeness.)

On the subject of subclasses, CanNativeZWRClass is implemented recursively to check all superclasses of the given class.

[tonyxiao]

I'm not sure whether allowsWeakReference is actually a private method, since it is declared in the public header inside <objc/runtime.h>, though you are not suppose to call it. The unfortunate thing is that sometimes you must implement it in order to get the correct behavior. See this stack overflow post. So even if we use big table approach, we'd still have to check for supportsWeakPointers or allowsWeakReference to be correct and complete.

[mikeash]

Looks like the method is only declared in the iOS 5 SDK. It's not on the Mac side anywhere. I guess that explains why I never saw it being public before.

If that's enough to consider it to be public (and it probably is, even on the Mac, although it's annoying that it's not declared there), then we could go ahead and use that. In that case, I'd suggest mirroring the implementation from objc-weak.mm:

    // ensure that the referenced object is viable
    BOOL (*allowsWeakReference)(id, SEL) = (BOOL(*)(id, SEL))
    class_getMethodImplementation(object_getClass(referent), 
                                  @selector(allowsWeakReference));
    if ((IMP)allowsWeakReference != _objc_msgForward) {
        if (! (*allowsWeakReference)(referent, @selector(allowsWeakReference))) {
            _objc_fatal("cannot form weak reference to instance (%p) of class %s", referent, object_getClassName(referent));
        }
    }
    else {
        return NULL;
    }

Although this may be overkill, as in this case the objects really ought to be descended from NSObject, so the responds check would be unnecessary. In that case, just if([obj allowsWeakReference]) would be enough. Otherwise, the above responds check would need to be done a little differently, since _objc_msgForward is private.

If we consider that method to be public, the other stuff with the table and the (apparently mistakenly documented) supportsWeakPointers method can be removed.

[mikeash]

I take it back, it exists on the Mac side before. I could have sworn it was not there before. I wonder if it's been added since the release of ARC. In any case, I'd say that's good enough to call it public.

[tonyxiao]

Yea, allowsWeakReference is definitely defined in 10.7 SDK inside <Foundation/NSObject.h>. However, the objc-weak.mm implementation doesn't work because NSObject by default implements allowsWeakReference and by default calling it also causes program crash.

My workaround was thus to only call allowsWeakReference if is is overridden (i.e. not the same as NSObject's implementation)

// NOTE: Also test for overriden implementation of allowsWeakReference in NSObject subclass.
// We must use a bit of hackery here because by default NSObject's allowsWeakReference causes
// assertion failure and program crash if it is not called by the runtime
Method defaultMethod = class_getInstanceMethod([NSObject class], @selector(allowsWeakReference));
Method overridenMethod = class_getInstanceMethod([self class], @selector(allowsWeakReference));
if (overridenMethod != defaultMethod)
    return [[self performSelector:@selector(allowsWeakReference)] boolValue];

The issue is that we would still need to use some kind of big table / class-name based approach in conjunction with the allowsWeakReference check to be sure that we cover all the classes. (Because NSWindowController for example has the same allowsWeakReference implementation as NSObject, and calling it would cause program crash.

[tonyxiao]

I think the question comes down to whether to use a big-table based approach or a class-name based approach if a class does not override allowsWeakReference. My thought is that the class-name based approach is smaller footprint and cleaner. If app store review is the real concern, i think worst case we could do something along the lines like

weakRefUnavailableClasses = [NSSet setWithObjects:
    // Classes that don't support zeroing-weak references
    [NSString stringWithFormat:@"%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c",'N','S','V','i','e','w','C','o','n','t','r','o','l','l','e','r'],
    ..., nil];

Although frankly I don't think we will run into review problem at all if we are just testing the classes Apple explicitly say don't support weak reference, which would certainly not cause any App store trouble because they are documented by Apple

Which classes don’t support zeroing-weak references?(Apple Docs)

You cannot currently create zeroing-weak references to instances of the following classes:

NSATSTypesetter, NSColorSpace, NSFont, NSFontManager, NSFontPanel, NSImage, NSMenuView, NSParagraphStyle, NSSimpleHorizontalTypesetter, NSTableCellView, NSTextView, NSViewController, NSWindow, and NSWindowController. In addition, in OS X no classes in the AV Foundation framework support weak references.

For declared properties, you should use assign instead of weak; for variables you should use __unsafe_unretained instead of __weak.

In addition, you cannot create weak references from instances of NSHashTable, NSMapTable, or NSPointerArray under ARC.

---

As for the private classes that are do not support weak pointers, well, first it's extremely hard to keep track of all of them, secondly I think developers using private APIs realizes that it entails a lot of responsibilities on their part to keep things working, including which private classes they use support weak references. (Which they can easy find out by having the program crash when unable to form weak references)

[mikeash]

OK, this really is a maze of twisty passages all alike.

I was extremely puzzled by the fact that the runtime clearly calls allowsWeakReference, yet it blows up if we call it ourselves. Diving into the code, it turns out that you can only safely call allowsWeakReference when the weak reference table is locked, which happens in objc_storeWeak. Thus, they can call it, but we can't.

Are there any disallowed classes which don't override allowsWeakReference? I'd be surprised if Apple didn't at least catch them all with that. The default implementation doesn't have any hooks, so you'd have to override it to get it to return NO for a live instance. If that's the case, can we just check to see if the implementation is different from the one on NSObject, call it if it is, and assume YES if it isn't? That would make for fairly simple code, and I'd hope it would be reliable.

[gwynne]

Honestly, I'm somewhat on the side of just never taking a weak reference at all. The semantics of an __unsafe_unretained reference are guaranteed when automatic deregistration is enabled - specifically, because the code is guaranteed to be called during dealloc on either observer or target, and because the helper sets are locked (good call, Mike), the references to observer and target will always be valid long enough to actually do the removeObserver:forKeyPath:context: calls. (Not 100% sure on the locking timing, Mike's better at seeing race conditions than me).

If the caller passed MAKeyValueObservingOptionUnregisterManually, they've already explicitly requested the "never deallocate without unobserving" semantics that standard KVO already has. In that mode, it is never possible to reliably deal with potentially deallocated objects and the caller has taken responsibility for that. The only thing required to make it crash-safe is to remove the deregister call from _MAKVONotificationHelper's dealloc - it's always called explicitly either by the owning caller or the swizzled dealloc, so calling it in the helper's dealloc is redundant in the current code. Without weak references, it's dangerous, with no benefit. The result of failing to unobserve will be KVO throwing its usual loud warnings when the target object is deallocated, and its usual silent crash risk when the observer is (again, matching the semantics of standard KVO).

That way, the benefit of automatic deregistration is available to all classes, and the code gains Snow Leopard compatibility at the same time.

[mikeash]

That sounds fine to me. Automatic switching would be cool, but maybe not worth the pain. I see that the manual deregistration flag already has a nice big comment warning about crashes if you screw it up, so that ought to be good enough.

[tonyxiao]

Lol, I was hooked by the idea of automatic switching as well, and actually wrote it up so that it uses weak reference by default and falls back to unsafe_unretained if needed. You can see the changes here https://github.com/tonyxiao/MAKVONotificationCenter/commit/2f789fbdb47352f1c51112c8b9254553398d4a1a

However, I agree that this seems to make the codebase quite a bit more complicated because we are keeping track of whether target / observer can be observed using weak reference. If you think it make sense and better to keep things simple (and potentially faster because we are skipping the weak reference check), I'll roll back those changes and simply change the weak to unsafe_unretained instead.

btw the code to check for weak reference compatibility is in this commit https://github.com/tonyxiao/MAKVONotificationCenter/commit/4adc5b3a215facc8f94bd1974a5b55fe8b7fff8e

[gwynne]

Weak references don't actually prevent the crashing anyway; they just make it happen at a different time. See Tests.m, the test that's commented out because passing it causes memory corruption that breaks the tests after it. There's no advantage to using them, I now realize :).

[tonyxiao]

Ok done, opened pull request.

https://github.com/mikeash/MAKVONotificationCenter/pull/4

[mikeash]

And merged. Thanks for the discussion and patch!

[tonyxiao]

Np, thanks for the awesome library. KVO Done right!

[gwynne]

Hey tonyxaio, I'd like to cite you in my Friday Q&A article about the changes we made to this code, but you don't have an email in GitHub's system, so I can't contact you directly for permission/to ask how you'd like to be credited. Toss me an email (see my profile) and let me know! Thanks :)

[tonyxiao]

hey gwynne, it doesn't seem that you got an email address listed either. But my email is tonyx.ca [at] gmail dot com. If you'd like to site me just say Tony Xiao is fine. Hey thanks, I really wasn't quite expecting this, and it really wouldn't have been necessary ;p Cheers!