firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Firestore: Fix memory leak in Query.whereField()

Open dconeybe opened this issue 11 months ago • 2 comments

This PR fixes an apparent memory leak that is detected by the "Xcode Leaks" tool. As of today, this leak has been reported at least twice: https://github.com/firebase/firebase-ios-sdk/issues/13978 and https://github.com/firebase/firebase-ios-sdk/issues/12613. I don't completely understand why this is flagged as a memory leak because, to me, I cannot find any "leaks"; however, I fixed the detected leak anyways. The investigation also uncovered an unrelated use-after-free bug that was fixed as well: https://github.com/firebase/firebase-ios-sdk/pull/14306.

The leaks originated from std::shared_ptr<ThreadSafeMemoizer> usages, which manifested many public APIs, including Query.whereField(), queryWhereFieldPath:arrayContainsAny, and FIRQuery.addFields(using:).

Before this PR, all instances of ThreadSafeMemoizer were held in an std::shared_ptr so that the memoized value could be shared among copies of the object of which the std::shared_ptr is a member, and also to work around the limitation that ThreadSafeMemoizer itself was neither copyable nor moveable.

This PR completely rewrites ThreadSafeMemoizer<T> to make it both moveable and copyable, thus removing the need to store instances of it in a std::shared_ptr. The re-write has a single member variable of type std::shared_ptr<T> rather than a member variable of T directly. In order to make accesses to the std::shared_ptr<T> thread-safe, all access are done using the std::atomic_XXX() free functions, such as std::atomic_store(), std::atomic_load(), and std::atomic_compare_exchange_weak(). These std::atomic_XXX() free functions are deprecated in C++20, at which time the implementation should be changed to use std::atomic<std::shared_ptr<T>>, which is less error-prone.

The API of the re-written ThreadSafeMemoizer<T> also changes the const T& memoize(std::function<T()>) member function to const T& value(const std::function<std::shared_ptr<T>()>&); that is, it was renamed from "memoize" to "value" and the given function is expected to return std::shared_ptr<T> instead of just T. Doing this avoids unnecessarily copying or moving the T object into a newly-created std::shared_ptr<T> by ThreadSafeMemoizer<T>, thus supporting T objects that are not copyable and/or moveable.

The test suite for ThreadSafeMemoizer<T> was also vastly improved to test things like thread safety, copy and move operations, and invocation semantics. Various utility functions and classes were added into thread_safe_memoizer_testing.h (and implemented in the corresponding .cc file), with tests added for those utility classes and functions too.

Fixes: #13978

dconeybe avatar Jan 03 '25 05:01 dconeybe

AI(dconeybe): Verify that the implementation that uses std::shared_ptr internally still fixes the memory leak.

Update: It does NOT fix the memory leak. I'm reverting back to the std::atomic implementation.

dconeybe avatar Jan 06 '25 21:01 dconeybe

Uh oh, TSAN found a data race in ThreadSafeMemoizer

https://github.com/firebase/firebase-ios-sdk/actions/runs/12706313819/job/35419112948?pr=14300

https://gist.github.com/dconeybe/e05bd4b0b3476c95b205ad32ac0b49ba

UPDATE Fixed by https://github.com/firebase/firebase-ios-sdk/pull/14300/commits/4608f23f274d65cf2fe314c73d8c730a99547b29

dconeybe avatar Jan 10 '25 16:01 dconeybe