Fix a data race in notifier packaging (#5892)

CollectionNotifier::version() requires holding
RealmCoordinator::m_notifier_mutex, which was being violated in a few places.

CHANGELOG.md

@@ -7,8 +7,8 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
- 
+* Fix a data race reported by thread sanitizer when preparing to deliver change notifications. This probably did not cause observable problems in practice ([PR #5892](https://github.com/realm/realm-core/pull/5892) since 12.7.0).
+
 ### Breaking changes
 * None.
 

src/realm/object-store/impl/collection_notifier.cpp

@@ -413,28 +413,23 @@ NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>
 {
 }
 
-NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers)
- : m_notifiers(std::move(notifiers))
+NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers,
+ std::optional<VersionID> version)
+ : m_version(version)
+ , m_notifiers(std::move(notifiers))
 {
- set_version();
 }
 
 void NotifierPackage::package_and_wait(VersionID::version_type target_version)
 {
  if (!m_coordinator || !*this)
  return;
 
- m_coordinator->package_notifiers(m_notifiers, target_version);
- set_version();
+ m_version = m_coordinator->package_notifiers(m_notifiers, target_version);
  if (m_version && m_version->version < target_version)
  m_version = util::none;
 }
 
-void NotifierPackage::set_version()
-{
- m_version = m_notifiers.empty() ? util::none : std::make_optional(m_notifiers.front()->version());
-}
-
 void NotifierPackage::before_advance()
 {
  for (auto& notifier : m_notifiers)

src/realm/object-store/impl/collection_notifier.hpp

@@ -330,7 +330,7 @@ class NotifierPackage {
  NotifierPackage() = default;
 
  // Create a package which contains notifiers which have already been pacakged for delivery
- NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers);
+ NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers, std::optional<VersionID> version);
  // Create a package which can have package_and_wait() called on it later
  NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers, RealmCoordinator* coordinator);
 
@@ -362,8 +362,6 @@ class NotifierPackage {
  util::Optional<VersionID> m_version;
  std::vector<std::shared_ptr<CollectionNotifier>> m_notifiers;
  RealmCoordinator* m_coordinator = nullptr;
-
- void set_version();
 };
 
 } // namespace realm::_impl

src/realm/object-store/impl/realm_coordinator.cpp

@@ -1172,7 +1172,8 @@ void RealmCoordinator::advance_to_ready(Realm& realm)
  }
 
  // We have notifiers for a newer version, so advance to that
- transaction::advance(tr, realm.m_binding_context.get(), std::move(notifiers));
+ transaction::advance(tr, realm.m_binding_context.get(),
+ _impl::NotifierPackage(std::move(notifiers), notifier_version));
 }
 
 std::vector<std::shared_ptr<_impl::CollectionNotifier>> RealmCoordinator::notifiers_for_realm(Realm& realm)
@@ -1198,11 +1199,11 @@ bool RealmCoordinator::advance_to_latest(Realm& realm)
  util::CheckedUniqueLock lock(m_notifier_mutex);
  notifiers = notifiers_for_realm(realm);
  }
- package_notifiers(notifiers, m_db->get_version_of_latest_snapshot());
+ auto version = package_notifiers(notifiers, m_db->get_version_of_latest_snapshot());
 
- auto version = tr->get_version_of_current_transaction();
- transaction::advance(tr, realm.m_binding_context.get(), _impl::NotifierPackage(std::move(notifiers)));
- return !realm.is_closed() && version != tr->get_version_of_current_transaction();
+ auto prev_version = tr->get_version_of_current_transaction();
+ transaction::advance(tr, realm.m_binding_context.get(), _impl::NotifierPackage(std::move(notifiers), version));
+ return !realm.is_closed() && prev_version != tr->get_version_of_current_transaction();
 }
 
 void RealmCoordinator::promote_to_write(Realm& realm)
@@ -1257,7 +1258,8 @@ void RealmCoordinator::process_available_async(Realm& realm)
  realm.m_binding_context->did_send_notifications();
 }
 
-void RealmCoordinator::package_notifiers(NotifierVector& notifiers, VersionID::version_type target_version)
+std::optional<VersionID> RealmCoordinator::package_notifiers(NotifierVector& notifiers,
+ VersionID::version_type target_version)
 {
  auto ready = [&] {
  util::CheckedUniqueLock notifier_lock(m_notifier_mutex);
@@ -1280,6 +1282,7 @@ void RealmCoordinator::package_notifiers(NotifierVector& notifiers, VersionID::v
  notifier->version().version < target_version;
  };
  notifiers.erase(std::remove_if(begin(notifiers), end(notifiers), package), end(notifiers));
+ return notifiers.empty() ? std::optional<VersionID>{} : notifiers.front()->version();
 }
 
 bool RealmCoordinator::compact()

src/realm/object-store/impl/realm_coordinator.hpp

@@ -207,7 +207,7 @@ class RealmCoordinator : public std::enable_shared_from_this<RealmCoordinator> {
  // Called by NotifierPackage in the cases where we don't know what version
  // we need notifiers for until after we begin advancing (e.g. when
  // starting a write transaction).
- void package_notifiers(NotifierVector& notifiers, VersionID::version_type)
+ std::optional<VersionID> package_notifiers(NotifierVector& notifiers, VersionID::version_type)
  REQUIRES(!m_notifier_mutex, !m_running_notifiers_mutex);
 
  // testing hook only to verify that notifiers are not being run at times