Prevent competing GPT first impressions and resize PUC shells - #1079
Prevent competing GPT first impressions and resize PUC shells#1079ChristianPavilonis wants to merge 1 commit into
Conversation
ChristianPavilonis
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| claim = state.slots[elementId] = { | ||
| generation: state.generation, | ||
| slotElementId: elementId, | ||
| element: element, | ||
| owner: "publisher", | ||
| phase: phase, | ||
| expiresAt: Number.POSITIVE_INFINITY, | ||
| publisherAuctions: {}, | ||
| }; |
jevansnyc
left a comment
jevansnyc
left a comment
There was a problem hiding this comment.
I traced the initial SSAT/page-bids path, publisher requestBids path, synthetic refresh path, and Universal Creative response path. The direct single-slot case works. The inline comments cover cases that can replace or suppress the wrong impression, or leave the creative clipped at 1x1.
| const { deliverySlots, suppressedSlots } = publisherDeliverySlots(targetSlots); | ||
| suppressedSlots.forEach(restoreTrustedServerFirstImpressionTargeting); | ||
| const remainingSlots = targetSlots.filter((slot) => !suppressedSlots.has(slot)); | ||
| if (remainingSlots.length === 0) return; |
There was a problem hiding this comment.
When disableInitialLoad is enabled, a late handoff already has suppressPublisherRefresh = true. This return suppresses the correlated Prebid delivery without calling the handoff wrapper, so that flag remains set. The next publisher refresh reaches the handoff wrapper and gets dropped too. Can we consume the handoff flag when this path suppresses the slot, or consolidate these into one suppression mechanism? Please add a test with both wrappers installed.
| .map((unit) => unit.code) | ||
| .filter((code): code is string => typeof code === 'string' && code.length > 0) | ||
| ); | ||
| const firstImpressionTokens = |
There was a problem hiding this comment.
This derives claims from all pbjs.adUnits when opts.adUnits is absent, but native Prebid can scope the auction with opts.adUnitCodes. A request for A will claim B as well, which makes adInit() defer B for the five-second lease even though B was not auctioned. Please filter publisherAdUnitCodes and the ad units used for claim/correlation by opts.adUnitCodes.
| suppressedSlots.forEach(restoreTrustedServerFirstImpressionTargeting); | ||
| const remainingSlots = targetSlots.filter((slot) => !suppressedSlots.has(slot)); | ||
| if (remainingSlots.length === 0) return; | ||
| const forwardedSlots = suppressedSlots.size > 0 ? remainingSlots : slots; |
There was a problem hiding this comment.
forwardedSlots is correct here, but the !auctionSlots.length branch below calls originalRefresh(slots, opts). In a mixed request with one suppressed slot and only excluded remaining slots, that branch puts the suppressed slot back into the GPT request. It should forward forwardedSlots there too, with a regression test for this combination.
| adUnitCode: string; | ||
| expiresAt: number; | ||
| registrationId: number; | ||
| firstImpressionToken?: string; |
There was a problem hiding this comment.
These records are not stamped with navGeneration or the physical element. SPA navigation clears ts.firstImpression, but these module-level maps remain. A late route-A callback can register state that a route-B slot with the same code or ad ID consumes, causing stale targeting to reach GPT without a new auction. Please scope the records to generation and element identity, and reject stale callbacks.
| if (suppress) found.claim.suppressionConsumed = true; | ||
| return suppress; | ||
| } | ||
|
|
There was a problem hiding this comment.
suppressionConsumed is shared by the slot claim even though each auction stores a token-specific suppressDelivery decision. If two auctions start while TS has the claim, the first delivery sets this flag and the second in-flight delivery is allowed through immediately. Auctions registered before the first delivery should keep their token-local suppression; auctions registered afterward can proceed. The token is also deleted at five seconds, so a late callback from that same auction cannot be distinguished from a later refresh after fallback runs.
| height: cachedHeight, | ||
| }) | ||
| ); | ||
| resizeCollapsedCreativeFrame( |
There was a problem hiding this comment.
The stale checks happen inside resizeCollapsedCreativeFrame, after port.postMessage. If this fetch resolves after navigation or bid replacement, the old creative still receives a response, then recordTrustedServerCreativeResponse and the billing beacons run. Move the generation, bid, source, and connectivity checks before postMessage, and return without success or beacons when they fail. The current test at line 4662 expects the stale post and should expect zero posts.
| return; | ||
| } | ||
|
|
||
| const wrapper = frame.iframe.parentElement; |
There was a problem hiding this comment.
This only considers the immediate parent. GPT can produce slot -> __container__ -> iframe with both ancestors constrained to 1x1. Expanding __container__ leaves the slot clipped when it has an explicit size or overflow: hidden. Please walk collapsed ancestors up to the authenticated root while applying the same fixed, sticky, and interstitial guards, and add a browser test with two collapsed ancestors.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
First-impression arbitration across the bootstrap, GPT bundle, and Prebid refresh wrapper, plus authenticated collapsed-shell resizing in the Universal Creative bridge. The ownership model (navigation generation + exact DOM identity + auction token + expiry) is coherent and the resize guard set is thorough. Two blocking issues: the new ownership guard suppresses the stale-targeting sweep for publisher claims, and the render bridge silently lost frame-identity disambiguation for prefix-configured div IDs.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change spans several call sites or needs a matching test change and cannot be auto-applied.
Blocking
🔧 wrench
- Stale TS targeting survives on publisher-claimed slots — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1270 - Render bridge lost frame-identity disambiguation for prefix div IDs — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:262
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note
- Two divergent prefix resolvers can put the two claimants on different elements — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:100 - Overlap isolation is only half-realized — see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1040 - Flipped
disableInitialLoadassertions are now vacuous — see inline atcrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:1964 - Three independent 5000 ms constants, no cross-reference — see inline at
crates/trusted-server-core/src/integrations/gpt_bootstrap.js:105 - Resolver forces style recalc in the
requestBids()hot path — see inline atcrates/trusted-server-js/lib/src/core/first_impression.ts:105 - Bootstrap and bundle prune rules differ — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:59
👍 praise
- Collapsed-shell resize guard set and its tests — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:283
Verification performed for this review
Both suggestions were applied in an isolated reviewer worktree at this head and checked in isolation and as a batch:
prettier --check src/integrations/gpt/index.ts— cleannpx vitest run— 901/901 passednode build-all.mjs— 13 modules builtcargo check-fastly— cleancargo fmt --all -- --check— cleancargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt— 47/47 passed- Post-verify patch snapshots byte-identical before and after every verification run
CI Status
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- vitest: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
| const elementId = gptSlot.getSlotElementId(); | ||
| if (!prevTouchedDivIds.has(elementId)) return; | ||
| const element = document.getElementById(elementId); | ||
| if (element && firstImpressionClaim(ts, element)) return; |
There was a problem hiding this comment.
🔧 wrench — Stale TS targeting survives on publisher-claimed slots.
This guard skips the stale-targeting sweep for any live claim. Every key the sweep clears (TS_BASE_TARGETING_KEYS, i.e. hb_* and ts_initial, plus the prior route's prevSlotTargetingKeys[elementId]) was written by Trusted Server, so a publisher claim on this element has nothing to protect from it — but it now blocks the clear.
onNavigate does delete ts.firstImpression and then await waitForSlotElements(...) before calling adInit(). The new route's publisher requestBids() or GPT slotRequested routinely claims the slot inside that window, so this path is the common case rather than the exception.
Verified in an isolated reviewer worktree at this head with a probe test:
BASELINE (no claim) cleared keys: ["hb_pb","hb_bidder","hb_adid","hb_cache_host","hb_cache_path","ts_initial","ts_route"]
PROBE (publisher claim) cleared keys: []
The publisher's own GPT request then carries the previous route's ts_initial=1 and hb_adid. GAM can serve the TS line item for a bid that is no longer in window.tsjs.bids, the render bridge declines it, and the slot renders blank.
Narrowing the guard to Trusted Server claims keeps the intent (never strip targeting the bootstrap or a fallback pass just applied) and restores the sweep:
| if (element && firstImpressionClaim(ts, element)) return; | |
| if (element && firstImpressionClaim(ts, element)?.owner === 'trusted_server') return; |
Verified with this suggestion applied alone and batched with the other one: prettier --check clean, npx vitest run 901/901, node build-all.mjs 13 modules, cargo check-fastly clean, and the post-verify patch snapshot byte-identical to the approved patch.
| } | ||
|
|
||
| function messageSourceBelongsToAdUnit( | ||
| function sourceFrameForAdUnit( |
There was a problem hiding this comment.
🔧 wrench — The render bridge lost frame-identity disambiguation for prefix-configured div IDs.
Deleting candidateSlotRootsForConfiguredDivId replaced "every prefix-matched root, disambiguated by which one actually owns the requesting frame" with resolveSlotElementByDivId(...), which yields one element or nothing. When a configured prefix matches several live elements and the visibility/geometry tiers cannot separate them, this function and slotFrameForMessageSource both return undefined — and the bridge then declines to answer the Universal Creative request at all, not merely declines to resize the shell.
The PR's own test change documents the loss: uses the requesting frame to resolve a registered APS dynamic slot prefix became does not use the requesting frame to disambiguate a registered APS slot prefix, now asserting markUsed is never called and no creative iframe is created.
The PR description justifies rejecting ambiguous sources for the resize guard; it does not mention narrowing response delivery, which reads as collateral rather than intent. Responsive breakpoint duplicates and below-fold lazy slots are exactly the several-prefix-matches, no-usable-layout case, so this can cost real impressions.
Frame identity is a stronger discriminator than geometry here, and "exactly one prefix-matched root owns this window" is still an authenticated match:
function sourceFrameForAdUnit(
source: MessageEventSource | null,
adUnitCode: string
): MessageSourceFrame | undefined {
const resolved = resolveSlotElementByDivId(adUnitCode).element;
if (resolved) return sourceFrameInRoots(source, candidateSlotRoots(resolved.id));
// The resolver declines when a configured prefix matches several live
// elements. Fall back to frame ownership, which still authenticates: accept
// only when exactly one prefix-matched root contains the requesting window.
const owned = Array.from(document.querySelectorAll<HTMLElement>('[id]'))
.filter((el) => el.id.startsWith(adUnitCode) && !el.id.endsWith('-container'))
.map((el) => sourceFrameInRoots(source, candidateSlotRoots(el.id)))
.filter((frame): frame is MessageSourceFrame => frame !== undefined);
return owned.length === 1 ? owned[0] : undefined;
}Apply manually — cannot be expressed as a suggestion: the equivalent fallback is also needed in slotFrameForMessageSource (the inline adm and PBS Cache paths resolve through it), and the flipped test has to move back alongside it.
If the delivery narrowing is intended, it is worth stating in the design doc next to the resize guard, since it changes which slots can render rather than only which shells get resized.
| } | ||
|
|
||
| /** Resolve a publisher ad-unit code to one exact active physical slot element. */ | ||
| export function resolveFirstImpressionElement(adUnitCode: string): HTMLElement | undefined { |
There was a problem hiding this comment.
🤔 thinking — Two divergent prefix resolvers can put the two claimants on different elements.
resolveFirstImpressionElement accepts a prefix match only when matches.length === 1 after a visibleThroughAncestors walk. resolveSlotElementByDivId in integrations/gpt/index.ts has three tiers: unique prefix match, then unique visible match, then unique match with layout.
They disagree whenever two prefix matches are both visible but only one has layout: adInit resolves element A and claims it, while registerPublisherFirstImpressionAuctions resolves nothing and registers no publisher claim. Trusted Server then claims A with no arbitration and suppresses nothing — the duplicate-impression case this PR exists to close.
Sharing one resolver would close the gap. That means either exporting resolveSlotElementByDivId from the GPT bundle or moving it into core/, which is a larger change than this PR — but the two-resolver split is worth a comment at minimum so the divergence is a known limitation rather than an accident.
| if (pending.firstImpressionToken) { | ||
| forgetPublisherFirstImpressionToken(pending.adUnitCode, pending.firstImpressionToken); | ||
| } | ||
| removePendingPublisherBidsForCode(pending.adUnitCode); |
There was a problem hiding this comment.
🤔 thinking — Overlap isolation is only half-realized.
pendingPublisherCodes became Map<code, Map<registrationId, PendingPublisherCode>> specifically so overlapping publisher auctions on one ad-unit code stay separate, and the first-impression tokens do stay separate. But consuming one delivery calls removePendingPublisherBidsForCode(pending.adUnitCode) with no registration id, which deletes every other registration for that code plus all of their pending bids. The sibling auction's later delivery then finds no pending state and is treated as an independent refresh.
Passing pending.registrationId would isolate it, matching the nested-map design. I am not proposing that as a fix, because it trades this failure mode for the one the old docstring described: a leftover pending code can match a genuinely independent later refresh and pass it through without a client-side auction. Worth deciding deliberately and recording which way, since the design doc claims overlapping auctions cannot clear each other's state.
| expect(nativeRefresh).toHaveBeenCalledWith([mockSlot]); | ||
| // The slot already spent its first impression above. Changing GPT's | ||
| // initial-load mode must not make a repeated adInit request it again. | ||
| expect(nativeRefresh).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
♻️ refactor — These flipped assertions no longer exercise what the test is named for.
Every repeated adInit() call in this test now returns early on the first-impression ownership guard: the slot already carries a trusted_server claim from the first pass, claimFirstImpressionForTrustedServer returns undefined, the claim owner is not publisher so no fallback is scheduled, and the slot loop returns. slotsToRefresh and newSlots are therefore empty regardless of ts.gptInitialLoadDisabled, so expect(nativeRefresh).not.toHaveBeenCalled() holds no matter what the initial-load detector recorded.
Clearing ts.firstImpression (or bumping ts.navGeneration) before each repeated adInit() would keep each pass asserting the initial-load coupling instead of the ownership guard.
Not a coverage hole: the first-adInit() coupling is still covered at :1737 and in the bootstrap equivalent at :1786. This is coverage thinning in a previously-regressed area, so it seems worth keeping sharp.
| pubads.__tsInitialLoadHooked = true; | ||
| }); | ||
|
|
||
| var FIRST_IMPRESSION_LEASE_MS = 5000; |
There was a problem hiding this comment.
⛏ nitpick — Three independent 5000 ms constants with no cross-reference.
FIRST_IMPRESSION_LEASE_MS is duplicated here and in crates/trusted-server-js/lib/src/core/first_impression.ts:10, and it has to match PENDING_PUBLISHER_DELIVERY_TTL_MS in crates/trusted-server-js/lib/src/integrations/prebid/index.ts:140 for the token timers to line up. The diff also dropped the existing // Keep in sync with TS_INITIAL_TARGETING_KEY in index.ts comment when ts_initial moved into bootstrapTargeting, so the bootstrap now has no sync markers at all.
| var FIRST_IMPRESSION_LEASE_MS = 5000; | |
| // Keep in sync with FIRST_IMPRESSION_LEASE_MS in | |
| // crates/trusted-server-js/lib/src/core/first_impression.ts, and with | |
| // PENDING_PUBLISHER_DELIVERY_TTL_MS in | |
| // crates/trusted-server-js/lib/src/integrations/prebid/index.ts, which bounds | |
| // the publisher-auction tokens this lease arbitrates against. | |
| var FIRST_IMPRESSION_LEASE_MS = 5000; |
Verified with this suggestion applied alone and batched with the other one: cargo fmt --all -- --check clean, cargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt 47/47, and the post-verify patch snapshot byte-identical to the approved patch. Note GPT_BOOTSTRAP_JS is injected unminified via format!("<script>{}</script>", ...), so this adds roughly 250 bytes to the inline head script — consistent with the comment density already in this file.
| const exact = activePhysicalElement(document.getElementById(adUnitCode)); | ||
| if (exact) return exact; | ||
|
|
||
| const matches = Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter( |
There was a problem hiding this comment.
⛏ nitpick — This fallback forces a style recalc in the publisher's requestBids() path.
registerPublisherFirstImpressionAuctions calls resolveFirstImpressionElement once per ad-unit code, and every code that misses the getElementById fast path runs querySelectorAll('[id]') and then visibleThroughAncestors, which calls window.getComputedStyle for each element up each candidate's ancestor chain. On a page with many IDs that is a synchronous layout flush per publisher auction, on the critical path of the publisher's own bid request.
Caching the resolution per navigation generation, or narrowing the candidate query, would keep the arbitration cost off that path.
| state.slots ??= {}; | ||
| state.fallbackSlots ??= {}; | ||
| for (const [elementId, claim] of Object.entries(state.slots)) { | ||
| if (!claimMatchesElement(claim, claim.element, generation)) { |
There was a problem hiding this comment.
📝 note — The bootstrap and bundle prune rules differ, so they can disagree about whether a claim is live.
The bootstrap's firstImpressionState prunes a claim when claim.element !== document.getElementById(elementId). claimMatchesElement here is called from pruneFirstImpressionState as claimMatchesElement(claim, claim.element, generation), so its claim.element === element check compares the element to itself and the document-identity check is effectively absent. On a duplicate-ID DOM — or when a still-connected old element shares an ID with a newly inserted one — the bootstrap drops the claim and the bundle keeps it.
The bundle also bounds state with MAX_FIRST_IMPRESSION_SLOTS and MAX_PUBLISHER_AUCTIONS_PER_SLOT, while the bootstrap has no caps. Both are practically bounded by the DOM, but the design doc says the two implementations use the same contract, so the differences are worth either closing or documenting.
|
|
||
| const MAX_CREATIVE_SHELL_DIMENSION = 10_000; | ||
|
|
||
| /** Resize only the authenticated source iframe for a still-current collapsed display shell. */ |
There was a problem hiding this comment.
👍 praise — The guard set on this helper is genuinely careful.
It checks the width/height attributes and computed geometry, rejects fixed/sticky on both the iframe and its wrapper, walks out to anchor / interstitial / vignette ancestors, caps dimensions at MAX_CREATIVE_SHELL_DIMENSION, pins the navigation generation, re-verifies iframe.contentWindow === source and containment, and takes a stillOwnsCreative callback that re-resolves the frame after the response was posted. Mutating publisher DOM is the risky part of this PR and this is the right shape for it.
The tests match: the four-way it.each over fixed / anchor / expanded / oversized, the stale-cache-response-after-navigation case, and the assertion that a failed postMessage leaves the shell at 1x1.
4 participants
Stack
This child PR depends on #1070 and uses
esi-edge-terminated-auth-mainas its base. The parent PR's ESI and authentication changes are unchanged.Closes #1078.
What changed
The first valid claimant now owns each physical slot's first impression for the current navigation. A publisher
requestBids()call, GPT request, or GPT render prevents delayed page-bids data from retargeting and refreshing that slot. If Trusted Server claims first, the Prebid refresh wrapper filters one correlated losing publisher delivery, restores the TS targeting snapshot, and then allows later publisher refresh auctions.The ownership state is bounded by navigation generation, exact DOM identity, auction token, and expiry. Overlapping auctions keep separate tokens. An abandoned publisher claim gets at most one per-slot TS fallback after the lease expires. The bootstrap and full GPT bundle use the same contract, and ownership listeners do not depend on diagnostics.
The Universal Creative bridge now replaces the guarded resize that it suppresses in Prebid. After a TS response is posted successfully, it expands only the authenticated source iframe and its immediate collapsed parent from 1x1 to the validated winning dimensions. The guard rejects ambiguous or stale sources, invalid dimensions, anchors, interstitials, fixed or sticky shells, and already-expanded frames. It covers server APS, client-side APS capabilities, inline
adm, and PBS Cache responses.publisher_nativekeeps its separate replacement path.The design and APS guide now document first-claimant ownership, one-shot losing-delivery suppression, and authenticated shell resizing. Strict TS-first delivery remains a separate design choice.
Validation
cargo fmt --all -- --checkcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycd crates/trusted-server-js/lib && npx vitest run(901 passed)cd crates/trusted-server-js/lib && npm run lint && npm run format && node build-all.mjscd docs && npm run formatcd crates/trusted-server-integration-tests/browser && npx playwright test tests/shared/aps-renderer.spec.ts --project=chromium(5 passed, including real Prebid Universal Creative geometry and terminal render evidence)Generated TSJS distribution files remain ignored and are not committed.
Residual risks and rollback
The five-second lease intentionally bounds first-impression arbitration. Publisher flows that do not request or render within that interval can yield the untouched slot to the one-shot TS fallback. The shell resize also declines unusual or ambiguous GAM wrapper layouts rather than mutating them.
Rollback does not require a merge or deployment from this PR. Operators can disable the server-side opportunity path with
creative_opportunities.enabled = falsewhile reverting the child change. No stack PR is merged by this submission.