Memory leak (retain cycle) in [FIRQuery getDocumentsWithSource:completion:]

[ewanmellor]

[REQUIRED] Step 2: Describe your environment

• Xcode version: 10.1 (10B61)
• Firebase SDK version: 5.16.0
• Firebase Component: Firestore (Auth, Core, Database, Firestore, Messaging, Storage, etc)
• Component version: Firebase/Firestore (5.16.0), FirebaseFirestore (= 1.0.0) from CocoaPods

[REQUIRED] Step 3: Describe the problem

There is a retain cycle identified by Xcode Instruments as shown below.

From Firestore/Source/API/FIRQuery.mm:113:

- (void)getDocumentsWithSource:(FIRFirestoreSource)source
                    completion:(void (^)(FIRQuerySnapshot *_Nullable snapshot,
                                         NSError *_Nullable error))completion {

  // [Snip]

  __block id<FIRListenerRegistration> listenerRegistration;
  FIRQuerySnapshotBlock listener = ^(FIRQuerySnapshot *snapshot, NSError *error) {
    if (error) {
      completion(nil, error);
      return;
    }

    // Remove query first before passing event to user to avoid user actions affecting the
    // now stale query.
    dispatch_semaphore_wait(registered, DISPATCH_TIME_FOREVER);
    [listenerRegistration remove];

    if (snapshot.metadata.fromCache && source == FIRFirestoreSourceServer) {
      completion(nil,  /* [Snip NSError] */);
    } else {
      completion(snapshot, nil);
    }
  };

  listenerRegistration = [self addSnapshotListenerInternalWithOptions:listenOptions
                                                             listener:listener];
  dispatch_semaphore_signal(registered);
}

My reading is as follows:

1. FIRQuerySnapshotBlock listener is a callback block that is given to addSnapshotListenerInternalWithOptions.
2. That callback block references __block id<FIRListenerRegistration> listenerRegistration.
3. listenerRegistration is the result of the addSnapshotListenerInternalWithOptions call, which ultimately contains a reference to listener.
4. Nothing ever nils listenerRegistration, so there is a retain cycle and a leak once the callback is complete.

I don't know enough about the code here to be sure, but my guess is that setting listenerRegistration = nil after each of the three completion() calls would fix the problem.

[schmidt-sebastian]

Thanks for sending this over! I will route this internally to the right person and we should be able to get this fixed soon.

Looking at this briefly, the call [listenerRegistration remove] nils out its internal reference to listener, which should break the retain cycle. We do, however, not call [listenerRegistration remove] in the error case. I wonder if this is the case you are hitting.

[ewanmellor]

@schmidt-sebastian Yes, I think you're right. I've put a breakpoint on there and I'm going through that error path.

FIRQuerySnapshotBlock listener = ^(FIRQuerySnapshot *snapshot, NSError *error) {
    if (error) {
      completion(nil, error);
      return;
    }

[wilhuff]

Just FYI, I have managed to reproduce this. It only happens on the error path, but removing the listener isn't the fix since listeners automatically are unregistered when there's an error. I'm working on a fix.

[wilhuff]

@ewanmellor I haven't forgotten you, but we have a big effort underway to make this SDK available in C++ and I folded the resolution of this into that effort. Our next release window is coming up and if all goes well these fixes will end up in there. Thanks for your patience.

[wilhuff]

Those three fixes have now been submitted and will go out with our next release.