Skip to content

test(xterm): A/B observer-nullify variant vs WeakRef (upstream #5821)#652

Draft
srid wants to merge 1 commit intomasterfrom
test/observer-nullify-variant
Draft

test(xterm): A/B observer-nullify variant vs WeakRef (upstream #5821)#652
srid wants to merge 1 commit intomasterfrom
test/observer-nullify-variant

Conversation

@srid
Copy link
Copy Markdown
Member

@srid srid commented Apr 21, 2026

Purpose

Maintainers on xtermjs/xterm.js#5821 (jerch, Tyriar) pushed back on the WeakRef wrap and suggested storing the IntersectionObserver on this and explicitly nulling it on dispose — without WeakRef. Per W3C spec, disconnect() does not release the callback (the observer retains it so .observe() can be re-armed), so their hypothesis is: the observer itself is the JS-reachable root, and dropping that ref lets the whole chain GC.

This draft PR points the xterm override at a stacked test branch that implements that variant, so we can run a prod Task Manager A/B on pureintent.

Variant

juspay/xterm.js@test/kolu-observer-nullify-built stacks one commit on top of the existing fix/kolu-xterm-fixes-built. The diff (src/browser/services/RenderService.ts):

private _observer: IntersectionObserver | undefined;
...
this._observer = new w.IntersectionObserver(
  e => this._handleIntersectionChange(e[e.length - 1]),
  { threshold: 0 }
);
this._observer.observe(screenElement);
this._observerDisposable.value = toDisposable(() => {
  this._observer?.disconnect();
  this._observer = undefined;
});

No WeakRef. Strong this capture in the callback; observer ref hoisted onto the instance and nulled on dispose.

Local test was inconclusive

Ran a 30-toggle canvas-maximize A/B under chrome-devtools MCP (headless). All three variants — unpatched, WeakRef, observer-nullify — showed identical benign heap diffs: no JSArrayBufferData / Uint32Array / BufferLine growth. The MCP Chrome lacks whatever instrumentation/extension caused the original retention in real-world Chrome, so local can't distinguish the variants. Prod Task Manager on pureintent is the actual arbiter.

Repro recipe (updated for mode-less workspace)

The original #617 repro was Focus ↔ Canvas mode toggle, which no longer exists (#622 made the workspace mode-less — the canvas is always on). The modern equivalent that still cycles Terminal mount/unmount is the canvas-maximize toggle — the Maximize button on each tile's chrome.

Why it works: toggling canvasMaximized flips TerminalCanvas.tsx:352-370 between <For each={tileIds}> and <Show when={maximized && activeId} keyed>. With 7 tiles, one toggle cycle produces 8 Terminal disposes + 8 Terminal mounts (7 tiled tiles → 1 maximized, then back). Over 30 cycles that's 240 RenderService create/dispose pairs — the same surface as the original repro. Confirmed locally via window.__kolu.lifecycle(): mounts increments from 15 → 255, cleanups from 8 → 248 across 30 cycles, with 7 live terminals throughout.

Steps

  1. Fresh tab on pureintent with 7 terminals restored (Task Manager: capture baseline Memory Footprint)
  2. Paste in DevTools console:
    (async () => {
      for (let i = 0; i < 30; i++) {
        document.querySelector('[data-testid=canvas-tile-maximize]').click();
        await new Promise(r => setTimeout(r, 250));
        document.querySelector('[data-testid=canvas-tile-maximize]').click();
        await new Promise(r => setTimeout(r, 250));
      }
    })();
  3. Capture post Memory Footprint
  4. Compute Δ / 30 toggles

Interpreting the Δ

Fallback if canvas-maximize doesn't stress the path

If the prod Δ with canvas-maximize is implausibly flat (possible — WebGL addon lifecycle could mask the IntersectionObserver path through some unrelated interaction), fall back to the close/recreate cycle: 30× Ctrl+Enter followed by Ctrl+D per terminal. That exercises the exact same RenderService lifecycle at 30× the rate of the maximize toggle, and was already a documented repro in the perf-diagnose skill.

Rollback

Not for merge either way. Close draft after data is collected; the actual decision lands as a change to the upstream PR #5821 (either simplify to observer-nullify or keep WeakRef).

Refs xtermjs/xterm.js#5821, #617, #620

Maintainers on xtermjs/xterm.js#5821 asked whether the retention
could be fixed by storing the observer on `this` and explicitly
nulling it on dispose (the `observer = undefined` suggestion),
instead of the WeakRef wrap shipped in #617.

This branch points the xterm override at a stacked test branch that
swaps the WeakRef capture for the observer-nullify variant:

    src/browser/services/RenderService.ts

    private _observer: IntersectionObserver | undefined;
    ...
    this._observer = new w.IntersectionObserver(
      e => this._handleIntersectionChange(e[e.length - 1]),
      { threshold: 0 }
    );
    this._observer.observe(screenElement);
    this._observerDisposable.value = toDisposable(() => {
      this._observer?.disconnect();
      this._observer = undefined;
    });

Purpose: 30-toggle Task Manager A/B on real production Chrome to
verify whether observer-nullify recovers the same footprint as the
WeakRef fix, or only WeakRef defangs the retention.

Not for merge. Draft for prod A/B only.

Refs xtermjs/xterm.js#5821, #617
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.

1 participant