Merge pull request #77 from realm/mar/results-leak

Fix move-assigning to a Results that has a notifier to not leak the Realm

src/impl/collection_notifier.hpp

@@ -150,6 +150,9 @@ public:
     void prepare_handover();
     bool deliver(Realm&, SharedGroup&, std::exception_ptr);
 
+    template <typename T>
+    class Handle;
+
 protected:
     bool have_callbacks() const noexcept { return m_have_callbacks; }
     void add_changes(CollectionChangeBuilder change) { m_accumulated_changes.merge(std::move(change)); }
@@ -201,6 +204,43 @@ private:
     CollectionChangeCallback next_callback();
 };
 
+// A smart pointer to a CollectionNotifier that unregisters the notifier when
+// the pointer is destroyed. Movable. Copying will produce a null Handle.
+template <typename T>
+class CollectionNotifier::Handle : public std::shared_ptr<T> {
+public:
+    using std::shared_ptr<T>::shared_ptr;
+
+    Handle() = default;
+    ~Handle() { reset(); }
+
+    // Copying a Handle produces a null Handle.
+    Handle(const Handle&) : Handle() { }
+    Handle& operator=(const Handle& other)
+    {
+        if (this != &other) {
+            reset();
+        }
+        return *this;
+    }
+
+    Handle(Handle&&) = default;
+    Handle& operator=(Handle&& other)
+    {
+        reset();
+        std::shared_ptr<T>::shared_ptr::operator=(std::move(other));
+        return *this;
+    }
+
+    void reset()
+    {
+        if (*this) {
+            this->get()->unregister();
+            std::shared_ptr<T>::reset();
+        }
+    }
+};
+
 } // namespace _impl
 } // namespace realm
 

src/impl/results_notifier.cpp

@@ -18,8 +18,6 @@
 
 #include "impl/results_notifier.hpp"
 
-#include "results.hpp"
-
 using namespace realm;
 using namespace realm::_impl;
 
@@ -34,6 +32,14 @@ ResultsNotifier::ResultsNotifier(Results& target)
     m_query_handover = Realm::Internal::get_shared_group(*get_realm()).export_for_handover(q, MutableSourcePayload::Move);
 }
 
+void ResultsNotifier::target_results_moved(Results& old_target, Results& new_target)
+{
+    auto lock = lock_target();
+
+    REALM_ASSERT(m_target_results == &old_target);
+    m_target_results = &new_target;
+}
+
 void ResultsNotifier::release_data() noexcept
 {
     m_query = nullptr;

src/impl/results_notifier.hpp

@@ -30,6 +30,8 @@ class ResultsNotifier : public CollectionNotifier {
 public:
     ResultsNotifier(Results& target);
 
+    void target_results_moved(Results& old_target, Results& new_target);
+
 private:
     // Target Results to update
     // Can only be used with lock_target() held

src/list.cpp

@@ -30,12 +30,12 @@ using namespace realm;
 using namespace realm::_impl;
 
 List::List() noexcept = default;
-List::~List()
+List::~List() = default;
-{
+
-    if (m_notifier) {
+List::List(const List&) = default;
-        m_notifier->unregister();
+List& List::operator=(const List&) = default;
-    }
+List::List(List&&) = default;
-}
+List& List::operator=(List&&) = default;
 
 List::List(std::shared_ptr<Realm> r, const ObjectSchema& s, LinkViewRef l) noexcept
 : m_realm(std::move(r))

src/list.hpp

@@ -20,6 +20,7 @@
 #define REALM_LIST_HPP
 
 #include "collection_notifications.hpp"
+#include "impl/collection_notifier.hpp"
 
 #include <realm/link_view_fwd.hpp>
 #include <realm/row.hpp>
@@ -46,6 +47,11 @@ public:
     List(std::shared_ptr<Realm> r, const ObjectSchema& s, LinkViewRef l) noexcept;
     ~List();
 
+    List(const List&);
+    List& operator=(const List&);
+    List(List&&);
+    List& operator=(List&&);
+
     const std::shared_ptr<Realm>& get_realm() const { return m_realm; }
     Query get_query() const;
     const ObjectSchema& get_object_schema() const { return *m_object_schema; }
@@ -89,7 +95,7 @@ private:
     std::shared_ptr<Realm> m_realm;
     const ObjectSchema* m_object_schema;
     LinkViewRef m_link_view;
-    std::shared_ptr<_impl::CollectionNotifier> m_notifier;
+    _impl::CollectionNotifier::Handle<_impl::CollectionNotifier> m_notifier;
 
     void verify_valid_row(size_t row_ndx, bool insertion = false) const;
 

src/results.cpp

@@ -38,6 +38,9 @@ using namespace realm;
 #define REALM_FALLTHROUGH
 #endif
 
+Results::Results() = default;
+Results::~Results() = default;
+
 Results::Results(SharedRealm r, const ObjectSchema &o, Query q, SortOrder s)
 : m_realm(std::move(r))
 , m_object_schema(&o)
@@ -83,13 +86,45 @@ Results::Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s
     REALM_ASSERT(m_sort.column_indices.size() == m_sort.ascending.size());
 }
 
-Results::~Results()
+Results::Results(const Results& other) = default;
+
+// Cannot be defaulted as TableViewBase::operator= is missing from the core static library.
+// Delegate to the copy constructor and move-assignment operators instead.
+Results& Results::operator=(const Results& other)
+{
+    if (this != &other) {
+        *this = Results(other);
+    }
+
+    return *this;
+}
+
+Results::Results(Results&& other)
+: m_realm(std::move(other.m_realm))
+, m_object_schema(std::move(other.m_object_schema))
+, m_query(std::move(other.m_query))
+, m_table_view(std::move(other.m_table_view))
+, m_link_view(std::move(other.m_link_view))
+, m_table(other.m_table)
+, m_sort(std::move(other.m_sort))
+, m_live(other.m_live)
+, m_notifier(std::move(other.m_notifier))
+, m_mode(other.m_mode)
+, m_has_used_table_view(other.m_has_used_table_view)
+, m_wants_background_updates(other.m_wants_background_updates)
 {
     if (m_notifier) {
-        m_notifier->unregister();
+        m_notifier->target_results_moved(other, *this);
     }
 }
 
+Results& Results::operator=(Results&& other)
+{
+    this->~Results();
+    new (this) Results(std::move(other));
+    return *this;
+}
+
 void Results::validate_read() const
 {
     if (m_realm)
@@ -460,6 +495,7 @@ void Results::prepare_async()
     }
 
     if (!m_notifier) {
+        m_wants_background_updates = true;
         m_notifier = std::make_shared<_impl::ResultsNotifier>(*this);
         _impl::RealmCoordinator::register_notifier(m_notifier);
     }

src/results.hpp

@@ -21,6 +21,7 @@
 
 #include "collection_notifications.hpp"
 #include "shared_realm.hpp"
+#include "impl/collection_notifier.hpp"
 
 #include <realm/table_view.hpp>
 #include <realm/util/optional.hpp>
@@ -48,7 +49,7 @@ public:
     // Results can be either be backed by nothing, a thin wrapper around a table,
     // or a wrapper around a query and a sort order which creates and updates
     // the tableview as needed
-    Results() = default;
+    Results();
     Results(SharedRealm r, const ObjectSchema& o, Table& table);
     Results(SharedRealm r, const ObjectSchema& o, Query q, SortOrder s = {});
     Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s);
