Aspects icon indicating copy to clipboard operation
Aspects copied to clipboard

Collision with KVO

Open kzaher opened this issue 9 years ago • 1 comments

Hi,

Thnx for this library :)

I have a question about potential issue that I've uncovered while trying to implement something similar in one of my own library.

BaseClass *baseClass = [[BaseClass alloc] init];
[baseClass addObserver:self forKeyPath:@"property" options:0 context:nil];
SubClass *subclass = [[SubClass alloc] init];
[subclass addObserver:self forKeyPath:@"property" options:0 context:nil];

[baseClass aspect_hookSelector:@selector(method) withOptions:0 usingBlock:^(id instance) {
    NSLog(@"class");
} error:nil];

[subclass aspect_hookSelector:@selector(method) withOptions:0 usingBlock:^(id instance) {
    NSLog(@"subclass");
} error:nil];

// Commenting these 2 lines make it to work again because it looks like wrong class is being swizzled.
// KVO will swap `object_getClass` to original after all observers have been removed {
[baseClass removeObserver:self forKeyPath:@"property"];
[subclass removeObserver:self forKeyPath:@"property"];
// }

[subclass method];

... where ...

@interface BaseClass : NSObject

@property (nonatomic, copy) NSString *property;

-(void)method;

@end

@interface SubClass : BaseClass
@end

@implementation BaseClass

-(void)method {
    NSLog(@"called base");
}

@end

@implementation SubClass

-(void)method {
    NSLog(@"called sub");
    [super method];
}

@end

It looks to me that the problem is here

static Class aspect_hookClass(NSObject *self, NSError **error) {
    NSCParameterAssert(self);
    Class statedClass = self.class;
    Class baseClass = object_getClass(self);
    NSString *className = NSStringFromClass(baseClass);

    // Already subclassed
    if ([className hasSuffix:AspectsSubclassSuffix]) {
        return baseClass;

        // We swizzle a class object, not a single object.
    }else if (class_isMetaClass(baseClass)) {
        return aspect_swizzleClassInPlace((Class)self);
        // Probably a KVO'ed class. Swizzle in place. Also swizzle meta classes in place.
    }else if (statedClass != baseClass) {
        return aspect_swizzleClassInPlace(statedClass); // <-- I believe this should be used
        //return aspect_swizzleClassInPlace(baseClass);
    }

If I understand this code correctly, if target object is already being swizzled (like with KVO), the "acted" class should be used because otherwise class returned with object_getClass could be swapped and changed again, thus killing installed observer (like it happens in that example that I've pasted).

It also seems to me that it should be

Class baseClass = self.class;
Class subclass = object_getClass(self);

... instead of ..

Class statedClass = self.class;
Class baseClass = object_getClass(self);

Is this a bug or am I doing something wrong?

Kruno

kzaher avatar Nov 29 '15 19:11 kzaher

Class statedClass = self.class; Class actualClass = object_getClass(self);

It should be named as above I think.

P.S. You can take a look at PR #115 if you need KVO coexistence.

doggy avatar Mar 03 '17 16:03 doggy