Skip to content

Commit 316889b

Browse files
jedelbojedelbo
committed
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.
1 parent bf70a3f commit 316889b

File tree

6 files changed

+155
-28
lines changed

6 files changed

+155
-28
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
### Fixed
44
* Clearing a nested collection may end with a crash ([#7556](https://github.com/realm/realm-core/issues/7556), since v14.0.0)
5+
* Removing nested collections in Mixed for synced realms throws realm::StaleAccessor ([#7573](https://github.com/realm/realm-core/issues/7573), since v14.0.0)
56
* Add a privacy manifest to the Swift package ([Swift #8535](https://github.com/realm/realm-swift/issues/8535)).
67

78
### Compatibility

‎src/realm/dictionary.cpp

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,10 @@ Dictionary::~Dictionary() = default;
7777

7878
Dictionary& Dictionary::operator=(const Dictionary& other)
7979
{
80-
Base::operator=(static_cast<const Base&>(other));
81-
8280
if (this != &other) {
81+
Base::operator=(other);
82+
CollectionParent::operator=(other);
83+
8384
// Back to scratch
8485
m_dictionary_top.reset();
8586
reset_content_version();
@@ -437,24 +438,35 @@ void Dictionary::insert_collection(const PathElement& path_elem, CollectionType
437438
insert(path_elem.get_key(), Mixed(0, dict_or_list));
438439
}
439440

440-
DictionaryPtr Dictionary::get_dictionary(const PathElement& path_elem) const
441+
template <class T>
442+
inline std::shared_ptr<T> Dictionary::do_get_collection(const PathElement& path_elem)
441443
{
442444
update();
443-
auto weak = const_cast<Dictionary*>(this)->weak_from_this();
444-
auto shared = weak.expired() ? std::make_shared<Dictionary>(*this) : weak.lock();
445-
DictionaryPtr ret = std::make_shared<Dictionary>(m_col_key, get_level() + 1);
445+
auto get_shared = [&]() -> std::shared_ptr<CollectionParent> {
446+
auto weak = weak_from_this();
447+
448+
if (weak.expired()) {
449+
REALM_ASSERT_DEBUG(m_level == 1);
450+
return std::make_shared<Dictionary>(*this);
451+
}
452+
453+
return weak.lock();
454+
};
455+
456+
auto shared = get_shared();
457+
auto ret = std::make_shared<T>(m_col_key, get_level() + 1);
446458
ret->set_owner(shared, build_index(path_elem.get_key()));
447459
return ret;
448460
}
449461

462+
DictionaryPtr Dictionary::get_dictionary(const PathElement& path_elem) const
463+
{
464+
return const_cast<Dictionary*>(this)->do_get_collection<Dictionary>(path_elem);
465+
}
466+
450467
std::shared_ptr<Lst<Mixed>> Dictionary::get_list(const PathElement& path_elem) const
451468
{
452-
update();
453-
auto weak = const_cast<Dictionary*>(this)->weak_from_this();
454-
auto shared = weak.expired() ? std::make_shared<Dictionary>(*this) : weak.lock();
455-
std::shared_ptr<Lst<Mixed>> ret = std::make_shared<Lst<Mixed>>(m_col_key, get_level() + 1);
456-
ret->set_owner(shared, build_index(path_elem.get_key()));
457-
return ret;
469+
return const_cast<Dictionary*>(this)->do_get_collection<Lst<Mixed>>(path_elem);
458470
}
459471

460472
Mixed Dictionary::get(Mixed key) const
@@ -983,12 +995,12 @@ bool Dictionary::clear_backlink(size_t ndx, CascadeState& state) const
983995
return Base::remove_backlink(m_col_key, value.get_link(), state);
984996
}
985997
if (value.is_type(type_Dictionary)) {
986-
auto key = do_get_key(ndx);
987-
return get_dictionary(key.get_string())->remove_backlinks(state);
998+
Dictionary dict{*const_cast<Dictionary*>(this), m_values->get_key(ndx)};
999+
return dict.remove_backlinks(state);
9881000
}
9891001
if (value.is_type(type_List)) {
990-
auto key = do_get_key(ndx);
991-
return get_list(key.get_string())->remove_backlinks(state);
1002+
Lst<Mixed> list{*const_cast<Dictionary*>(this), m_values->get_key(ndx)};
1003+
return list.remove_backlinks(state);
9921004
}
9931005
return false;
9941006
}

‎src/realm/dictionary.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,8 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
277277
void get_key_type();
278278

279279
UpdateStatus do_update_if_needed(bool allow_create) const;
280+
template <class T>
281+
std::shared_ptr<T> do_get_collection(const PathElement& path_elem);
280282
};
281283

282284
class Dictionary::Iterator {

‎src/realm/list.cpp

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,37 @@ void Lst<ObjLink>::do_remove(size_t ndx)
347347

348348
/******************************** Lst<Mixed> *********************************/
349349

350+
Lst<Mixed>& Lst<Mixed>::operator=(const Lst<Mixed>& other)
351+
{
352+
if (this != &other) {
353+
Base::operator=(other);
354+
CollectionParent::operator=(other);
355+
356+
// Just reset the pointer and rely on init_from_parent() being called
357+
// when the accessor is actually used.
358+
m_tree.reset();
359+
Base::reset_content_version();
360+
}
361+
362+
return *this;
363+
}
364+
365+
Lst<Mixed>& Lst<Mixed>::operator=(Lst<Mixed>&& other) noexcept
366+
{
367+
if (this != &other) {
368+
Base::operator=(std::move(other));
369+
CollectionParent::operator=(std::move(other));
370+
371+
m_tree = std::exchange(other.m_tree, nullptr);
372+
if (m_tree) {
373+
m_tree->set_parent(this, 0);
374+
}
375+
}
376+
377+
return *this;
378+
}
379+
380+
350381
UpdateStatus Lst<Mixed>::init_from_parent(bool allow_create) const
351382
{
352383
Base::update_content_version();
@@ -550,24 +581,35 @@ void Lst<Mixed>::set_collection(const PathElement& path_elem, CollectionType dic
550581
set(path_elem.get_ndx(), Mixed(0, dict_or_list));
551582
}
552583

553-
DictionaryPtr Lst<Mixed>::get_dictionary(const PathElement& path_elem) const
584+
template <class T>
585+
inline std::shared_ptr<T> Lst<Mixed>::do_get_collection(const PathElement& path_elem)
554586
{
555587
update();
556-
auto weak = const_cast<Lst<Mixed>*>(this)->weak_from_this();
557-
auto shared = weak.expired() ? std::make_shared<Lst<Mixed>>(*this) : weak.lock();
558-
DictionaryPtr ret = std::make_shared<Dictionary>(m_col_key, get_level() + 1);
588+
auto get_shared = [&]() -> std::shared_ptr<CollectionParent> {
589+
auto weak = weak_from_this();
590+
591+
if (weak.expired()) {
592+
REALM_ASSERT_DEBUG(m_level == 1);
593+
return std::make_shared<Lst<Mixed>>(*this);
594+
}
595+
596+
return weak.lock();
597+
};
598+
599+
auto shared = get_shared();
600+
auto ret = std::make_shared<T>(m_col_key, get_level() + 1);
559601
ret->set_owner(shared, m_tree->get_key(path_elem.get_ndx()));
560602
return ret;
561603
}
562604

605+
DictionaryPtr Lst<Mixed>::get_dictionary(const PathElement& path_elem) const
606+
{
607+
return const_cast<Lst<Mixed>*>(this)->do_get_collection<Dictionary>(path_elem);
608+
}
609+
563610
std::shared_ptr<Lst<Mixed>> Lst<Mixed>::get_list(const PathElement& path_elem) const
564611
{
565-
update();
566-
auto weak = const_cast<Lst<Mixed>*>(this)->weak_from_this();
567-
auto shared = weak.expired() ? std::make_shared<Lst<Mixed>>(*this) : weak.lock();
568-
std::shared_ptr<Lst<Mixed>> ret = std::make_shared<Lst<Mixed>>(m_col_key, get_level() + 1);
569-
ret->set_owner(shared, m_tree->get_key(path_elem.get_ndx()));
570-
return ret;
612+
return const_cast<Lst<Mixed>*>(this)->do_get_collection<Lst<Mixed>>(path_elem);
571613
}
572614

573615
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
838880
return Base::remove_backlink(m_col_key, link, state);
839881
}
840882
else if (value.is_type(type_List)) {
841-
return get_list(ndx)->remove_backlinks(state);
883+
Lst<Mixed> list{*const_cast<Lst<Mixed>*>(this), m_tree->get_key(ndx)};
884+
return list.remove_backlinks(state);
842885
}
843886
else if (value.is_type(type_Dictionary)) {
844-
return get_dictionary(ndx)->remove_backlinks(state);
887+
Dictionary dict{*const_cast<Lst<Mixed>*>(this), m_tree->get_key(ndx)};
888+
return dict.remove_backlinks(state);
845889
}
846890
}
847891
return false;

‎src/realm/list.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,8 @@ class Lst<Mixed> final : public CollectionBaseImpl<LstBase>, public CollectionPa
541541
return unresolved_to_null(m_tree->get(ndx));
542542
}
543543
bool clear_backlink(size_t ndx, CascadeState& state) const;
544+
template <class T>
545+
inline std::shared_ptr<T> do_get_collection(const PathElement& path_elem);
544546
};
545547

546548
// Specialization of Lst<StringData>:

‎test/test_sync.cpp

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6314,6 +6314,72 @@ TEST(Sync_CollectionInCollection)
63146314
}
63156315
}
63166316

6317+
TEST(Sync_DeleteCollectionInCollection)
6318+
{
6319+
TEST_CLIENT_DB(db_1);
6320+
TEST_CLIENT_DB(db_2);
6321+
6322+
TEST_DIR(dir);
6323+
fixtures::ClientServerFixture fixture{dir, test_context};
6324+
fixture.start();
6325+
6326+
Session session_1 = fixture.make_session(db_1, "/test");
6327+
Session session_2 = fixture.make_session(db_2, "/test");
6328+
session_1.bind();
6329+
session_2.bind();
6330+
6331+
Timestamp now{std::chrono::system_clock::now()};
6332+
6333+
write_transaction(db_1, [&](WriteTransaction& tr) {
6334+
auto& g = tr.get_group();
6335+
auto table = g.add_table_with_primary_key("class_Table", type_Int, "id");
6336+
auto col_any = table->add_column(type_Mixed, "any");
6337+
6338+
auto foo = table->create_object_with_primary_key(123);
6339+
6340+
// Create list in Mixed property
6341+
foo.set_json(col_any, R"([
6342+
{
6343+
"1_map": {
6344+
"2_string": "map value"
6345+
},
6346+
"1_list": ["list value"]
6347+
}
6348+
])");
6349+
});
6350+
6351+
session_1.wait_for_upload_complete_or_client_stopped();
6352+
session_2.wait_for_download_complete_or_client_stopped();
6353+
6354+
write_transaction(db_2, [&](WriteTransaction& tr) {
6355+
auto table = tr.get_table("class_Table");
6356+
auto col_any = table->get_column_key("any");
6357+
CHECK_EQUAL(table->size(), 1);
6358+
6359+
auto obj = table->get_object_with_primary_key(123);
6360+
auto list = obj.get_list<Mixed>(col_any);
6361+
auto dict = list.get_dictionary(0);
6362+
dict->erase("1_map");
6363+
});
6364+
6365+
session_2.wait_for_upload_complete_or_client_stopped();
6366+
session_1.wait_for_download_complete_or_client_stopped();
6367+
6368+
{
6369+
ReadTransaction read_1{db_1};
6370+
6371+
auto table = read_1.get_table("class_Table");
6372+
auto col_any = table->get_column_key("any");
6373+
6374+
CHECK_EQUAL(table->size(), 1);
6375+
6376+
auto obj = table->get_object_with_primary_key(123);
6377+
auto list = obj.get_list<Mixed>(col_any);
6378+
auto dict = list.get_dictionary(0);
6379+
CHECK_EQUAL(dict->size(), 1);
6380+
}
6381+
}
6382+
63176383
TEST(Sync_Dictionary_Links)
63186384
{
63196385
TEST_CLIENT_DB(db_1);

0 commit comments

Comments
 (0)