From 52115d07c2990fcd06f19bce62f1d5965c091b61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20D=E2=80=99Aquino?= Date: Wed, 19 Nov 2025 20:14:34 -0800 Subject: [PATCH] Fix profile crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash that would occasionally occur when visiting profiles. NdbTxn objects were being deinitialized on different threads from their initialization, causing incorrect reference count decrements in thread-local transaction dictionaries. This led to premature destruction of shared ndb_txn C objects still in use by other tasks, resulting in use-after-free crashes. The root cause is that Swift does not guarantee tasks resume on the same thread after await suspension points, while NdbTxn's init/deinit rely on thread-local storage to track inherited transaction reference counts. This means that `NdbTxn` objects cannot be used in async functions, as that may cause the garbage collector to deinitialize `NdbTxn` at the end of such function, which may be running on a different thread at that point, causing the issue explained above. The fix in this case is to eliminate the `async` version of the `NdbNoteLender.borrow` method, and update usages to utilize other available methods. Note: This is a rewrite of the fix in https://github.com/damus-io/damus/pull/3329 Note 2: This relates to the fix of an unreleased feature, so therefore no changelog is needed. Changelog-None Co-authored-by: alltheseas <64376233+alltheseas@users.noreply.github.com> Closes: https://github.com/damus-io/damus/issues/3327 Signed-off-by: Daniel D’Aquino --- .../UserRelayListManager.swift | 13 ++- .../Profile/Models/ProfileModel.swift | 16 ++-- .../NostrNetworkManagerTests.swift | 81 +++++++++++++++++++ nostrdb/UnownedNdbNote.swift | 13 --- 4 files changed, 94 insertions(+), 29 deletions(-) diff --git a/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift b/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift index 83fc96ca..8e40177e 100644 --- a/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift +++ b/damus/Core/Networking/NostrNetworkManager/UserRelayListManager.swift @@ -135,13 +135,12 @@ extension NostrNetworkManager { let filter = NostrFilter(kinds: [.relay_list], authors: [delegate.keypair.pubkey]) for await noteLender in self.reader.streamIndefinitely(filters: [filter]) { let currentRelayListCreationDate = self.getUserCurrentRelayListCreationDate() - try? await noteLender.borrow({ note in - guard note.pubkey == self.delegate.keypair.pubkey else { return } // Ensure this new list was ours - guard note.createdAt > (currentRelayListCreationDate ?? 0) else { return } // Ensure this is a newer list - guard let relayList = try? NIP65.RelayList(event: note) else { return } // Ensure it is a valid NIP-65 list - - try? await self.set(userRelayList: relayList) // Set the validated list - }) + guard let note = noteLender.justGetACopy() else { continue } + guard note.pubkey == self.delegate.keypair.pubkey else { continue } // Ensure this new list was ours + guard note.created_at > (currentRelayListCreationDate ?? 0) else { continue } // Ensure this is a newer list + guard let relayList = try? NIP65.RelayList(event: note) else { continue } // Ensure it is a valid NIP-65 list + + try? await self.set(userRelayList: relayList) // Set the validated list } } diff --git a/damus/Features/Profile/Models/ProfileModel.swift b/damus/Features/Profile/Models/ProfileModel.swift index 309d98c9..fa67159d 100644 --- a/damus/Features/Profile/Models/ProfileModel.swift +++ b/damus/Features/Profile/Models/ProfileModel.swift @@ -116,15 +116,13 @@ class ProfileModel: ObservableObject, Equatable { let conversations_filter_us = NostrFilter(kinds: conversation_kinds, pubkeys: [pubkey], limit: limit, authors: [damus.pubkey]) print("subscribing to conversation events from and to profile \(pubkey)") for await noteLender in self.damus.nostrNetwork.reader.streamIndefinitely(filters: [conversations_filter_them, conversations_filter_us]) { - try? await noteLender.borrow { ev in - if await !seen_event.contains(ev.id) { - let event = ev.toOwned() - Task { await self.add_event(event) } - conversation_events.insert(ev.id) - } - else if !conversation_events.contains(ev.id) { - conversation_events.insert(ev.id) - } + guard let event = noteLender.justGetACopy() else { continue } + if await !seen_event.contains(event.id) { + Task { await self.add_event(event) } + conversation_events.insert(event.id) + } + else if !conversation_events.contains(event.id) { + conversation_events.insert(event.id) } } } diff --git a/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift b/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift index 431ca8af..44e25c24 100644 --- a/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift +++ b/damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift @@ -163,4 +163,85 @@ class NostrNetworkManagerTests: XCTestCase { XCTAssertEqual(count, expectedCount, "Should receive all \(expectedCount) events") XCTAssertEqual(receivedIds.count, expectedCount, "Should receive \(expectedCount) unique events") } + + /// Ensures the relay list listener ignores a bad event and still applies the next valid update. + func testRelayListListenerSkipsInvalidEventsAndContinues() async throws { + let ndb = Ndb.test + let delegate = MockNetworkDelegate(ndb: ndb, keypair: test_keypair, bootstrapRelays: [RelayURL("wss://relay.damus.io")!]) + let pool = RelayPool(ndb: ndb, keypair: test_keypair) + let reader = MockSubscriptionManager(pool: pool, ndb: ndb) + let manager = SpyUserRelayListManager(delegate: delegate, pool: pool, reader: reader) + let appliedExpectation = expectation(description: "Applies valid relay list after encountering an invalid event") + manager.setExpectation = appliedExpectation + + guard let invalidEvent = NostrEvent(content: "invalid", keypair: test_keypair, kind: NostrKind.metadata.rawValue, createdAt: 1) else { + XCTFail("Failed to create invalid test event") + return + } + let validRelayList = NIP65.RelayList(relays: [RelayURL("wss://relay-2.damus.io")!]) + guard let validEvent = validRelayList.toNostrEvent(keypair: test_keypair_full) else { + XCTFail("Failed to create valid relay list event") + return + } + + // Feed the listener a bad event followed by a valid relay list. + reader.queuedLenders = [.owned(invalidEvent), .owned(validEvent)] + + await manager.listenAndHandleRelayUpdates() + await fulfillment(of: [appliedExpectation], timeout: 1.0) + + XCTAssertEqual(manager.setCallCount, 1) + XCTAssertEqual(manager.appliedRelayLists.first?.relays.count, validRelayList.relays.count) + } +} + +// MARK: - Test doubles + +private final class MockNetworkDelegate: NostrNetworkManager.Delegate { + var ndb: Ndb + var keypair: Keypair + var latestRelayListEventIdHex: String? + var latestContactListEvent: NostrEvent? + var bootstrapRelays: [RelayURL] + var developerMode: Bool = false + var experimentalLocalRelayModelSupport: Bool = false + var relayModelCache: RelayModelCache + var relayFilters: RelayFilters + var nwcWallet: WalletConnectURL? + + init(ndb: Ndb, keypair: Keypair, bootstrapRelays: [RelayURL]) { + self.ndb = ndb + self.keypair = keypair + self.bootstrapRelays = bootstrapRelays + self.relayModelCache = RelayModelCache() + self.relayFilters = RelayFilters(our_pubkey: keypair.pubkey) + } +} + +private final class MockSubscriptionManager: NostrNetworkManager.SubscriptionManager { + var queuedLenders: [NdbNoteLender] = [] + + init(pool: RelayPool, ndb: Ndb) { + super.init(pool: pool, ndb: ndb, experimentalLocalRelayModelSupport: false) + } + + override func streamIndefinitely(filters: [NostrFilter], to desiredRelays: [RelayURL]? = nil, streamMode: NostrNetworkManager.StreamMode? = nil, id: UUID? = nil) -> AsyncStream { + let lenders = queuedLenders + return AsyncStream { continuation in + lenders.forEach { continuation.yield($0) } + continuation.finish() + } + } +} + +private final class SpyUserRelayListManager: NostrNetworkManager.UserRelayListManager { + var setCallCount = 0 + var appliedRelayLists: [NIP65.RelayList] = [] + var setExpectation: XCTestExpectation? + + override func set(userRelayList: NIP65.RelayList) async throws(UpdateError) { + setCallCount += 1 + appliedRelayLists.append(userRelayList) + setExpectation?.fulfill() + } } diff --git a/nostrdb/UnownedNdbNote.swift b/nostrdb/UnownedNdbNote.swift index 2c971659..e3d8c418 100644 --- a/nostrdb/UnownedNdbNote.swift +++ b/nostrdb/UnownedNdbNote.swift @@ -66,19 +66,6 @@ enum NdbNoteLender: Sendable { } } - /// Borrows the note temporarily (asynchronously) - func borrow(_ lendingFunction: (_: borrowing UnownedNdbNote) async throws -> T) async throws -> T { - switch self { - case .ndbNoteKey(let ndb, let noteKey): - guard !ndb.is_closed else { throw LendingError.ndbClosed } - guard let ndbNoteTxn = ndb.lookup_note_by_key(noteKey) else { throw LendingError.errorLoadingNote } - guard let unownedNote = UnownedNdbNote(ndbNoteTxn) else { throw LendingError.errorLoadingNote } - return try await lendingFunction(unownedNote) - case .owned(let note): - return try await lendingFunction(UnownedNdbNote(note)) - } - } - /// Gets an owned copy of the note func getCopy() throws -> NdbNote { return try self.borrow({ ev in