fix: remove stale listeners in setReconnect() before adding new ones (OPTI-2441)#518
Conversation
OPTI-2441: setReconnect() adds event listeners via .on() but never removes old ones. During migrate paths, stale listeners on old signaling/peer instances can fire and operate on the current connection via this closure, potentially triggering unwanted reconnects. Changes: - Store listener references and remove them at the start of setReconnect() before adding new ones (Option A) - Add removeReconnectListeners() and call it from stop() as well - Use .once() for migrate cleanup listeners in View.js and Publish.js since they only need to fire once Co-Authored-By: craig.johnston <cjohn@dolby.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Call removeReconnectListeners() before swapping this.signaling and this.webRTCPeer to new instances in initConnection(), ensuring stale listeners are removed from the OLD instances (not the new ones). Co-Authored-By: craig.johnston <cjohn@dolby.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The .once() handler gets consumed by non-terminal states like 'connecting' (Chrome) or 'checking' (Firefox) before reaching a terminal state, causing old resources to leak. Use .on() with explicit .off() self-removal when a terminal state is reached. Co-Authored-By: craig.johnston <cjohn@dolby.com>
2 participants
Summary
setReconnect()inBaseWebRTC.jsadds event listeners via.on()tothis.signalingandthis.webRTCPeerbut never removes old listeners. This PR stores handler references and removes them before adding new ones, and also cleans up listeners instop().Changes:
BaseWebRTC.js: NewremoveReconnectListeners()method that removes stored handler references (_migrateHandler,_signalingErrorHandler,_peerStateHandler). Called at the top ofsetReconnect()and instop().View.js/Publish.js:removeReconnectListeners()is called before the instance swap (this.signaling = signalingInstance/this.webRTCPeer = webRTCPeerInstance) so that.off()targets the old instances, not the new ones.View.js/Publish.js: Migrate cleanup handlers use.on()with explicit self-removal (.off()) once a terminal state is reached. This replaces the original anonymous.on()listeners and avoids the pitfall of.once()being consumed by non-terminal states like'connecting'(Chrome) or'checking'(Firefox).Tracked in OPTI-2441.
Review & Testing Checklist for Human
.on()pattern should ignore states like'connecting'/'checking'and only fire cleanup + self-remove on terminal states (connected,disconnected,failed,closed). Confirm this in a real browser migration scenario.removeReconnectListeners()is called before instance swap ininitConnection()(View.js:384, Publish.js:250), so.off()should target the old signaling/peer. Note thatsetReconnect()also callsremoveReconnectListeners()internally, but at that point it operates on the new instances (effectively a no-op for old listeners). The pre-swap call is what provides the actual cleanup.stop()→connect()still works correctly (non-migrate path) and that listeners don't accumulate across reconnects.Notes
re-emittercleanup pattern (stopReemitingWebRTCPeerInstanceEvents) already exists for re-emitted events but was not extended tosetReconnectlisteners. This PR takes a parallel approach by storing and removing individual handler references.removeReconnectListeners()does not null out handler references after removal — they are overwritten by the subsequentsetReconnect()call. This is intentional to keep the method simple and idempotent.Link to Devin session: https://dolby.devinenterprise.com/sessions/4d843ed3136b4e299f5bf3e30f6ff944
Requested by: @craig-johnston