Skip to content

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

Description

@Shivansh1205

SSModeComponent defers attaching its document listeners with an untracked setTimeout, so destroying the component while that timer is pending re-attaches them after cleanup and leaks them for the life of the page.

The offending code

ss-mode.component.ts#L26-L38

toggleSSMode() {
  this.ssMode = !this.ssMode;
  document.body.classList.toggle('ss-mode');
  if (this.ssMode) {
    // WORKAROUND - Adding the event listener directly somehow calls it on the first click
    setTimeout(() => {                                    // <-- id never stored
      document.addEventListener('click', this.onDocumentClick);
      document.addEventListener('touchstart', this.onDocumentClick);
    }, 1);

and ss-mode.component.ts#L49-L55:

ngOnDestroy() {
  document.onfullscreenchange = null;
  document.removeEventListener('click', this.onDocumentClick);        // nothing attached yet
  document.removeEventListener('touchstart', this.onDocumentClick);
  ...
}

Why it is wrong

The listener attachment is deferred by 1ms to work around the activating click firing it immediately. The timer id is never stored, so ngOnDestroy has no way to cancel it.

If the component is destroyed inside that 1ms window, the order is:

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

The listeners are now attached to a destroyed component, and nothing will ever remove them: the only removal paths are ngOnDestroy (already run) and toggleSSMode's else-branch (unreachable — the component is gone). They stay on document for the lifetime of the page, and because the handler is

private onDocumentClick = () => {
  document.exitFullscreen?.();
};

every subsequent click anywhere in the app calls exitFullscreen().

This is a half-applied fix, not a new bug

MakePictureComponent contains 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:

// make-picture.component.ts — already correct
private ssTimeoutId: ReturnType<typeof setTimeout> | null = null;
...
this.ssTimeoutId = setTimeout(() => {
  this.ssTimeoutId = null;
  document.addEventListener('click', this.onDocumentClick);
  ...

SSModeComponent was never given the same treatment, so the reviewer's fix covers one of the two copies. Related earlier work: #762 / #763 cleaned up listeners in this component but predates the deferred-attachment workaround being recognised as a hazard.

What the user sees

After the screenshot-mode component is torn down during that window, clicking anywhere in Phoenix can drop the user out of fullscreen unexpectedly, with no visible cause. The listeners also keep the destroyed component instance reachable, so it is never garbage collected.

Steps to reproduce

The window is narrow in manual use, but it is deterministic under fake timers:

component.toggleSSMode();   // arms the 1ms timer
fixture.destroy();          // ngOnDestroy removes nothing (nothing attached yet)
jest.runAllTimers();        // timer fires -> listeners attached post-destroy

The listeners are then present on document with no owner.

Expected behaviour

  • ngOnDestroy cancels the pending timer, so no listener is attached after teardown.
  • Toggling screenshot mode off also cancels a still-pending timer.
  • A normal toggle still attaches both listeners as before.

I have a branch with the fix and tests, and will open a PR referencing this issue.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions