Use proper constructor when clearing links is nested collections (#7578)

It is only safe to use get_dictionary/get_list on a collection that is either at level 1 or if referenced by a shared pointer. The collection created in instruction_applier is always stack allocated and can be at any level.
realm · Apr 12, 2024 · 316889b · 316889b
1 parent bf70a3f
commit 316889b
Show file tree

Hide file tree

Showing 6 changed files with 155 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ### Fixed
 * Clearing a nested collection may end with a crash ([#7556](https://github.com/realm/realm-core/issues/7556), since v14.0.0)
+* Removing nested collections in Mixed for synced realms throws realm::StaleAccessor ([#7573](https://github.com/realm/realm-core/issues/7573), since v14.0.0)
 * Add a privacy manifest to the Swift package ([Swift #8535](https://github.com/realm/realm-swift/issues/8535)).
 
 ### Compatibility

diff --git a/src/realm/dictionary.cpp b/src/realm/dictionary.cpp
@@ -77,9 +77,10 @@ Dictionary::~Dictionary() = default;
 
 Dictionary& Dictionary::operator=(const Dictionary& other)
 {
-    Base::operator=(static_cast<const Base&>(other));
-
     if (this != &other) {
+        Base::operator=(other);
+        CollectionParent::operator=(other);
+
         // Back to scratch
         m_dictionary_top.reset();
         reset_content_version();
@@ -437,24 +438,35 @@ void Dictionary::insert_collection(const PathElement& path_elem, CollectionType
     insert(path_elem.get_key(), Mixed(0, dict_or_list));
 }
 
-DictionaryPtr Dictionary::get_dictionary(const PathElement& path_elem) const
+template <class T>
+inline std::shared_ptr<T> Dictionary::do_get_collection(const PathElement& path_elem)
 {
     update();
-    auto weak = const_cast<Dictionary*>(this)->weak_from_this();
-    auto shared = weak.expired() ? std::make_shared<Dictionary>(*this) : weak.lock();
-    DictionaryPtr ret = std::make_shared<Dictionary>(m_col_key, get_level() + 1);
+    auto get_shared = [&]() -> std::shared_ptr<CollectionParent> {
+        auto weak = weak_from_this();
+
+        if (weak.expired()) {
+            REALM_ASSERT_DEBUG(m_level == 1);
+            return std::make_shared<Dictionary>(*this);
+        }
+
+        return weak.lock();
+    };
+
+    auto shared = get_shared();
+    auto ret = std::make_shared<T>(m_col_key, get_level() + 1);
     ret->set_owner(shared, build_index(path_elem.get_key()));
     return ret;
 }
 
+DictionaryPtr Dictionary::get_dictionary(const PathElement& path_elem) const
+{
+    return const_cast<Dictionary*>(this)->do_get_collection<Dictionary>(path_elem);
+}
+
 std::shared_ptr<Lst<Mixed>> Dictionary::get_list(const PathElement& path_elem) const
 {
-    update();
-    auto weak = const_cast<Dictionary*>(this)->weak_from_this();
-    auto shared = weak.expired() ? std::make_shared<Dictionary>(*this) : weak.lock();
-    std::shared_ptr<Lst<Mixed>> ret = std::make_shared<Lst<Mixed>>(m_col_key, get_level() + 1);
-    ret->set_owner(shared, build_index(path_elem.get_key()));
-    return ret;
+    return const_cast<Dictionary*>(this)->do_get_collection<Lst<Mixed>>(path_elem);
 }
 
 Mixed Dictionary::get(Mixed key) const
@@ -983,12 +995,12 @@ bool Dictionary::clear_backlink(size_t ndx, CascadeState& state) const
         return Base::remove_backlink(m_col_key, value.get_link(), state);
     }
     if (value.is_type(type_Dictionary)) {
-        auto key = do_get_key(ndx);
-        return get_dictionary(key.get_string())->remove_backlinks(state);
+        Dictionary dict{*const_cast<Dictionary*>(this), m_values->get_key(ndx)};
+        return dict.remove_backlinks(state);
     }
     if (value.is_type(type_List)) {
-        auto key = do_get_key(ndx);
-        return get_list(key.get_string())->remove_backlinks(state);
+        Lst<Mixed> list{*const_cast<Dictionary*>(this), m_values->get_key(ndx)};
+        return list.remove_backlinks(state);
     }
     return false;
 }

diff --git a/src/realm/dictionary.hpp b/src/realm/dictionary.hpp
@@ -277,6 +277,8 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
     void get_key_type();
 
     UpdateStatus do_update_if_needed(bool allow_create) const;
+    template <class T>
+    std::shared_ptr<T> do_get_collection(const PathElement& path_elem);
 };
 
 class Dictionary::Iterator {

diff --git a/src/realm/list.cpp b/src/realm/list.cpp
@@ -347,6 +347,37 @@ void Lst<ObjLink>::do_remove(size_t ndx)
 
 /******************************** Lst<Mixed> *********************************/
 
+Lst<Mixed>& Lst<Mixed>::operator=(const Lst<Mixed>& other)
+{
+    if (this != &other) {
+        Base::operator=(other);
+        CollectionParent::operator=(other);
+
+        // Just reset the pointer and rely on init_from_parent() being called
+        // when the accessor is actually used.
+        m_tree.reset();
+        Base::reset_content_version();
+    }
+
+    return *this;
+}
+
+Lst<Mixed>& Lst<Mixed>::operator=(Lst<Mixed>&& other) noexcept
+{
+    if (this != &other) {
+        Base::operator=(std::move(other));
+        CollectionParent::operator=(std::move(other));
+
+        m_tree = std::exchange(other.m_tree, nullptr);
+        if (m_tree) {
+            m_tree->set_parent(this, 0);
+        }
+    }
+
+    return *this;
+}
+
+
 UpdateStatus Lst<Mixed>::init_from_parent(bool allow_create) const
 {
     Base::update_content_version();
@@ -550,24 +581,35 @@ void Lst<Mixed>::set_collection(const PathElement& path_elem, CollectionType dic
     set(path_elem.get_ndx(), Mixed(0, dict_or_list));
 }
 
-DictionaryPtr Lst<Mixed>::get_dictionary(const PathElement& path_elem) const
+template <class T>
+inline std::shared_ptr<T> Lst<Mixed>::do_get_collection(const PathElement& path_elem)
 {
     update();
-    auto weak = const_cast<Lst<Mixed>*>(this)->weak_from_this();
-    auto shared = weak.expired() ? std::make_shared<Lst<Mixed>>(*this) : weak.lock();
-    DictionaryPtr ret = std::make_shared<Dictionary>(m_col_key, get_level() + 1);
+    auto get_shared = [&]() -> std::shared_ptr<CollectionParent> {
+        auto weak = weak_from_this();
+
+        if (weak.expired()) {
+            REALM_ASSERT_DEBUG(m_level == 1);
+            return std::make_shared<Lst<Mixed>>(*this);
+        }
+
+        return weak.lock();
+    };
+
+    auto shared = get_shared();
+    auto ret = std::make_shared<T>(m_col_key, get_level() + 1);
     ret->set_owner(shared, m_tree->get_key(path_elem.get_ndx()));
     return ret;
 }
 
+DictionaryPtr Lst<Mixed>::get_dictionary(const PathElement& path_elem) const
+{
+    return const_cast<Lst<Mixed>*>(this)->do_get_collection<Dictionary>(path_elem);
+}
+
 std::shared_ptr<Lst<Mixed>> Lst<Mixed>::get_list(const PathElement& path_elem) const
 {
-    update();
-    auto weak = const_cast<Lst<Mixed>*>(this)->weak_from_this();
-    auto shared = weak.expired() ? std::make_shared<Lst<Mixed>>(*this) : weak.lock();
-    std::shared_ptr<Lst<Mixed>> ret = std::make_shared<Lst<Mixed>>(m_col_key, get_level() + 1);
-    ret->set_owner(shared, m_tree->get_key(path_elem.get_ndx()));
-    return ret;
+    return const_cast<Lst<Mixed>*>(this)->do_get_collection<Lst<Mixed>>(path_elem);
 }
 
 void Lst<Mixed>::do_set(size_t ndx, Mixed value)
@@ -838,10 +880,12 @@ bool Lst<Mixed>::clear_backlink(size_t ndx, CascadeState& state) const
             return Base::remove_backlink(m_col_key, link, state);
         }
         else if (value.is_type(type_List)) {
-            return get_list(ndx)->remove_backlinks(state);
+            Lst<Mixed> list{*const_cast<Lst<Mixed>*>(this), m_tree->get_key(ndx)};
+            return list.remove_backlinks(state);
         }
         else if (value.is_type(type_Dictionary)) {
-            return get_dictionary(ndx)->remove_backlinks(state);
+            Dictionary dict{*const_cast<Lst<Mixed>*>(this), m_tree->get_key(ndx)};
+            return dict.remove_backlinks(state);
         }
     }
     return false;

diff --git a/src/realm/list.hpp b/src/realm/list.hpp
@@ -541,6 +541,8 @@ class Lst<Mixed> final : public CollectionBaseImpl<LstBase>, public CollectionPa
         return unresolved_to_null(m_tree->get(ndx));
     }
     bool clear_backlink(size_t ndx, CascadeState& state) const;
+    template <class T>
+    inline std::shared_ptr<T> do_get_collection(const PathElement& path_elem);
 };
 
 // Specialization of Lst<StringData>:

diff --git a/test/test_sync.cpp b/test/test_sync.cpp
@@ -6314,6 +6314,72 @@ TEST(Sync_CollectionInCollection)
     }
 }
 
+TEST(Sync_DeleteCollectionInCollection)
+{
+    TEST_CLIENT_DB(db_1);
+    TEST_CLIENT_DB(db_2);
+
+    TEST_DIR(dir);
+    fixtures::ClientServerFixture fixture{dir, test_context};
+    fixture.start();
+
+    Session session_1 = fixture.make_session(db_1, "/test");
+    Session session_2 = fixture.make_session(db_2, "/test");
+    session_1.bind();
+    session_2.bind();
+
+    Timestamp now{std::chrono::system_clock::now()};
+
+    write_transaction(db_1, [&](WriteTransaction& tr) {
+        auto& g = tr.get_group();
+        auto table = g.add_table_with_primary_key("class_Table", type_Int, "id");
+        auto col_any = table->add_column(type_Mixed, "any");
+
+        auto foo = table->create_object_with_primary_key(123);
+
+        // Create list in Mixed property
+        foo.set_json(col_any, R"([
+            {
+              "1_map": {
+                "2_string": "map value"
+               },
+              "1_list": ["list value"]
+            }
+        ])");
+    });
+
+    session_1.wait_for_upload_complete_or_client_stopped();
+    session_2.wait_for_download_complete_or_client_stopped();
+
+    write_transaction(db_2, [&](WriteTransaction& tr) {
+        auto table = tr.get_table("class_Table");
+        auto col_any = table->get_column_key("any");
+        CHECK_EQUAL(table->size(), 1);
+
+        auto obj = table->get_object_with_primary_key(123);
+        auto list = obj.get_list<Mixed>(col_any);
+        auto dict = list.get_dictionary(0);
+        dict->erase("1_map");
+    });
+
+    session_2.wait_for_upload_complete_or_client_stopped();
+    session_1.wait_for_download_complete_or_client_stopped();
+
+    {
+        ReadTransaction read_1{db_1};
+
+        auto table = read_1.get_table("class_Table");
+        auto col_any = table->get_column_key("any");
+
+        CHECK_EQUAL(table->size(), 1);
+
+        auto obj = table->get_object_with_primary_key(123);
+        auto list = obj.get_list<Mixed>(col_any);
+        auto dict = list.get_dictionary(0);
+        CHECK_EQUAL(dict->size(), 1);
+    }
+}
+
 TEST(Sync_Dictionary_Links)
 {
     TEST_CLIENT_DB(db_1);