Skip to content

fix(ui-components): cancel SSMode's deferred listener timer on destroy - #1035

Merged
EdwardMoyse merged 1 commit into
HSF:mainfrom
Shivansh1205:fix/ss-mode-timer-leak
Sep 15, 2026
Merged

EdwardMoyse merged 1 commit into
HSF:mainfrom
Shivansh1205:fix/ss-mode-timer-leak

Conversation

@Shivansh1205

Copy link
Copy Markdown
Contributor

Fixes #1034

Problem

toggleSSMode defers attaching the document click / touchstart listeners by 1ms, working around the activating click firing them immediately. The timer id was never stored, so ngOnDestroy had no way to cancel it.

Destroy the component inside that window and the order is:

  1. ngOnDestroy calls removeEventListener — no-ops, nothing is attached yet.
  2. The timer fires and calls addEventListener for both events.

The listeners are now owned by a destroyed component, and neither removal path can reach them: ngOnDestroy has already run, and toggleSSMode's else-branch is unreachable once the component is gone. They stay on document for the life of the page, and since the handler is () => document.exitFullscreen?.(), every later click anywhere in the app drops the user out of fullscreen.

This is a half-applied fix

MakePictureComponent has the identical workaround and was hardened for exactly this in ef594ae — "store setTimeout id and clear it in ngOnDestroy to prevent re-adding listeners after destroy" — from review feedback on #867. SSModeComponent was never updated, so that fix covered one of the two copies. This PR is the other one, and deliberately mirrors make-picture's shape so the two stay in step.

Fix

  • Store the timer id in ssTimeoutId, cleared inside the callback when it fires normally.
  • Cancel it in ngOnDestroy, before the removeEventListener calls.
  • Also cancel it in the toggle-off branch, so turning screenshot mode off while the timer is still pending does not attach listeners a moment later.

The shared clearSSTimeout() helper keeps the null-check in one place.

Tests

  • does not attach listeners after the component is destroyed — arms the timer, destroys the fixture, runs timers, and asserts nothing was attached for the component's handler afterwards. Fails before this change.
  • still attaches listeners on a normal toggle — guards against over-correcting into never attaching them. Passes before and after.

A note on how these are written, since it cost me a wrong turn: onDocumentClick is an arrow-function class field, so it is a stable per-instance reference and removeEventListener genuinely matches it. A test that merely counts add-vs-remove calls therefore nets to zero and passes even against the bug. The assertion has to be about ordering — that an attach happens after ngOnDestroy — which is what these tests check.

I also dropped an assertion on jest.getTimerCount(); zone.js keeps its own timers pending in a TestBed fixture (it returned 6), so it is not a meaningful signal here.

Verification

  • Full phoenix-ng suite: 62 suites / 223 tests passed, 0 failures.
  • eslint clean on the ss-mode directory.
  • No pre-existing failures found, so none are carried here.

`toggleSSMode` defers attaching the document click/touchstart listeners by 1ms,
working around the activating click firing them immediately. The timer id was
never stored, so `ngOnDestroy` had no way to cancel it.

Destroying the component inside that window runs the removals first, while
nothing is attached yet, and the timer then attaches both listeners to a
component that no longer exists. Neither removal path can reach them
afterwards - `ngOnDestroy` has already run and the toggle-off branch is
unreachable - so they stay on `document` for the life of the page and every
later click calls `exitFullscreen()`.

MakePictureComponent carries the identical workaround and was hardened for
exactly this in ef594ae, from review feedback on HSF#867. That fix covered one of
the two copies; this is the other.

Store the timer id and clear it in `ngOnDestroy`, and also when the user
toggles screenshot mode off while it is still pending. Mirrors make-picture so
the two stay in step.

Added tests covering that no listener is attached after destroy and that a
normal toggle still attaches both. The first fails before this change.

Fixes HSF#1034
@github-actions

Copy link
Copy Markdown

🚀 Preview deployed: http://phoenix-pr-1035.surge.sh

Built from 454ab44.

@EdwardMoyse
EdwardMoyse merged commit 8657e7c into HSF:main Sep 15, 2026
4 checks passed

This branch was successfully deployed

1 active deployment
pull-request — 454ab44e Deployed Sep 14, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSModeComponent leaks document listeners: untracked setTimeout re-attaches them after ngOnDestroy

2 participants