The swizzling in this module is crashing the KochavaCore SDK with .cxx_destruct about googleutilities HOT 4 CLOSED

cc: @visumickey

from googleutilities.

@johnbushnell Sorry you encountered the issue, but thank you so much for your investigation and so detailed explanation of the issue!

We will need to spend a bit more on the investigation on our end but I would like to share a couple early notes:

• while we are working on a solution that works well for both our SDKs you may recommend the customers temporary disable instrumentation in Firebase Performance which will disable swizzling.
• objc_disposeClassPair(_generatedClass); should cause issues only if there are alive instances or subclasses (possible only if the subclass was created in runtime. Another SDK/component doing ISA swizzling?) of the _generatedClass.
• instances of the mentioned GDTCOREvent should not be swizzled by Firebase/GoogleUtilities code. __destroy_helper_block_... calls in the call stack suggest that the class may've been swizzled outside of Firebase and this swizzling implementation may have its issues even without Firebase involved.

We will try to reproduce the issue on our end to get more details.

From the first glance it looks like there is another ISA swizzling implementation in the customer's app which conflicts with Firebase and potentially may cause issues on its own. If you could find out and share with us the details on the additional swizzling, it will help us significantly.

from googleutilities.

@maksymmalyhin thank you for your assistance and prompt reply.

while we are working on a solution that works well for both our SDKs you may recommend the customers temporary disable instrumentation in Firebase Performance which will disable swizzling.

Good to know.

objc_disposeClassPair(_generatedClass); should cause issues only if there are alive instances or subclasses (possible only if the subclass was created in runtime. Another SDK/component doing ISA swizzling?) of the _generatedClass.

The class KVANetTransaction is not subclassed by us (or anyone else in the sample app), nor are any classes/subclasses created at runtime. As far as any other SDK/component doing ISA swizzling, we're not doing any. Additionally we're reproducing this in our case inside of your Performance sample app. We presume that there are no other SDKs other than Firebase/Google SDKs (with ours added), but I suppose it's possible that some of the other Firebase libraries might be doing swizzling on the same object. If more swizzling is happening, that would seem to be the only possibility in the test environment we've setup.

instances of the mentioned GDTCOREvent should not be swizzled by Firebase/GoogleUtilities code. _destroy_helper_block... calls in the call stack suggest that the class may've been swizzled outside of Firebase and this swizzling implementation may have its issues even without Firebase involved.

For clarity, we're not suggesting that GDTCOREvent is being swizzled, but only that a crash is subsequently occurring in this object. This would be similar to our KVALogMessage object. We don't believe the KVALogMessage objects were being swizzled, but that they are often the unlucky benefactor of some environmental problem following the call to objc_disposeClassPair(_generatedClass); for an instance of class KVANetTransaction. In the cases where our crashes were seen, the KVANetTransaction class was always the class being swizzled, and it is that class which is indicated in the dealloc logic of class GULObjectSwizzler (where we put a print statement temporarily), but it was never identified in the crashes which occurred immediately following. It was KVALogMessage which was being identified as damaged.

From the first glance it looks like there is another ISA swizzling implementation in the customer's app which conflicts with Firebase and potentially may cause issues on its own. If you could find out and share with us the details on the additional swizzling, it will help us significantly.

Again, we're reproducing this right now in the Firebase performance sample app, which we take to be a fairly isolated environment. To the existing Firebase Performance sample project we added two of our pods to the podfile and ran pod update, adding no other libraries, and we're now observing the problem. So while our clients certainly have a lot of other libraries, those aren't a factor here, as they have been isolated away. See the sample performance.zip project for more details. Note in the modified AppDelegate that we start our SDK after having cleared away any previous install data from NSUserDefaults, ensuring that our SDK performs all of its typical first time launch behavior, and we didn't modify the sample app beyond that.

Be advised that if we comment out the line which calls objc_disposeClassPair, the crash goes away. Presumably there is a leak created there when we do this, but it does seem to prevent the problem in the short term. I also tried modifying it briefly to perform a dispatch to the main queue before doing the dispose and it still crashed. I'm now a little curious about what would happen with a dispatch_after of maybe 5 seconds, but I have not yet tried it. I think it's potentially problematic to be retaining the object like that from the dealloc method.

We can also comment out the line which sets logging to trace, and this also suppresses the crash in most cases. We take this to be because the log entry we generate in the dealloc of class KVANetTransaction does not occur. For reference, the following is the dealloc method of class KVANetTransaction:

- (void)dealloc
{
    // self.globalConsentDidMutateNotificationObserver
    [self globalConsentDidMutateNotificationObserver_remove];

    // self.attemptStartedBool
    // ⓘ This is important because the setter manages a state with KVANetTransactionQueue.  This may result in the releasing of attempt-in-progress-concurrency-maximum-regulation and rate throttling.
    self.attemptStartedBool = NO;
    
    // self.startedBool
    // ⓘ This is important because the setter manages a state with KVABackgroundTaskController.  This may result in the ending of a background task.
    self.startedBool = NO;

    // KVALogMessage
    // ⓘ This message cannot contain anything which may attempt to acquire a weak reference, such as an adapter block.  It would be nice to display the uuidString, but it would have to be accessed from a simple string.
    if (self.completeBool)
    {
        NSString *logMessageHeadlineString = [NSString stringWithFormat: @"%@ - Did Complete",
            [NSObject kva_nonnullObjectFromObject: self.nameString]
        ];
    
        [KVALogMessage printWithLogLevel:
            KVALogLevel.trace
            sourceClass: self.class
            headlineString: logMessageHeadlineString
            dictionary: nil
        ];
    }
}

We've looked over this code and asked ourselves if there's anything unsafe in general for the dealloc method going on here, and we don't see anything. If you see an issue we'd like to know. However, notice how the call to KVALogMessage printWithLogLevel passes self.class (KVANetTransaction) as a parameter in the creation of the KVALogMessage, and it will be captured there briefly while the log message is being printed and processed. Then a moment later when class KVALogMessage is in the process of deallocation, this property "sourceClass" will be released. With your swizzling, that is probably the last moment in which the swizzle class is referenced, and that means it will continue to be referenced for some time after the deallocation of the instance of KVANetTransaction which was being swizzled. That seems potentially problematic for your current implementation, although you're probably be a better judge of that than we would. I think this means that you would be getting rid of your swizzled class at a time when we had captured it, because you were assuming that nobody would be capturing self.class longer than the instance itself.

from googleutilities.

Closing for staleness. Please comment or open another issue to continue the discussion.

from googleutilities.