fix(extension): close the frame-registry gaps found in a manual audit

Read the whole target workflow end to end rather than re-running the suite.
Five defects, none of which the existing tests could have caught.

A srcless iframe resolves to its parent document's URL, so a hidden ad slot
could mark the page containing it as hidden and exclude the real player. The
hidden-frame filter now requires the frame element to have actually carried a
src, and ignores any frame claiming the href of the document that reported it.

The frame registry was fed only by incoming sender.frameId, which is empty
during the first activation — exactly when the all-frames sweep needs a
fallback. The resolver now reports every frame it reached and those ids are
recorded before anything is injected.

Monitor injection and deactivation both went through the sweep alone. A
rejected sweep therefore left deep frames without a monitor (so they could
never report themselves, keeping the registry empty) and, on the way out, left
stale monitors reporting after a target switch. Both now address known frames
individually as well.

Registry eviction skipped when the oldest entry was the top frame, so the set
could grow without bound, and a cap of 64 meant up to 64 individual probes —
the same cost the removed 0..64 sweep had. The cap is 24, eviction always
removes a non-top frame, and a committed navigation drops the tab's ids so dead
frames from the previous page are not probed forever.

The stuck-activation watchdog only ran inside GET_STATUS, so it never fired
while the popup was closed — the one situation it exists for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Timo
2026-08-18 16:25:19 +02:00
parent 68f2d9f27c
commit 414be96432
4 changed files with 147 additions and 15 deletions
+47 -8
View File
@@ -91,6 +91,7 @@ let activeTargetActivation = null;
// through webNavigation; this registry rebuilds the same knowledge from
// sender.frameId, which every content script hands us for free.
const knownFrameIdsByTab = new Map();
const MAX_KNOWN_FRAMES_PER_TAB = 24;
function rememberFrameId(tabId, frameId) {
const normalizedTabId = normalizeTabId(tabId);
@@ -101,11 +102,14 @@ function rememberFrameId(tabId, frameId) {
knownFrameIdsByTab.set(normalizedTabId, frames);
}
frames.add(frameId);
// A tab cannot plausibly hold this many media-bearing frames; cap the set so
// a page that recycles frames forever cannot grow it without bound.
if (frames.size > 64) {
const oldest = frames.values().next().value;
if (oldest !== 0) frames.delete(oldest);
// Every id in here is probed individually when a sweep is rejected, so the
// set is also a cost ceiling. Keep it small: a page with more media-bearing
// frames than this is not a player page, and the sweep still covers the
// normal case. The top frame is never evicted.
while (frames.size > MAX_KNOWN_FRAMES_PER_TAB) {
const evictable = Array.from(frames).find(candidate => candidate !== 0);
if (evictable === undefined) break;
frames.delete(evictable);
}
}
@@ -2262,7 +2266,14 @@ function setPageApiSeekEnabled(enabled) {
}
async function deactivateMediaFrameMonitors(tabId) {
const targets = listMediaFrameScriptTargets(tabId);
// Same reasoning as the injection: a rejected sweep must not leave a monitor
// running in a deep frame, or the old target keeps reporting after a switch.
const targets = [
...listMediaFrameScriptTargets(tabId),
...listKnownFrameIds(tabId)
.filter(frameId => frameId !== 0)
.map(frameId => ({ tabId, frameIds: [frameId] }))
];
await Promise.all(targets.map(async target => {
const documentId = target.documentIds?.[0];
const frameId = target.frameIds?.[0];
@@ -2405,7 +2416,15 @@ function executeScriptWithTimeout(options, timeoutMs = SCRIPT_INJECTION_TIMEOUT_
}
async function injectMediaFrameMonitors(tabId, contentTarget) {
const targets = listMediaFrameScriptTargets(tabId);
// The sweep is best effort; the known frames are addressed individually so a
// rejected sweep cannot leave the deep player frame without a monitor — and
// therefore without any way to report itself later.
const targets = [
...listMediaFrameScriptTargets(tabId),
...listKnownFrameIds(tabId)
.filter(frameId => frameId !== 0)
.map(frameId => ({ tabId, frameIds: [frameId] }))
];
let injectedCount = 0;
await Promise.all(targets.map(async target => {
try {
@@ -2463,6 +2482,13 @@ async function injectContentScript(tabId, {
contentTarget = await resolveMediaContentTarget(chrome, tabId, {
knownFrameIds: listKnownFrameIds(tabId)
});
// The probe reached these frames; remember them before anything is
// injected. Waiting for a content script to message us first would leave
// the registry empty exactly when it is needed most — the first
// activation, before any script exists to report itself.
for (const frameId of contentTarget.discoveredFrameIds || []) {
rememberFrameId(tabId, frameId);
}
if (!isTargetActivationSuperseded(tabId, activationGeneration)
&& activeTargetActivation?.tabId === tabId) {
activeTargetActivation.frameId = contentTarget.frameId;
@@ -3048,6 +3074,9 @@ async function selectedMediaTargetMoved(tabId) {
attempts: 1,
knownFrameIds: listKnownFrameIds(tabId)
});
for (const frameId of resolved.discoveredFrameIds || []) {
rememberFrameId(tabId, frameId);
}
} catch {
// An access-required error must reach the full activation path so the
// popup can surface it.
@@ -3079,7 +3108,9 @@ function refreshCurrentMediaTarget(tabId, { queueIfRunning = false, onlyIfTarget
return mediaTargetRefreshTask;
}
if (activeTargetActivation?.tabId === selectedTabId) {
return Promise.resolve({ status: 'activation_in_progress' });
if (!expireStuckActivation()) {
return Promise.resolve({ status: 'activation_in_progress' });
}
}
const task = (async () => {
@@ -3189,6 +3220,14 @@ async function retryPendingTarget({ expectedRequestId = null, requireGrantedAcce
}
}
chrome.tabs.onUpdated.addListener((tabId, changeInfo) => {
// A committed navigation replaces every frame in the tab. Keeping the old
// ids would mean probing dead frames on every resolve from then on.
if (changeInfo.status === 'loading' && typeof changeInfo.url === 'string') {
forgetFrameIds(tabId);
}
});
if (chrome.permissions?.onAdded?.addListener) {
chrome.permissions.onAdded.addListener((addedPermissions) => {
ensureState().then(async () => {
+23 -6
View File
@@ -167,10 +167,16 @@ export function inspectMediaFrame(expectedVisibilityToken = null) {
const rect = frame.getBoundingClientRect();
const directVisible = elementIsVisible(frame, rect);
const visible = ancestorVisible && directVisible;
// An iframe without src resolves to the *parent* document's URL, so
// record whether the element really carried one. Treating a srcless
// ad slot's href as its own would let a hidden slot mark the page
// that contains it as hidden.
const rawSrc = frame.getAttribute?.('src') || '';
let href = '';
try { href = new URL(frame.src || '', doc.location.href).href; } catch { href = ''; }
embeddedFrames.push({
href,
explicitSrc: rawSrc.trim().length > 0,
origin: (() => { try { return new URL(href).origin; } catch { return null; } })(),
area: Math.max(0, rect.width) * Math.max(0, rect.height),
width: Math.max(0, rect.width),
@@ -347,6 +353,10 @@ function hiddenFrameHrefs(injectionResults) {
for (const entry of Array.isArray(injectionResults) ? injectionResults : []) {
for (const frame of entry?.result?.embeddedFrames || []) {
if (typeof frame?.href !== 'string' || !frame.href) continue;
// Without an explicit src the href is the parent's, not this frame's.
if (frame.explicitSrc !== true) continue;
// A frame cannot testify about the document that reported it.
if (frame.href === entry?.result?.href) continue;
// Any ancestor reporting it visible wins over one reporting it hidden.
visibility.set(frame.href, (visibility.get(frame.href) === true) || frame.visible === true);
}
@@ -442,11 +452,14 @@ function accessRequiredError(access) {
return error;
}
function contentTarget(tabId, selected) {
function contentTarget(tabId, selected, discoveredFrameIds = null) {
const frameId = normalizeFrameId(selected?.frameId);
const documentId = typeof selected?.documentId === 'string' ? selected.documentId : null;
return {
frameId,
// Every frame this probe reached, so the caller can remember them and
// recover directly next time the all-frames sweep is rejected.
discoveredFrameIds: Array.isArray(discoveredFrameIds) ? discoveredFrameIds : [],
documentId,
frameUrl: typeof selected?.result?.href === 'string' ? selected.result.href : null,
hasVideo: !!selected?.result?.bestVideo,
@@ -567,6 +580,7 @@ export async function resolveMediaContentTarget(chromeApi, tabId, {
let missingAccess = null;
let ambiguous = false;
let unresolvedGrantedHost = null;
const discovered = new Set(Array.isArray(knownFrameIds) ? knownFrameIds : []);
for (let attempt = 0; attempt < attempts; attempt++) {
const scriptTargets = listMediaFrameScriptTargets(tabId);
@@ -664,6 +678,9 @@ export async function resolveMediaContentTarget(chromeApi, tabId, {
}
}
for (const entry of results) {
if (Number.isInteger(entry?.frameId)) discovered.add(entry.frameId);
}
const selected = selectMediaFrame(results);
const videoCandidates = results.filter(entry => entry?.result?.bestVideo?.rendered === true
&& entry.result.parentFrameVisible !== false);
@@ -688,7 +705,7 @@ export async function resolveMediaContentTarget(chromeApi, tabId, {
if (selected.result.bestVideo.hasSource
&& selected.result.bestVideo.rendered
&& !shouldPreferMissingAccess(currentMissingAccess, selected)) {
return contentTarget(tabId, selected);
return contentTarget(tabId, selected, Array.from(discovered));
}
}
@@ -701,17 +718,17 @@ export async function resolveMediaContentTarget(chromeApi, tabId, {
}
if (missingAccess) throw accessRequiredError(missingAccess);
if (fallback) return contentTarget(tabId, fallback);
if (fallback) return contentTarget(tabId, fallback, Array.from(discovered));
// A player whose origin is already granted but which never answered is a
// timing problem, not a user decision. Keep the tab selected on its top
// frame so the injected monitor can promote the real player once it loads,
// instead of failing the activation or prompting for nothing.
if (unresolvedGrantedHost) return contentTarget(tabId, null);
if (unresolvedGrantedHost) return contentTarget(tabId, null, Array.from(discovered));
// Several equally-ranked players — anime mirrors, alternative dubs — are a
// normal page layout, not an error. Refusing to activate made those pages
// unusable, and flipping between candidates restarted the target forever.
// Hold the top frame and let the monitor promote the one that starts
// playing, which is the signal that breaks the tie.
if (ambiguous) return { ...contentTarget(tabId, null), ambiguous: true };
return contentTarget(tabId, null);
if (ambiguous) return { ...contentTarget(tabId, null, Array.from(discovered)), ambiguous: true };
return contentTarget(tabId, null, Array.from(discovered));
}
+62
View File
@@ -108,6 +108,63 @@ describe('cross-origin media-frame targeting', () => {
expect(selected.frameId).toBe(0);
});
it('ignores a hidden srcless ad slot that resolves to another frame url', () => {
// An iframe without src resolves to its parent document's URL. A hidden
// ad slot must therefore never be able to declare a real player hidden.
const selected = selectMediaFrame([
frame(0, {
bestVideo: null,
videoCount: 0,
embeddedFrames: [{
href: 'https://player-5.example/embed',
explicitSrc: false,
visible: false,
area: 0,
width: 0,
height: 0,
depth: 1,
mediaHint: false
}]
}),
frame(5, { parentFrameVisible: null })
]);
expect(selected?.frameId).toBe(5);
});
it('drops a player parked inside a collapsed same-origin wrapper', () => {
const selected = selectMediaFrame([
frame(0, {
bestVideo: null,
videoCount: 0,
embeddedFrames: [
{
href: 'https://player-5.example/embed',
explicitSrc: true,
visible: true,
area: 830 * 498,
width: 830,
height: 498,
depth: 2,
mediaHint: true
},
{
href: 'https://player-6.example/embed',
explicitSrc: true,
visible: false,
area: 0,
width: 0,
height: 0,
depth: 2,
mediaHint: true
}
]
}),
frame(5, { parentFrameVisible: null }),
frame(6, { parentFrameVisible: null })
]);
expect(selected?.frameId).toBe(5);
});
it('refuses to guess between equally-ranked frames without visibility evidence', () => {
expect(selectMediaFrame([
frame(3, { parentFrameVisible: null }),
@@ -127,6 +184,9 @@ describe('cross-origin media-frame targeting', () => {
documentId: 'document-8',
frameUrl: 'https://player-8.example/embed',
hasVideo: true,
// Reported back so the caller can address these frames directly when
// a later all-frames sweep is rejected wholesale.
discoveredFrameIds: [0, 8],
scriptTarget: { tabId: 42, documentIds: ['document-8'] }
});
const visibilityDispatches = executeScript.mock.calls.filter(([options]) => (
@@ -153,6 +213,7 @@ describe('cross-origin media-frame targeting', () => {
documentId: null,
frameUrl: null,
hasVideo: false,
discoveredFrameIds: [0, 6],
scriptTarget: { tabId: 42 }
});
});
@@ -492,6 +553,7 @@ describe('embedded player access diagnosis', () => {
documentId: null,
frameUrl: null,
hasVideo: false,
discoveredFrameIds: [0],
scriptTarget: { tabId: 42 }
});
expect(contains).toHaveBeenCalledWith({
+15 -1
View File
@@ -59,6 +59,17 @@ describe('target tab lifecycle', () => {
expect(monitorSource).toContain('element.readyState > 0 ? 1 : 0');
});
it('keeps the frame registry bounded and free of a navigation permission', () => {
expect(backgroundSource).toContain('const MAX_KNOWN_FRAMES_PER_TAB = 24');
// Frame ids are learned, never enumerated through a permission.
expect(backgroundSource).toContain('function rememberFrameId(tabId, frameId)');
expect(backgroundSource).toContain('rememberFrameId(senderTabId, sender?.frameId)');
expect(backgroundSource).toContain('contentTarget.discoveredFrameIds');
// A committed navigation invalidates every id from the previous page.
expect(backgroundSource).toMatch(/changeInfo\.status === 'loading'[\s\S]{0,120}forgetFrameIds\(tabId\)/);
expect(backgroundSource).not.toMatch(/chrome\.webNavigation/);
});
it('bounds every frame probe and verifies withheld origins', () => {
const resolverSource = fs.readFileSync(
path.join(extensionDir, 'media-frame-target.js'),
@@ -90,7 +101,10 @@ describe('target tab lifecycle', () => {
it('uses all-frame probing for cross-origin targets without navigation permissions', () => {
expect(backgroundSource).toContain("files: ['media-frame-monitor.js']");
expect(backgroundSource).toContain('const targets = listMediaFrameScriptTargets(tabId)');
// Monitors must reach the frames we know about, not only whatever the
// all-frames sweep happens to accept — both on the way in and out.
expect(backgroundSource.match(/\.\.\.listMediaFrameScriptTargets\(tabId\),/g)?.length).toBe(2);
expect(backgroundSource.match(/\.\.\.listKnownFrameIds\(tabId\)/g)?.length).toBe(2);
expect(backgroundSource).toContain('One denied widget frame must not block the selected player');
expect(backgroundSource).toContain("navigationError.code = 'media_target_navigated'");
expect(backgroundSource).toContain("{ type: 'MEDIA_MONITOR_DEACTIVATE' }");