Fix an invalid use of frozen Realms in client reset

CHANGELOG.md

@@ -31,6 +31,7 @@
 * Make log level threshold atomic and shared ([#6009](https://github.com/realm/realm-core/issues/6009))
 * Add c_api error category for resolve errors instead of reporting unknown category, part 2. ([PR #6186](https://github.com/realm/realm-core/pull/6186))
 * Remove `File::is_removed` ([#6222](https://github.com/realm/realm-core/pull/6222))
+* Client reset recovery froze Realms for the callbacks in an invalid way. It is unclear if this resulted in any actual problems.
 
 ----------------------------------------------
 

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

@@ -238,6 +238,7 @@ std::shared_ptr<Realm> RealmCoordinator::do_get_cached_realm(Realm::Config const
 
 std::shared_ptr<Realm> RealmCoordinator::get_realm(Realm::Config config, util::Optional<VersionID> version)
 {
+    REALM_ASSERT(!version || *version != VersionID());
     if (!config.scheduler)
         config.scheduler = version ? util::Scheduler::make_frozen(*version) : util::Scheduler::make_default();
     // realm must be declared before lock so that the mutex is released before

src/realm/object-store/shared_realm.cpp

@@ -82,6 +82,7 @@ Realm::Realm(Config config, util::Optional<VersionID> version, std::shared_ptr<_
 {
     if (version) {
         m_auto_refresh = false;
+        REALM_ASSERT(*version != VersionID());
     }
     else if (!coordinator->get_cached_schema(m_schema, m_schema_version, m_schema_transaction_version)) {
         m_transaction = coordinator->begin_read();
@@ -1327,7 +1328,7 @@ void Realm::copy_schema_from(const Realm& source)
     REALM_ASSERT(m_frozen_version == source.read_transaction_version());
     m_schema = source.m_schema;
     m_schema_version = source.m_schema_version;
-    m_schema_transaction_version = source.m_schema_transaction_version;
+    m_schema_transaction_version = m_frozen_version->version;
     m_dynamic_schema = false;
 }
 

src/realm/object-store/sync/sync_session.cpp

@@ -730,45 +730,36 @@ void SyncSession::handle_progress_update(uint64_t downloaded, uint64_t downloada
     m_progress_notifier.update(downloaded, downloadable, uploaded, uploadable, download_version, snapshot_version);
 }
 
-static sync::Session::Config::ClientReset make_client_reset_config(RealmConfig& session_config, DBRef&& fresh_copy,
+static sync::Session::Config::ClientReset make_client_reset_config(RealmConfig session_config, DBRef&& fresh_copy,
                                                                    bool recovery_is_allowed)
 {
-    RealmConfig copy_config = session_config;
-    copy_config.sync_config = std::make_shared<SyncConfig>(*session_config.sync_config); // deep copy
-    sync::Session::Config::ClientReset config;
     REALM_ASSERT(session_config.sync_config->client_resync_mode != ClientResyncMode::Manual);
+
+    sync::Session::Config::ClientReset config;
     config.mode = session_config.sync_config->client_resync_mode;
-    if (copy_config.sync_config->notify_after_client_reset) {
-        config.notify_after_client_reset = [config = copy_config](std::string local_path, VersionID previous_version,
-                                                                  bool did_recover) {
-            REALM_ASSERT(local_path == config.path);
+    config.fresh_copy = std::move(fresh_copy);
+    config.recovery_is_allowed = recovery_is_allowed;
+
+    session_config.sync_config = std::make_shared<SyncConfig>(*session_config.sync_config); // deep copy
+    session_config.scheduler = nullptr;
+    if (session_config.sync_config->notify_after_client_reset) {
+        config.notify_after_client_reset = [config = session_config](VersionID previous_version, bool did_recover) {
             auto local_coordinator = RealmCoordinator::get_coordinator(config);
             REALM_ASSERT(local_coordinator);
-            auto local_config = local_coordinator->get_config();
             ThreadSafeReference active_after = local_coordinator->get_unbound_realm();
-            local_config.scheduler = nullptr;
-            SharedRealm frozen_before = local_coordinator->get_realm(local_config, previous_version);
+            SharedRealm frozen_before = local_coordinator->get_realm(config, previous_version);
             REALM_ASSERT(frozen_before);
             REALM_ASSERT(frozen_before->is_frozen());
-            config.sync_config->notify_after_client_reset(frozen_before, std::move(active_after), did_recover);
+            config.sync_config->notify_after_client_reset(std::move(frozen_before), std::move(active_after),
+                                                          did_recover);
         };
     }
-    if (copy_config.sync_config->notify_before_client_reset) {
-        config.notify_before_client_reset = [config = copy_config](std::string local_path) {
-            REALM_ASSERT(local_path == config.path);
-            auto local_coordinator = RealmCoordinator::get_coordinator(config);
-            REALM_ASSERT(local_coordinator);
-            auto local_config = local_coordinator->get_config();
-            local_config.scheduler = nullptr;
-            SharedRealm frozen_local = local_coordinator->get_realm(local_config, VersionID());
-            REALM_ASSERT(frozen_local);
-            REALM_ASSERT(frozen_local->is_frozen());
-            config.sync_config->notify_before_client_reset(frozen_local);
+    if (session_config.sync_config->notify_before_client_reset) {
+        config.notify_before_client_reset = [config = session_config](VersionID version) {
+            config.sync_config->notify_before_client_reset(Realm::get_frozen_realm(config, version));
         };
     }
 
-    config.fresh_copy = std::move(fresh_copy);
-    config.recovery_is_allowed = recovery_is_allowed;
     return config;
 }
 

src/realm/sync/client_base.hpp

@@ -62,9 +62,8 @@ struct ClientReset {
     realm::ClientResyncMode mode;
     DBRef fresh_copy;
     bool recovery_is_allowed = true;
-    util::UniqueFunction<void(std::string path)> notify_before_client_reset;
-    util::UniqueFunction<void(std::string path, VersionID before_version, bool did_recover)>
-        notify_after_client_reset;
+    util::UniqueFunction<void(VersionID)> notify_before_client_reset;
+    util::UniqueFunction<void(VersionID before_version, bool did_recover)> notify_after_client_reset;
 };
 
 /// \brief Protocol errors discovered by the client.

src/realm/sync/noinst/client_reset_operation.cpp

@@ -59,42 +59,44 @@ bool ClientResetOperation::finalize(sync::SaltedFileIdent salted_file_ident, syn
     // only do the reset if there is data to reset
     // if there is nothing in this Realm, then there is nothing to reset and
     // sync should be able to continue as normal
-    bool local_realm_exists = m_db->get_version_of_latest_snapshot() != 0;
-    if (local_realm_exists) {
-        REALM_ASSERT_EX(m_db_fresh, m_db->get_path(), m_mode);
-        m_logger.debug("ClientResetOperation::finalize, realm_path = %1, local_realm_exists = %2, mode = %3",
-                       m_db->get_path(), local_realm_exists, m_mode);
+    auto latest_version = m_db->get_version_id_of_latest_snapshot();
 
-        client_reset::LocalVersionIDs local_version_ids;
-        auto always_try_clean_up = util::make_scope_exit([&]() noexcept {
-            clean_up_state();
-        });
+    bool local_realm_exists = latest_version.version != 0;
+    m_logger.debug("ClientResetOperation::finalize, realm_path = %1, local_realm_exists = %2, mode = %3",
+                   m_db->get_path(), local_realm_exists, m_mode);
+    if (!local_realm_exists) {
+        return false;
+    }
 
-        std::string local_path = m_db->get_path();
-        if (m_notify_before) {
-            m_notify_before(local_path);
-        }
+    REALM_ASSERT_EX(m_db_fresh, m_db->get_path(), m_mode);
 
-        // If m_notify_after is set, pin the previous state to keep it around.
-        TransactionRef previous_state;
-        if (m_notify_after) {
-            previous_state = m_db->start_frozen();
-        }
-        bool did_recover_out = false;
-        local_version_ids = client_reset::perform_client_reset_diff(
-            m_db, m_db_fresh, m_salted_file_ident, m_logger, m_mode, m_recovery_is_allowed, &did_recover_out,
-            sub_store, std::move(on_flx_version_complete)); // throws
+    client_reset::LocalVersionIDs local_version_ids;
+    auto always_try_clean_up = util::make_scope_exit([&]() noexcept {
+        clean_up_state();
+    });
 
-        if (m_notify_after) {
-            m_notify_after(local_path, previous_state->get_version_of_current_transaction(), did_recover_out);
-        }
+    if (m_notify_before) {
+        m_notify_before(latest_version);
+    }
 
-        m_client_reset_old_version = local_version_ids.old_version;
-        m_client_reset_new_version = local_version_ids.new_version;
+    // If m_notify_after is set, pin the previous state to keep it around.
+    TransactionRef previous_state;
+    if (m_notify_after) {
+        previous_state = m_db->start_frozen();
+    }
+    bool did_recover_out = false;
+    local_version_ids = client_reset::perform_client_reset_diff(
+        m_db, m_db_fresh, m_salted_file_ident, m_logger, m_mode, m_recovery_is_allowed, &did_recover_out, sub_store,
+        std::move(on_flx_version_complete)); // throws
 
-        return true;
+    if (m_notify_after) {
+        m_notify_after(previous_state->get_version_of_current_transaction(), did_recover_out);
     }
-    return false;
+
+    m_client_reset_old_version = local_version_ids.old_version;
+    m_client_reset_new_version = local_version_ids.new_version;
+
+    return true;
 }
 
 void ClientResetOperation::clean_up_state() noexcept

src/realm/sync/noinst/client_reset_operation.hpp

@@ -34,8 +34,8 @@ namespace realm::_impl {
 // state Realm download.
 class ClientResetOperation {
 public:
-    using CallbackBeforeType = util::UniqueFunction<void(std::string)>;
-    using CallbackAfterType = util::UniqueFunction<void(std::string, VersionID, bool)>;
+    using CallbackBeforeType = util::UniqueFunction<void(VersionID)>;
+    using CallbackAfterType = util::UniqueFunction<void(VersionID, bool)>;
 
     ClientResetOperation(util::Logger& logger, DBRef db, DBRef db_fresh, ClientResyncMode mode,
                          CallbackBeforeType notify_before, CallbackAfterType notify_after, bool recovery_is_allowed);