Fix most Unity leaks caused by incorrect SWIG configuration.

var-const · a-maurice · commit 210bfc270c78 · 2020-06-09T17:45:16.000-07:00
There were two major sources of leaks: - SWIG was configured to ignore destructors of the public C++ classes; consequently, all of them were leaked; - SWIG was leaking the standard containers (and all their elements). The former problem is solved by fixing the SWIG configuration (as outlined in https://docs.google.com/document/d/1qqYXGzbB-01l00Mv-dGbGRXZn9XCAHjGtwstgj31R1Q). The intended SWIG way to use the standard library containers is to instantiate them using the `%template` directive. Unfortunately, this runs in a few problems in our build environment: - `swig_post_process.py` script changes visibility of any entity containing `Internal` in its name to `internal` (see https://cs.corp.google.com/piper///depot/google3/firebase/app/client/unity/swig_post_process.py?rcl=267221433&l=438). This breaks the C# code that SWIG generates for `std::vector`; - the older version of SWIG we're using doesn't support `std::unordered_map`. To work around this, C++ wrapper functions and classes are used to ensure that `std::vector` and `std::unordered_map` objects are never actually created in the C# layer. For example, the C# code would create a `FieldValueMap` (which is a simple non-template class) which will get converted into the underlying `std::unordered_map` in the C++ layer. Running "Leaks" instrument in XCode gives the following numbers when running a somewhat-outdated version of the Unity testapp (containing 29 tests): - ~7.1K leaks baseline; - ~4.5K leaks when enabling destructors for Firestore public C++ classes; - 69 leaks with this CL, a 99% reduction. Fixing the remaining leaks will be done in a follow-up. PiperOrigin-RevId: 314847898
diff --git a/firestore/src/include/firebase/csharp/map_helper.h b/firestore/src/include/firebase/csharp/map_helper.h
@@ -5,55 +5,217 @@
 #include <vector>
 
 #include "app/meta/move.h"
+#include "firebase/firestore/document_reference.h"
+#include "firebase/firestore/document_snapshot.h"
 #include "firebase/firestore/field_path.h"
 #include "firebase/firestore/field_value.h"
-#include "firebase/firestore/map_field_value.h"
+#include "firebase/firestore/set_options.h"
+#include "firebase/firestore/transaction.h"
+#include "firebase/firestore/write_batch.h"
 
 namespace firebase {
 namespace firestore {
 namespace csharp {
 
-// FieldValue is converted to FieldValueInternal of internal access, which
-// makes it impossible to use standard containers. What's more,
-// MapFieldValue type depends on compilation settings. Providing the helper
-// here, we provide a more transparent layer for C# calls.
-
-// We cannot call std::vector<FieldValue>::foo() in C# since it is not
-// exposed via the .dll interface as SWIG cannot convert it. Here we
-// essentially define a function, which SWIG can convert and thus expose it
-// via the .dll. So C# code can call it.
-
-// It is OK to re-construct a vector of strings since those strings can be
-// re-used by the C# code. This way is more clean than to expose an
-// iterator.
-inline std::vector<std::string> map_fv_keys(const MapFieldValue& self) {
-  std::vector<std::string> result;
-  result.reserve(self.size());
-  for (const auto& kv : self) {
-    result.push_back(kv.first);
-  }
-  return result;
-}
-
-inline const FieldValue& map_fv_get(const MapFieldValue& self,
-                                    const std::string& key) {
-  return self.at(key);
-}
-
-inline MapFieldValue map_fv_create() { return MapFieldValue{}; }
-
-inline void map_fv_set(MapFieldValue* self, const std::string& key,
-                       FieldValue value) {
-  // This could be either a move-assignment or a normal assignment.
-  (*self)[key] = firebase::Move(value);
-}
-
-inline MapFieldPathValue map_fpv_create() { return MapFieldPathValue{}; }
-
-inline void map_set(MapFieldPathValue* self, FieldPath key, FieldValue value) {
-  // These could be either move or copy.
-  (*self)[firebase::Move(key)] = firebase::Move(value);
-}
+// Simple wrappers to avoid exposing C++ standard library containers to SWIG.
+//
+// While it's normally possible to work with standard library containers in SWIG
+// (by instantiating them for each template type used via the `%template`
+// directive), issues in the build environment make that approach too
+// complicated to be worth it. Instead, use simple wrappers and make sure the
+// underlying containers are never exposed to C#.
+//
+// Note: most of the time, these classes should be declared with a `using`
+// statement to ensure predictable lifetime of the object when dealing with
+// iterators or unsafe views.
+
+// Wraps `MapFieldValue` for use in C# code.
+class FieldValueMap {
+ public:
+  FieldValueMap() = default;
+  explicit FieldValueMap(const FieldValue& value)
+      : container_(value.map_value()) {}
+
+  std::size_t Size() const { return container_.size(); }
+
+  // The returned reference is only valid as long as this `FieldValueMap` is
+  // valid. In C#, declare the map with a `using` statement to ensure its
+  // lifetime exceeds the lifetime of the reference.
+  const FieldValue& GetUnsafeView(const std::string& key) const {
+    auto found = container_.find(key);
+    if (found != container_.end()) {
+      return found->second;
+    }
+
+    return InvalidFieldValue();
+  }
+
+  FieldValue GetCopy(const std::string& key) const {
+    // Note: this is safe because the reference returned by `GetUnsafeView` is
+    // copied into the return value.
+    return GetUnsafeView(key);
+  }
+
+  void Insert(const std::string& key, const FieldValue& value) {
+    container_[key] = value;
+  }
+
+  class FieldValueMapIterator {
+   public:
+    bool HasMore() const { return cur_ != end_; }
+
+    void Advance() { ++cur_; }
+
+    const std::string& UnsafeKeyView() const { return cur_->first; }
+    const FieldValue& UnsafeValueView() const { return cur_->second; }
+
+    std::string KeyCopy() const { return cur_->first; }
+    FieldValue ValueCopy() const { return cur_->second; }
+
+   private:
+    friend class FieldValueMap;
+    explicit FieldValueMapIterator(const FieldValueMap& wrapper)
+        : cur_{wrapper.Unwrap().begin()}, end_{wrapper.Unwrap().end()} {}
+
+    MapFieldValue::const_iterator cur_, end_;
+  };
+
+  FieldValueMapIterator Iterator() const {
+    return FieldValueMapIterator(*this);
+  }
+
+  FieldValue ToFieldValue() const { return FieldValue::Map(Unwrap()); }
+
+  static FieldValue SnapshotToFieldValue(
+      const DocumentSnapshot& snapshot,
+      DocumentSnapshot::ServerTimestampBehavior stb) {
+    return FieldValue::Map(snapshot.GetData(stb));
+  }
+
+  static void TransactionUpdate(Transaction* transaction,
+                                const DocumentReference& doc,
+                                const FieldValue& field_value) {
+    transaction->Update(doc, field_value.map_value());
+  }
+
+  static void TransactionUpdate(Transaction* transaction,
+                                const DocumentReference& doc,
+                                const FieldValueMap& wrapper) {
+    transaction->Update(doc, wrapper.Unwrap());
+  }
+
+  static void WriteBatchUpdate(WriteBatch* batch, const DocumentReference& doc,
+                               const FieldValue& field_value) {
+    batch->Update(doc, field_value.map_value());
+  }
+
+  static void WriteBatchUpdate(WriteBatch* batch, const DocumentReference& doc,
+                               const FieldValueMap& wrapper) {
+    batch->Update(doc, wrapper.Unwrap());
+  }
+
+  static Future<void> DocumentReferenceSet(DocumentReference& doc,
+                                           const FieldValue& field_value,
+                                           const SetOptions& options) {
+    return doc.Set(field_value.map_value(), options);
+  }
+
+  static Future<void> DocumentReferenceUpdate(DocumentReference& doc,
+                                              const FieldValue& field_value) {
+    return doc.Update(field_value.map_value());
+  }
+
+ private:
+  const MapFieldValue& Unwrap() const { return container_; }
+
+  static const FieldValue& InvalidFieldValue() {
+    static FieldValue value;
+    return value;
+  }
+
+  MapFieldValue container_;
+};
+
+// Wraps `MapFieldPathValue` for use in C# code.
+// TODO(varconst): this should be a template instantiation, not a separate
+// class.
+class FieldPathValueMap {
+ public:
+  FieldPathValueMap() = default;
+
+  std::size_t Size() const { return container_.size(); }
+
+  // The returned reference is only valid as long as this `FieldPathValueMap` is
+  // valid. In C#, declare the map with a `using` statement to ensure its
+  // lifetime exceeds the lifetime of the reference.
+  const FieldValue& GetUnsafeView(const FieldPath& key) const {
+    auto found = container_.find(key);
+    if (found != container_.end()) {
+      return found->second;
+    }
+
+    return InvalidFieldValue();
+  }
+
+  FieldValue GetCopy(const FieldPath& key) const {
+    // Note: this is safe because the reference returned by `GetUnsafeView` is
+    // copied into the return value.
+    return GetUnsafeView(key);
+  }
+
+  void Insert(const FieldPath& key, const FieldValue& value) {
+    container_[key] = value;
+  }
+
+  class FieldPathValueMapIterator {
+   public:
+    bool HasMore() const { return cur_ != end_; }
+    void Advance() { ++cur_; }
+
+    const FieldPath& UnsafeKeyView() const { return cur_->first; }
+    const FieldValue& UnsafeValueView() const { return cur_->second; }
+
+    FieldPath KeyCopy() const { return cur_->first; }
+    FieldValue ValueCopy() const { return cur_->second; }
+
+   private:
+    friend class FieldPathValueMap;
+    explicit FieldPathValueMapIterator(const FieldPathValueMap& wrapper)
+        : cur_{wrapper.Unwrap().begin()}, end_{wrapper.Unwrap().end()} {}
+
+    MapFieldPathValue::const_iterator cur_, end_;
+  };
+
+  FieldPathValueMapIterator Iterator() const {
+    return FieldPathValueMapIterator(*this);
+  }
+
+  static void TransactionUpdate(Transaction* transaction,
+                                const DocumentReference& doc,
+                                const FieldPathValueMap& wrapper) {
+    transaction->Update(doc, wrapper.Unwrap());
+  }
+
+  static void WriteBatchUpdate(WriteBatch* batch, const DocumentReference& doc,
+                               const FieldPathValueMap& wrapper) {
+    batch->Update(doc, wrapper.Unwrap());
+  }
+
+  static Future<void> DocumentReferenceUpdate(
+      DocumentReference& doc, const FieldPathValueMap& wrapper) {
+    return doc.Update(wrapper.Unwrap());
+  }
+
+ private:
+  const MapFieldPathValue& Unwrap() const { return container_; }
+
+  static const FieldValue& InvalidFieldValue() {
+    static FieldValue value;
+    return value;
+  }
+
+  MapFieldPathValue container_;
+};
 
 }  // namespace csharp
 }  // namespace firestore
diff --git a/firestore/src/include/firebase/csharp/vector_helper.h b/firestore/src/include/firebase/csharp/vector_helper.h
@@ -11,36 +11,55 @@ namespace firebase {
 namespace firestore {
 namespace csharp {
 
-// FieldValue is converted to FieldValueInternal of internal access, which makes
-// it impossible to convert std::vector<FieldValue> with std_vector.i. Instead
-// of making a fully working std::vector<FieldValue>, we defined a few function
-// here.
+// Simple wrappers to avoid exposing C++ standard library containers to SWIG.
+//
+// While it's normally possible to work with standard library containers in SWIG
+// (by instantiating them for each template type used via the `%template`
+// directive), issues in the build environment make that approach too
+// complicated to be worth it. Instead, use simple wrappers and make sure the
+// underlying containers are never exposed to C#.
+//
+// Note: most of the time, these classes should be declared with a `using`
+// statement to ensure predictable lifetime of the object when dealing with
+// iterators or unsafe views.
 
-// We cannot call std::vector<FieldValue>::foo() in C# since it is not exposed
-// via the .dll interface as SWIG cannot convert it. Here we essentially define
-// a function, which SWIG can convert and thus expose it via the .dll. So C#
-// code can call it.
+// Wraps `std::vector<FieldValue>` for use in C# code.
+class FieldValueVector {
+ public:
+  FieldValueVector() = default;
 
-inline std::size_t vector_size(const std::vector<FieldValue>& self) {
-  return self.size();
-}
+  explicit FieldValueVector(const FieldValue& value)
+      : container_(value.array_value()) {}
 
-inline const FieldValue& vector_get(const std::vector<FieldValue>& self,
-                                       std::size_t index) {
-  return self[index];
-}
+  std::size_t Size() const { return container_.size(); }
+
+  // The returned reference is only valid as long as this `FieldValueVector` is
+  // valid. In C#, declare the vector with a `using` statement to ensure its
+  // lifetime exceeds the lifetime of the reference.
+  const FieldValue& GetUnsafeView(std::size_t i) const { return container_[i]; }
+
+  FieldValue GetCopy(std::size_t i) const { return container_[i]; }
 
-inline std::vector<FieldValue> vector_fv_create(std::size_t size) {
-  std::vector<FieldValue> result(size);
-  return result;
+  void PushBack(const FieldValue& value) {
+    container_.push_back(value);
+  }
+
+ private:
+  friend FieldValue ArrayToFieldValue(const FieldValueVector& wrapper);
+
+  const std::vector<FieldValue>& Unwrap() const { return container_; }
+
+  std::vector<FieldValue> container_;
+};
+
+FieldValue ArrayToFieldValue(const FieldValueVector& wrapper) {
+  return FieldValue::Array(wrapper.Unwrap());
 }
 
-// This is the only way to make it efficient for non-STLPort and also
-// STLPort-compatible.
-inline void vector_set(std::vector<FieldValue>* self, std::size_t index,
-                          FieldValue field_value) {
-  // This could be either a move-assignment or a normal assignment.
-  (*self)[index] = firebase::Move(field_value);
+// TODO(varconst): handle everything below in a similar way.
+
+inline std::size_t vector_size(const std::vector<FieldValue>& self) {
+  return self.size();
 }
 
 // Similarly, DocumentSnapshot is converted to DocumentSnapshotInternal and we
