mirror of
https://github.com/got-feedBack/feedBack-desktop.git
synced 2026-09-10 22:14:10 +00:00
fix(renderer): roll back the mute-suppression latch when the IPC fails
CodeRabbit caught a real bug in the previous commit's fix. The latch mirrors the NATIVE refcount, but it was flipped before the invoke resolved: a rejected release left it reading "released" while the engine still held the suppression, so every later release short-circuited and monitor mute stayed suppressed for good — the same stuck-suppression bug the latch exists to prevent, just one level up. The latch now only stays flipped if the call actually landed, and rolls back otherwise (guarded so a newer call can't be clobbered by a stale rejection). A downlevel addon with no arbiter leaves the latch untouched instead of recording a hold it never acquired. Pins the whole contract with a vm-extracted unit test on the real screen.js function: unpaired acquires hold at most one native suppression, cycles stay balanced across 25 song loads, a rejected release retries, and both the downlevel and sync-throw paths are clean. Fails 3/5 against the original branch (the refcount leak) and 2/5 against the pre-rollback version.
This commit is contained in:
byrongamatos
+18
-7
@@ -4390,16 +4390,27 @@ window.__feedBackDesktopAudioHooks = window.__feedBackDesktopAudioHooks || {};
|
||||
function aeSetMonitorMuteSuppressed(suppressed) {
|
||||
const want = !!suppressed;
|
||||
if (want === aeMonitorMuteSuppressionHeld) return; // idempotent, like the old bool
|
||||
aeMonitorMuteSuppressionHeld = want;
|
||||
const api = window.feedBackDesktop?.audio;
|
||||
// Optional-chained: a downlevel native addon simply ignores this.
|
||||
// Downlevel addon (no arbiter): nothing is ever acquired, so leave the
|
||||
// latch alone rather than recording a hold we don't have.
|
||||
if (typeof api?.setMonitorMuteSuppressed !== 'function') return;
|
||||
aeMonitorMuteSuppressionHeld = want;
|
||||
// The latch mirrors the NATIVE refcount, so it may only stay flipped if
|
||||
// the call actually landed. A rejected release that left the latch at
|
||||
// "released" would short-circuit every later release while the native
|
||||
// count stayed held — the same stuck-suppression bug, one level up. Roll
|
||||
// back on failure so the next call retries (and only if no newer call
|
||||
// has moved the latch on in the meantime).
|
||||
const rollback = () => {
|
||||
if (aeMonitorMuteSuppressionHeld === want) aeMonitorMuteSuppressionHeld = !want;
|
||||
};
|
||||
// setMonitorMuteSuppressed is async (ipcRenderer.invoke) — the sync
|
||||
// try/catch only covers a missing method, so also swallow the
|
||||
// returned promise's rejection to avoid an unhandled rejection.
|
||||
// try/catch only covers a throwing call, so handle the returned
|
||||
// promise's rejection too (which also avoids an unhandled rejection).
|
||||
try {
|
||||
const r = api?.setMonitorMuteSuppressed?.(want);
|
||||
if (r && typeof r.catch === 'function') r.catch(() => {});
|
||||
} catch (_) { /* downlevel */ }
|
||||
const r = api.setMonitorMuteSuppressed(want);
|
||||
if (r && typeof r.catch === 'function') r.catch(rollback);
|
||||
} catch (_) { rollback(); }
|
||||
}
|
||||
// Called by clearChainForNewSong (IIFE 1) and the preload below.
|
||||
window._aeBeginChainRebuildGuard = function () { aeSetMonitorMuteSuppressed(true); };
|
||||
|
||||
@@ -0,0 +1,174 @@
|
||||
// PR #107 review: the native monitor-mute arbiter REFCOUNTS suppressions
|
||||
// (SourceChain::setMonitorMuteSuppressed — true = acquire, false = release),
|
||||
// but it kept the old boolean signature. The renderer's rebuild guard is
|
||||
// deliberately unpaired: resolveChainRebuildGuard() leaves the suppression on
|
||||
// when a rebuild produced an empty chain, and returns early without releasing
|
||||
// while a provider route is still resolving. Against the old LATCHED BOOL that
|
||||
// was self-correcting (repeated trues were idempotent, any false reset it);
|
||||
// against a refcount every unpaired call is a permanent +1, so after a couple
|
||||
// of song loads the count can never return to zero and monitor mute is silently
|
||||
// dead for the rest of the session.
|
||||
//
|
||||
// aeSetMonitorMuteSuppressed() therefore holds AT MOST ONE native suppression.
|
||||
// These cases pin that, plus the rollback: the latch mirrors the native
|
||||
// refcount, so it may only stay flipped if the IPC actually landed.
|
||||
|
||||
'use strict';
|
||||
|
||||
const test = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('node:path');
|
||||
const fs = require('node:fs');
|
||||
const vm = require('node:vm');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const SCREEN_JS = fs.readFileSync(path.join(ROOT, 'src', 'renderer', 'screen.js'), 'utf8');
|
||||
|
||||
function extractFunction(src, name) {
|
||||
const sig = `function ${name}(`;
|
||||
const start = src.indexOf(sig);
|
||||
assert.ok(start !== -1, `function '${name}' not found`);
|
||||
let i = src.indexOf('{', src.indexOf(')', start));
|
||||
let depth = 1;
|
||||
i++;
|
||||
while (i < src.length && depth > 0) {
|
||||
if (src[i] === '{') depth++;
|
||||
else if (src[i] === '}') depth--;
|
||||
i++;
|
||||
}
|
||||
assert.ok(depth === 0, `unbalanced braces in '${name}'`);
|
||||
return src.slice(start, i);
|
||||
}
|
||||
|
||||
// Build a sandbox with the real function plus a fake native api that records
|
||||
// every acquire/release and can be made to fail. `calls` is the ground truth
|
||||
// for what the native refcount would have done.
|
||||
function makeHarness({ mode = 'ok' } = {}) {
|
||||
const calls = [];
|
||||
const setMonitorMuteSuppressed = (suppressed) => {
|
||||
if (mode === 'throw') { calls.push({ suppressed, outcome: 'threw' }); throw new Error('sync boom'); }
|
||||
if (mode === 'reject') {
|
||||
calls.push({ suppressed, outcome: 'rejected' });
|
||||
return Promise.reject(new Error('ipc boom'));
|
||||
}
|
||||
calls.push({ suppressed, outcome: 'ok' });
|
||||
return Promise.resolve();
|
||||
};
|
||||
const audio = mode === 'downlevel' ? {} : { setMonitorMuteSuppressed };
|
||||
const ctx = {
|
||||
window: { feedBackDesktop: { audio } },
|
||||
// The native refcount, simulated: clamped at 0 exactly like SourceChain's
|
||||
// compare_exchange loop, so an unpaired release can't underflow.
|
||||
nativeCount: 0,
|
||||
};
|
||||
vm.createContext(ctx);
|
||||
vm.runInContext(
|
||||
'let aeMonitorMuteSuppressionHeld = false;\n'
|
||||
+ extractFunction(SCREEN_JS, 'aeSetMonitorMuteSuppressed')
|
||||
+ '\nglobalThis.__call = aeSetMonitorMuteSuppressed;'
|
||||
+ '\nglobalThis.__held = () => aeMonitorMuteSuppressionHeld;',
|
||||
ctx,
|
||||
);
|
||||
return {
|
||||
calls,
|
||||
set: (v) => ctx.__call(v),
|
||||
held: () => ctx.__held(),
|
||||
// Replay the recorded calls through the native refcount semantics.
|
||||
nativeCount: () => calls.reduce((n, c) => {
|
||||
if (c.outcome !== 'ok') return n; // never reached the engine
|
||||
return c.suppressed ? n + 1 : Math.max(0, n - 1);
|
||||
}, 0),
|
||||
};
|
||||
}
|
||||
|
||||
const flush = () => new Promise((r) => setImmediate(r));
|
||||
|
||||
test('repeated unpaired acquires hold at most ONE native suppression', async () => {
|
||||
const h = makeHarness();
|
||||
// Three song loads whose guard never releases (empty-chain / provider-pending
|
||||
// branches). Under a raw refcount this would be +3 and never recoverable.
|
||||
h.set(true); h.set(true); h.set(true);
|
||||
await flush();
|
||||
assert.equal(h.calls.filter((c) => c.suppressed).length, 1, 'only one acquire may reach the engine');
|
||||
assert.equal(h.nativeCount(), 1);
|
||||
|
||||
// ...and one release still returns the count to zero, so monitor mute works.
|
||||
h.set(false);
|
||||
await flush();
|
||||
assert.equal(h.nativeCount(), 0, 'a single release must fully un-suppress');
|
||||
assert.equal(h.held(), false);
|
||||
});
|
||||
|
||||
test('acquire/release cycles stay balanced across many song loads', async () => {
|
||||
const h = makeHarness();
|
||||
for (let i = 0; i < 25; i++) {
|
||||
h.set(true); // clearChainForNewSong + preload both call the guard
|
||||
h.set(true);
|
||||
await flush();
|
||||
h.set(false); // resolveChainRebuildGuard
|
||||
await flush();
|
||||
}
|
||||
assert.equal(h.nativeCount(), 0, 'refcount must not drift across sessions');
|
||||
assert.equal(h.held(), false);
|
||||
});
|
||||
|
||||
test('a rejected release rolls the latch back so the next release retries', async () => {
|
||||
// The bug this guards: if the latch flipped to "released" on an IPC that
|
||||
// never landed, every later release would short-circuit while the native
|
||||
// count stayed held — stuck suppression, one level up from the C++ leak.
|
||||
const calls = [];
|
||||
let failNext = false;
|
||||
const ctx = {
|
||||
window: {
|
||||
feedBackDesktop: {
|
||||
audio: {
|
||||
setMonitorMuteSuppressed: (s) => {
|
||||
if (failNext) { calls.push({ suppressed: s, outcome: 'rejected' }); return Promise.reject(new Error('boom')); }
|
||||
calls.push({ suppressed: s, outcome: 'ok' });
|
||||
return Promise.resolve();
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
vm.createContext(ctx);
|
||||
vm.runInContext(
|
||||
'let aeMonitorMuteSuppressionHeld = false;\n'
|
||||
+ extractFunction(SCREEN_JS, 'aeSetMonitorMuteSuppressed')
|
||||
+ '\nglobalThis.__call = aeSetMonitorMuteSuppressed;'
|
||||
+ '\nglobalThis.__held = () => aeMonitorMuteSuppressionHeld;',
|
||||
ctx,
|
||||
);
|
||||
|
||||
ctx.__call(true); // acquire lands: native = 1
|
||||
await flush();
|
||||
assert.equal(ctx.__held(), true);
|
||||
|
||||
failNext = true;
|
||||
ctx.__call(false); // release REJECTS: native still 1
|
||||
await flush();
|
||||
assert.equal(ctx.__held(), true, 'a failed release must not leave the latch "released"');
|
||||
|
||||
failNext = false;
|
||||
ctx.__call(false); // retry must actually be attempted
|
||||
await flush();
|
||||
const releases = calls.filter((c) => !c.suppressed);
|
||||
assert.equal(releases.length, 2, 'the retry must reach the engine, not short-circuit');
|
||||
assert.equal(releases.at(-1).outcome, 'ok');
|
||||
assert.equal(ctx.__held(), false);
|
||||
});
|
||||
|
||||
test('a downlevel addon without the arbiter is a clean no-op', async () => {
|
||||
const h = makeHarness({ mode: 'downlevel' });
|
||||
assert.doesNotThrow(() => { h.set(true); h.set(false); });
|
||||
await flush();
|
||||
assert.equal(h.calls.length, 0);
|
||||
assert.equal(h.held(), false, 'nothing was acquired, so nothing may be recorded as held');
|
||||
});
|
||||
|
||||
test('a synchronously throwing bridge rolls the latch back', async () => {
|
||||
const h = makeHarness({ mode: 'throw' });
|
||||
assert.doesNotThrow(() => h.set(true));
|
||||
await flush();
|
||||
assert.equal(h.held(), false, 'a throw means nothing was acquired');
|
||||
});
|
||||
Reference in New Issue
Block a user