From 599627a940874231c6d7cdb876b610ce784b19ee Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Sun, 13 Sep 2026 11:51:26 -0400 Subject: [PATCH 1/2] fix: stop the cycle timer leaking an interval on every restore fixes #5135 cycleStart() took a fresh setInterval id straight into cycleIntervalId without clearing what was already there. Once overwritten the old id is unrecoverable, so the orphan keeps calling nextCycleView every second and cyclePause() can only ever stop the last one armed. Several callers reach cycleStart() with no cyclePause() in between: the play button, the are-you-still-watching modal closing, and startPage(). That last one is the routine path - it runs from visibilitychange, from resume and from pageshow, and a restore fires more than one of those, so a tab coming back while cycling was active armed two. The monitor restart in the same function is protected, since it nulls prevStateStarted on the way through, but the cycle branch below it never cleared prevStateCycle. Clear the interval at the top of cycleStart(), which makes every caller safe whatever order they arrive in, and clear prevStateCycle in startPage() the way prevStateStarted already is. cycle.js has the same shape in its own cycleStart() behind a play button, so it gets the same guard. Tests in tests/js/watch-cycle-interval.test.js drive watch.js under stubbed timers and count what is left running: two starts leave one interval, a pause after three starts leaves none, and a second startPage() does not re-arm. Three of the four fail against the unfixed file. Full JS suite passes, ESLint clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UkQwahn9pi1y4wJe9BTxjM --- tests/js/watch-cycle-interval.test.js | 150 ++++++++++++++++++++++++++ web/skins/classic/views/js/cycle.js | 3 + web/skins/classic/views/js/watch.js | 14 ++- 3 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/js/watch-cycle-interval.test.js diff --git a/tests/js/watch-cycle-interval.test.js b/tests/js/watch-cycle-interval.test.js new file mode 100644 index 000000000..1b0684bcc --- /dev/null +++ b/tests/js/watch-cycle-interval.test.js @@ -0,0 +1,150 @@ +'use strict'; + +// cycleStart() took a fresh setInterval id without clearing the one already in +// cycleIntervalId. Several callers reach it with no cyclePause() in between - +// the play button, the are-you-still-watching modal closing, and startPage(), +// which runs on visibilitychange, resume and pageshow, more than one of which +// fires on a single restore. The overwritten id is unrecoverable, so the orphan +// interval keeps ticking and cyclePause() can only ever stop the last one. +// refs #5135 + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const vm = require('vm'); + +const src = fs.readFileSync( + path.join(__dirname, '../../web/skins/classic/views/js/watch.js'), 'utf8'); + +let passed = 0; +let failed = 0; +function test(name, fn) { + try { + fn(); + console.log(' ok ' + name); + passed++; + } catch (e) { + console.error(' FAIL ' + name); + console.error(' ' + (e.stack || e.message)); + failed++; + } +} + +// Chains like jQuery, but val() answers with a number so cycleStart()'s +// `secondsToCycle == 0` has something real to compare against. +function makeChainable() { + const proxy = new Proxy(function() {}, { + get: (target, prop) => { + if (prop === 'then') return undefined; + if (prop === 'val') return () => '10'; + if (prop === 'text') return () => ''; + return proxy; + }, + apply: () => proxy, + }); + return proxy; +} + +// watch.js is a browser file that leans on globals from skin.js and the view +// template. Resolve unknown globals to undefined, and give real values only to +// the timer functions, which are what these tests measure. +function loadWatch() { + const timers = {live: new Set(), nextId: 1, cleared: []}; + const globals = { + $j: makeChainable(), + $: makeChainable(), + console: {log() {}, warn() {}, error() {}, debug() {}}, + document: { + getElementById: () => null, + querySelector: () => null, + querySelectorAll: () => [], + addEventListener: () => {}, + createElement: () => ({}), + }, + setInterval: () => { + const id = timers.nextId++; + timers.live.add(id); + return id; + }, + clearInterval: (id) => { + timers.cleared.push(id); + timers.live.delete(id); + }, + setTimeout: () => 0, + clearTimeout: () => 0, + Object: Object, + Array: Array, + JSON: JSON, + monitorData: [{id: 1}, {id: 2}], + ZM_WEB_VIEWING_TIMEOUT: 0, + addEventListener: () => {}, + }; + const sandbox = new Proxy(globals, { + has: () => true, + get: (target, prop) => (prop === Symbol.unscopables ? undefined : target[prop]), + }); + globals.window = sandbox; + vm.createContext(sandbox); + vm.runInContext(src, sandbox, {filename: 'watch.js'}); + assert.strictEqual(typeof globals.cycleStart, 'function', + 'watch.js did not define cycleStart'); + return {globals, timers}; +} + +console.log('watch.js cycle interval lifecycle'); + +test('the hazard is real: an overwritten interval id cannot be cleared', () => { + // What the bug was, independent of watch.js: take a second id into the same + // variable and the first interval is running with nobody holding its handle. + const live = new Set(); + let next = 1; + const set = () => { + const id = next++; live.add(id); return id; + }; + const clear = (id) => live.delete(id); + let handle = set(); + handle = set(); + clear(handle); + assert.strictEqual(live.size, 1, 'the orphaned interval should still be live'); +}); + +test('calling cycleStart twice leaves exactly one interval running', () => { + const {globals, timers} = loadWatch(); + globals.cycleStart(); + assert.strictEqual(timers.live.size, 1, 'first start should arm one interval'); + globals.cycleStart(); + assert.strictEqual(timers.live.size, 1, + 'second start left ' + timers.live.size + ' intervals running'); +}); + +test('cyclePause after repeated starts leaves nothing running', () => { + const {globals, timers} = loadWatch(); + globals.cycleStart(); + globals.cycleStart(); + globals.cycleStart(); + globals.cyclePause(); + assert.strictEqual(timers.live.size, 0, + 'cyclePause could not stop every interval cycleStart armed'); +}); + +test('startPage clears prevStateCycle so a second restore does not re-arm', () => { + // A restore fires more than one of visibilitychange/resume/pageshow, so + // startPage() runs twice. prevStateStarted is nulled on the way through; + // prevStateCycle has to be too, or the second run starts cycling again. + const {globals, timers} = loadWatch(); + // Run the auth gate synchronously; it is not what this test is about. + globals.whenAuthFresh = (cb) => cb(); + globals.prevStateCycle = true; + + globals.startPage(); + assert.strictEqual(timers.live.size, 1, 'first restore should arm one interval'); + assert.strictEqual(globals.prevStateCycle, null, + 'prevStateCycle should be cleared once acted on'); + + globals.startPage(); + assert.strictEqual(timers.live.size, 1, + 'second restore left ' + timers.live.size + ' intervals running'); +}); + +console.log('\n' + passed + ' passed, ' + failed + ' failed'); +process.exit(failed ? 1 : 0); diff --git a/web/skins/classic/views/js/cycle.js b/web/skins/classic/views/js/cycle.js index a21393798..682655dfb 100644 --- a/web/skins/classic/views/js/cycle.js +++ b/web/skins/classic/views/js/cycle.js @@ -17,6 +17,9 @@ function cyclePause() { } function cycleStart() { + // The play button can be reached without a pause in between, and the old id + // is unrecoverable once overwritten. refs #5135 + clearInterval(intervalId); intervalId = setInterval(nextCycleView, cycleRefreshTimeout); pauseBtn.prop('disabled', false); playBtn.prop('disabled', true); diff --git a/web/skins/classic/views/js/watch.js b/web/skins/classic/views/js/watch.js index a74dea003..cbbcc050e 100644 --- a/web/skins/classic/views/js/watch.js +++ b/web/skins/classic/views/js/watch.js @@ -1269,6 +1269,12 @@ function cyclePause() { } function cycleStart() { + // Drop any interval already running before taking a new id. Several callers + // can reach this without a cyclePause() in between - the play button, the + // are-you-still-watching modal closing, and startPage() on every restore - + // and the old id is unrecoverable once overwritten, so the orphan ticks on + // and cyclePause() can only ever stop the last one. refs #5135 + clearInterval(cycleIntervalId); if (secondsToCycle == 0) secondsToCycle = $j('#cyclePeriod').val(); cycleIntervalId = setInterval(nextCycleView, 1000); cycle = true; @@ -1583,7 +1589,13 @@ function startPage() { } else if (monitorStream && monitorStream.element && ((monitorStream.zmsState == 'paused') || (monitorStream.element.video && monitorStream.element.video.paused) || monitorStream.element.paused)) { prevStateStarted = null; } - if (prevStateCycle) cycleStart(); + // Clear it the way prevStateStarted is cleared above: startPage() runs on + // visibilitychange, resume and pageshow, and a restore fires more than one + // of those. refs #5135 + if (prevStateCycle) { + prevStateCycle = null; + cycleStart(); + } }); } From fa000bc365a53eb67c6ebb18d3753cab3b34f809 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:43:44 +0000 Subject: [PATCH 2/2] build(deps): bump github/codeql-action from 4.37.9 to 4.38.0 Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.37.9 to 4.38.0. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/github/codeql-action/compare/v4.37.9...v4.38.0) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.38.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/codeql-analysis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index c2b472785..c47041fbc 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -67,7 +67,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@v4.37.9 + uses: github/codeql-action/init@v4.38.0 with: languages: ${{ matrix.language }} config-file: ./.github/codeql/codeql-config.yml @@ -90,7 +90,7 @@ jobs: # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild - uses: github/codeql-action/autobuild@v4.37.9 + uses: github/codeql-action/autobuild@v4.38.0 # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl- @@ -105,4 +105,4 @@ jobs: # make release - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v4.37.9 + uses: github/codeql-action/analyze@v4.38.0