@@ -56,10 +57,10 @@ public:
     ~Results();
 
     // Results is copyable and moveable
-    Results(Results const&) = default;
+    Results(Results&&);
-    Results(Results&&) = default;
+    Results& operator=(Results&&);
-    Results& operator=(Results&&) = default;
+    Results(const Results&);
-    Results& operator=(Results const&) = default;
+    Results& operator=(const Results&);
 
     // Get the Realm
     SharedRealm get_realm() const { return m_realm; }
@@ -203,7 +204,7 @@ private:
     SortOrder m_sort;
     bool m_live = true;
 
-    std::shared_ptr<_impl::ResultsNotifier> m_notifier;
+    _impl::CollectionNotifier::Handle<_impl::ResultsNotifier> m_notifier;
 
     Mode m_mode = Mode::Empty;
     bool m_has_used_table_view = false;

tests/results.cpp

@@ -533,3 +533,54 @@ TEST_CASE("Async Results error handling") {
         }
     }
 }
+
+TEST_CASE("Notifications on moved Results") {
+    InMemoryTestFile config;
+    config.cache = false;
+    config.automatic_change_notifications = false;
+    config.schema = std::make_unique<Schema>(Schema{
+        {"object", "", {
+            {"value", PropertyType::Int},
+        }},
+    });
+
+    auto r = Realm::get_shared_realm(config);
+    auto table = r->read_group()->get_table("class_object");
+    auto results = std::make_unique<Results>(r, *config.schema->find("object"), *table);
+
+    int notification_calls = 0;
+    auto token = results->add_notification_callback([&](CollectionChangeSet c, std::exception_ptr err) {
+        REQUIRE_FALSE(err);
+        ++notification_calls;
+    });
+
+    advance_and_notify(*r);
+
+    auto write = [&](auto&& f) {
+        r->begin_transaction();
+        f();
+        r->commit_transaction();
+        advance_and_notify(*r);
+    };
+
+    SECTION("notifications continue to work after Results is moved (move-constructor)") {
+        Results r(std::move(*results));
+        results.reset();
+
+        write([&] {
+            table->set_int(0, table->add_empty_row(), 1);
+        });
+        REQUIRE(notification_calls == 2);
+    }
+
+    SECTION("notifications continue to work after Results is moved (move-assignment)") {
+        Results r;
+        r = std::move(*results);
+        results.reset();
+
+        write([&] {
+            table->set_int(0, table->add_empty_row(), 1);
+        });
+        REQUIRE(notification_calls == 2);
+    }
+}