From f09dee810cd13800106f85f96396944740330de0 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Tue, 28 Jul 2026 14:17:43 -0500 Subject: [PATCH] Record where a player crashed, not just what it said Three players died with "Cannot set properties of null (setting 'textContent')" and it could not be traced. The message names no file, and every candidate line in the current player was ruled out by inspection: the unguarded writes all build their element with createElement, every getElementById target exists in the markup, and the script runs after the markup. That points at an older cached build still served by the service worker, which is exactly the case where reading current source proves nothing. The ErrorEvent already carried filename, lineno and colno. They were being discarded. Keeping them makes the next occurrence name its own line. Composed to fit the 200 characters the server stores, so the location is not truncated away: message plus one location, basename only since the origin is already known from the device. A promise rejection has no filename, so it falls back to the first stack frame. A cross-origin script, which reports a bare "Script error." with nothing else, says so rather than emitting :0:0 as if that were an answer. A resource load failure still is not a crash; a test guards that, since this touched the handler that decides it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/player/index.html | 38 ++++++++- server/test/crash-beacon-location.test.js | 99 +++++++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 server/test/crash-beacon-location.test.js diff --git a/server/player/index.html b/server/player/index.html index 0f1ccdb..8f0e8bc 100644 --- a/server/player/index.html +++ b/server/player/index.html @@ -395,16 +395,50 @@ fetch(url, { method: 'POST', body, headers: { 'Content-Type': 'application/json' }, keepalive: true }).catch(() => {}); } catch (e) { /* a dying page must never throw */ } } + // A crash message on its own is not actionable. Three players died with + // "Cannot set properties of null (setting 'textContent')" and it could not be traced: the + // message names no file, and every candidate line in the CURRENT player was ruled out by + // inspection, which points at an older cached build still being served by the service + // worker — precisely the case where guessing from source is worthless. The error event + // already carries filename/lineno/colno; it was simply being discarded. Keep it. + // + // Deliberately compact: the server stores 200 chars (liveness.sanitizeExitReason), so this + // sends the message plus ONE location rather than a whole stack that would be truncated + // mid-frame. Only the basename is sent — the origin is already known from the device. + function crashDetail(message, file, line, col) { + let out = String(message || 'error'); + try { + if (file) { + const base = String(file).split('/').pop().split('?')[0] || String(file); + out += ` @ ${base}:${line || 0}:${col || 0}`; + } + } catch (e) { /* a dying page must never throw */ } + return out.slice(0, 200); + } window.addEventListener('error', (ev) => { // ONLY a real uncaught script error is a crash — a resource (img/script/link) load failure is NOT. if (!ev) return; const isResourceError = ev.target && ev.target !== window && (ev.target.src || ev.target.href); if (isResourceError) return; - sendExitBeacon('crashed', (ev.error && ev.error.message) || ev.message || 'error'); + // A cross-origin script reports a bare "Script error." with no filename or line. Say so + // explicitly rather than emitting a location of :0:0 that reads like a real answer. + const msg = (ev.error && ev.error.message) || ev.message || 'error'; + sendExitBeacon('crashed', ev.filename + ? crashDetail(msg, ev.filename, ev.lineno, ev.colno) + : crashDetail(msg + ' (no location — cross-origin script)')); }); window.addEventListener('unhandledrejection', (ev) => { const r = ev && ev.reason; - sendExitBeacon('crashed', (r && (r.message || String(r))) || 'unhandledrejection'); + const msg = (r && (r.message || String(r))) || 'unhandledrejection'; + // A rejection carries no filename/lineno, so take the first stack frame instead. + let frame = null; + try { + if (r && typeof r.stack === 'string') { + const l = r.stack.split('\n').find(x => /:\d+:\d+/.test(x)); + if (l) frame = l.trim().replace(/^at\s+/, '').slice(0, 120); + } + } catch (e) { /* never throw here */ } + sendExitBeacon('crashed', (frame ? `${msg} @ ${frame}` : String(msg)).slice(0, 200)); }); window.addEventListener('pagehide', (ev) => { if (ev && ev.persisted) return; // bfcache SUSPEND (may restore) — NOT a death; watchdog owns it diff --git a/server/test/crash-beacon-location.test.js b/server/test/crash-beacon-location.test.js new file mode 100644 index 0000000..27022eb --- /dev/null +++ b/server/test/crash-beacon-location.test.js @@ -0,0 +1,99 @@ +'use strict'; + +// A crash message with no location is not actionable. Three production players died with +// "Cannot set properties of null (setting 'textContent')" and it could not be traced: the +// message names no file, and every candidate line in the current player was ruled out by +// inspection — which points at an older cached build still served by the service worker, +// exactly the case where reading current source proves nothing. +// +// The ErrorEvent already carries filename/lineno/colno. It was being discarded. This pins that +// the location is kept, that it stays inside the 200 characters the server will store +// (lib/liveness.sanitizeExitReason truncates), and that the two cases which genuinely have no +// location — a cross-origin "Script error." and a promise rejection — say so or fall back to a +// stack frame, rather than reporting :0:0 as if it were an answer. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { sanitizeExitReason } = require('../lib/liveness'); + +const HTML = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); + +// Lift the real helper out of the player so this tests shipped code, not a paraphrase. +function loadCrashDetail() { + const start = HTML.indexOf('function crashDetail('); + assert.notEqual(start, -1, 'crashDetail() should exist in the player'); + let depth = 0; + for (let j = HTML.indexOf('{', start); j < HTML.length; j++) { + if (HTML[j] === '{') depth++; + else if (HTML[j] === '}' && --depth === 0) { + return new Function(`${HTML.slice(start, j + 1)} return crashDetail;`)(); + } + } + throw new Error('unbalanced braces'); +} +const crashDetail = loadCrashDetail(); + +test('THE POINT: the crash location survives instead of being discarded', () => { + const d = crashDetail("Cannot set properties of null (setting 'textContent')", + 'https://screentinker.com/player/index.html', 3087, 41); + assert.match(d, /index\.html:3087:41/, 'file, line and column are all kept'); + assert.match(d, /textContent/, 'and the message is still there'); +}); + +test('only the basename is sent — the origin is already known from the device', () => { + const d = crashDetail('boom', 'https://screentinker.com/player/transitions.js', 12, 3); + assert.match(d, /transitions\.js:12:3/); + assert.doesNotMatch(d, /screentinker\.com/, 'no redundant origin eating the character budget'); +}); + +test('a cache-busted asset URL does not smuggle a query string in', () => { + const d = crashDetail('boom', '/player/transitions.js?v=3', 9, 1); + assert.match(d, /transitions\.js:9:1/); + assert.doesNotMatch(d, /\?v=3/); +}); + +test('it fits what the server will actually store', () => { + const long = 'x'.repeat(400); + const d = crashDetail(long, '/player/index.html', 1, 1); + assert.ok(d.length <= 200, 'composed within the limit rather than truncated blindly'); + const stored = sanitizeExitReason('crashed', d); + assert.equal(stored.detail, d, 'survives the server sanitiser unchanged'); +}); + +test('a real location is never faked when there is none', () => { + const d = crashDetail('Script error. (no location — cross-origin script)'); + assert.doesNotMatch(d, /:\d+:\d+/, 'no :0:0 masquerading as a location'); + assert.match(d, /cross-origin/, 'says why instead'); +}); + +test('a missing line/column still yields a usable file name', () => { + const d = crashDetail('boom', '/player/index.html'); + assert.match(d, /index\.html:0:0/, 'the file alone is still worth having'); +}); + +// The player installs TWO window 'error' listeners — an early boot logger and, later, the +// crash beacon. Anchor past crashDetail() so these assertions target the beacon; anchoring on +// the first match reads the boot logger and proves nothing about the beacon. +const BEACON = HTML.slice(HTML.indexOf('function crashDetail(')); +const beaconErrorHandler = BEACON.slice( + BEACON.indexOf("window.addEventListener('error'"), + BEACON.indexOf("window.addEventListener('unhandledrejection'")); +const beaconRejectionHandler = BEACON.slice(BEACON.indexOf("window.addEventListener('unhandledrejection'")); + +test('the handlers are wired to it, not just the helper existing', () => { + assert.ok(beaconErrorHandler.length > 0 && beaconErrorHandler.length < 2000, 'located the beacon handler'); + assert.match(beaconErrorHandler, /ev\.filename/, 'the error handler reads filename'); + assert.match(beaconErrorHandler, /ev\.lineno/, 'and lineno'); + assert.match(beaconErrorHandler, /ev\.colno/, 'and colno'); + assert.match(beaconErrorHandler, /crashDetail\(/, 'and composes through the helper'); + assert.match(beaconRejectionHandler.slice(0, 900), /\.stack/, + 'the rejection handler falls back to a stack frame, having no filename of its own'); +}); + +test('a resource load failure is still not a crash', () => { + // Guarding pre-existing behaviour: an that 404s must not report the player dead. + assert.match(beaconErrorHandler, /isResourceError/); + assert.match(beaconErrorHandler, /if \(isResourceError\) return;/); +});