Merge branch 'master' of github.com:ZoneMinder/zoneminder

.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

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);

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);

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();
+    }
   });
 }