Potential memory leak / logic error in transaction error handler

[isegal]

We were troubleshooting a crash with transaction handling and stumbled upon this line (although might be unrelated).

https://github.com/firebase/flutterfire/blob/49921a4362c5965d2efeed17eb73775302007ea8/packages/cloud_firestore/cloud_firestore/ios/Classes/FLTFirebaseFirestorePlugin.m#L348

See inserted // comments for details:

    FIRTransaction *transaction = self->_transactions[transactionId];

    NSError *error = [[NSError alloc] init]; // <--- Memory allocated here
    FIRDocumentSnapshot *snapshot = [transaction getDocument:document error:&error]; // <--- pointer is immediately overwritten

    if (error != nil) { // <--- If [FIRTransaction getDocument] has any execution path where it does not set error pointer to nil, this will result in a false error because we would not be nil due to initial alloc+init on the second line.
      result.error(nil, nil, nil, error);
    } else if (snapshot != nil) {
      result.success(snapshot);
    } else {
      result.success(nil);
    }

It appears that FIRTransaction.getDocument has error as __autoreleasing

Apple's official documentation examples show that error should not be initialized:

NSError *error;
--
BOOL OK = [myObject performOperationWithError:&error];
if (!OK) {
// Report the error.
// ...

-(BOOL)performOperationWithError:(NSError * __autoreleasing *)error;

Source: https://developer.apple.com/library/archive/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html#//apple_ref/doc/uid/TP40011226-CH1-SW4

[isegal]

Posting FIRTransaction.getDocument for reference:

https://github.com/firebase/firebase-ios-sdk/blob/e8f419baf275899068e915a6c2ecb4ae1e43d264/Firestore/Source/API/FIRTransaction.mm#L157

[darshankawar]

Thanks for the report and analysis @isegal Keeping this issue open for further insights from the team.

/cc @russellwheatley

[isegal]

Update:

The issue shows up as a crash in production during transactions on spotty WiFi/Cellular networks.

Although it has been elusive to reproduce this in 'lab environment', I am able to reproduce the crash if I pause an async execution within a transaction Dart code in between transaction.get().

#0	0x0000000108c17590 in CFHash.cold.1 ()
#1	0x0000000108b00b28 in CFHash ()
#2	0x0000000108bc5e34 in CFBasicHashFindBucket ()
#3	0x0000000108ab6d14 in CFDictionaryGetValue ()
#4	0x0000000108ab9dd8 in CFErrorCopyCallBackBlockForDomain ()
#5	0x0000000108ab92b0 in _CFErrorCopyUserInfoKeyFromCallBack ()
#6	0x0000000108ab8f58 in _CFErrorCreateLocalizedDescription ()
#7	0x0000000105d3f2a4 in -[NSError localizedDescription] ()
#8	0x000000010259b984 in +[FLTFirebasePlugin createFlutterErrorFromCode:message:optionalDetails:andOptionalNSError:] at /Users/isegal/.pub-cache/hosted/pub.dartlang.org/firebase_core-1.19.1/ios/Classes/FLTFirebasePlugin.m:35

Here is what I see going on based on debugger:

1. A transaction gets created and placed into self->_transactions[transactionId]
2. In between reads (Dart side), a timeout happens and the transaction is removed from self->_transactions[transactionId]
3. When our transaction callback gets invoked we get a nil here:

4. We create an empty NSError:

    NSError *error = [[NSError alloc] init]; // <--- Memory allocated here

5. Since transaction is nil, invoking this method also results in snapshot being nil. FIRDocumentSnapshot *snapshot = [transaction getDocument:document error:&error]; // <--- pointer is immediately overwritten
6. Since we never touched error local variable, it is an empty error but not nil:

    if (error != nil) {
      result.error(nil, nil, nil, error);

7. We crash when we call error.localizedDescription:

@implementation FLTFirebasePlugin
+ (FlutterError *_Nonnull)createFlutterErrorFromCode:(NSString *_Nonnull)code
                                             message:(NSString *_Nonnull)message
                                     optionalDetails:(NSDictionary *_Nullable)details
                                  andOptionalNSError:(NSError *_Nullable)error {
  NSMutableDictionary *detailsDict = [NSMutableDictionary dictionaryWithDictionary:details ?: @{}];
  if (error != nil) {
    detailsDict[@"nativeErrorCode"] = [@(error.code) stringValue];
    detailsDict[@"nativeErrorMessage"] = error.localizedDescription; // <--- Crash here

The reason it crashes is discussed here: https://stackoverflow.com/questions/31573419/occasional-crash-when-accessing-nserror-localizeddescription

I was able to confirm this by reproducing a crash in with two lines of code:

NSError *error = [[NSError alloc] init];
      error.localizedDescription;

Other notes:

Since self->_transactions appears to be asynchronously modified across threads, then there should probably be an additional check if transaction lookup returns an empty result and we would need to callback with an error:

- (void)transactionGet:(id)arguments withMethodCallResult:(FLTFirebaseMethodCallResult *)result {
  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
    NSString *transactionId = arguments[@"transactionId"];
    FIRDocumentReference *document = arguments[@"reference"];

    FIRTransaction *transaction = self->_transactions[transactionId];

    /// <--- Check for nil transaction and invoke result.error(); return here?

    NSError *error = [[NSError alloc] init];
      error.localizedDescription;
    FIRDocumentSnapshot *snapshot = [transaction getDocument:document error:&error];

    if (error != nil) {
      result.error(nil, nil, nil, error);
    } else if (snapshot != nil) {
      result.success(snapshot);
    } else {
      result.success(nil);
    }
  });

[isegal]

The signature looks very much similar to the one reported here:

https://github.com/firebase/flutterfire/issues/6821

NOTE: I am not able to comment on #6821 but it does not look like it is related to #6900

The most likely way to reproduce this crash is with the following conditions:

1. A transaction that has many reads.
2. A spotty/unreliable connection (might be able to use Apple's Network Link conditioner
3. A fairly low timeout

[Lyokone]

Thanks for the detailed report with all your findings; it really helps! I'll do some tests on my end and publish a fix.