Core: RTD modules read stale geometry, and the two geometry caches invalidate independently

[dgirardi]

Type of issue

Bug (core / libraries). Two related defects in how cached layout reads are invalidated: one active, one latent.

Description

Prebid has two memoized sources of layout data, with unrelated invalidation policies:

• libraries/boundingClientRect/boundingClientRect.js caches getBoundingClientRect() per element and clears the cache from a startAuction.before hook — one measurement per element per auction.
• getWinDimensions() (src/utils/winDimensions.js) caches viewport properties — including document.documentElement.scrollTop / scrollLeft and the body equivalents — behind a 20 ms wall-clock TTL.

Both behave as intended for bid adapters. The first is wrong for RTD modules (problem 1, active). The second exposes continuously-varying scroll offsets through a cache tuned for values that change on resize, which makes it unsafe to combine with a rect (problem 2, latent).

Problem 1 — the rect cache is cleared after RTD runs, so RTD modules read the previous auction's layout

fun-hooks runs before hooks in descending priority order (node_modules/fun-hooks/no-eval/index.js:192), default priority 10 (:181).

• RTD registers at priority 20 — modules/rtdModule/index.ts:126
• the rect cache reset registers at the default 10 — libraries/boundingClientRect/boundingClientRect.js:5

RTD submodules therefore run before the cache is cleared for the auction they are contributing to, and an RTD module reading through the shared helper receives whatever was cached during the previous auction. setBidRequestsData additionally holds fn.call(...) until its providers finish or auctionDelay expires, so the reset lands at the end of the RTD wait rather than the start.

Observing it requires a second consumer: RTD's own read populates the cache, but the reset wipes it later in the same auction, so a stale entry has to come from a post-reset consumer of the same element in the previous auction — any bid adapter using the rect helper on the slot div, or any percentInView() call taking the percentInViewStatic path (which caches via getBoundingBox). Once such a module is bundled, every auction after the first is affected.

Two shipped RTD modules hit this, both while computing slot position for the bid request:

• modules/adagioRtdProvider.js — onGetBidRequestData (:340) → getSlotPosition (:365, :497) → getBoundingClientRect (:534); also getElementFromTopWindow (:482-483)
• modules/contxtfulRtdProvider.js — getBidRequestData (:406) → getAdUnitPositions (:422, :386) → tryGetAdUnitPosition → tryMultipleDivIdPositions → tryGetDivIdPosition → getDivIdPosition (:271) → getBoundingClientRect (:308); also :258-259

Neither wraps the read in an eslint exception, so this is silent.

The consequence is worse than staleness. Both modules convert the rect to document-relative coordinates by adding a live scroll offset — adagio wt.pageYOffset (:541-542), contxtful wt.scrollY (:313-314) — to a cached rect. The two halves come from different instants, so the emitted position.x / position.y is wrong by the distance scrolled since the previous auction and describes no layout state that ever existed. The error is unbounded in page height, not bounded by a cache interval, and it appears in a quantity that is supposed to be scroll-invariant by construction.

libraries/percentInView/percentInView.js:198 registers its intersection-warming hook at the default priority 10 as well, so it also runs after RTD. An RTD module calling percentInView() finds no observed intersection on the first auction and falls through to percentInViewStatic, i.e. onto the same stale rect cache.

Other consumers are unaffected: every other module importing the rect helper is a bid adapter reading in buildRequests (post-reset), and libraries/placementPositionInfo is used only by bid adapters. modules/rules/index.ts (priority 50) and modules/userId/index.ts (priority 100) run pre-reset but read no geometry.

Problem 2 — getWinDimensions() memoizes scroll offsets, which cannot safely be combined with a rect

CachedApiWrapper reads a property once and memoizes it until reset() (src/utils/cachedApiWrapper.js:11-18), and getWinDimensions() only calls reset() once more than CHECK_INTERVAL_MS = 20 has elapsed (src/utils/winDimensions.js:4, :43-45). A 20 ms TTL is a reasonable fit for the rest of that object — innerHeight, innerWidth, screen.*, clientWidth/Height all change on resize, which is rare and slow. Scroll offsets are different in kind: they change continuously, at up to thousands of CSS pixels per second during a fling.

Exposing scrollTop / scrollLeft through the same cache means any caller computing rect.top + getWinDimensions()…scrollTop combines two different sampling instants, and the resulting error is proportional to scroll velocity — so it appears only while the user is scrolling, which is exactly when a document-relative coordinate must not drift.

Nothing on master uses the memoized offsets as its primary source today, so this is currently latent rather than broadly broken. But it is reachable now through ||-on-zero fall-through, because a genuine scroll offset of 0 is falsy:

• modules/adagioRtdProvider.js:541-542 — wt.pageYOffset || windowDimensions.document.documentElement.scrollTop || windowDimensions.document.body.scrollTop. At the top of the page the live value is 0, so the memoized value is used, and it can be non-zero if the user has just scrolled back to the top within the last 20 ms. position.y is then off by the pre-scroll offset.
• modules/nextMillenniumBidAdapter.js:347 — window.pageYOffset || getWinDimensions().document.documentElement.scrollTop, same fall-through, reported as imp.ext.nextMillennium.scrollTop.

modules/contxtfulRtdProvider.js:313-314 avoids it by using ?? and by reading wt.document.documentElement directly rather than the wrapper.

The API shape is the underlying problem: getWinDimensions().document.documentElement.scrollTop reads like a live DOM access, so combining it with a rect is the natural thing to write. That the two modules above both reached for || guards, and that new code continues to reach for the memoized path (see Related), suggests the trap is worth closing rather than documenting.

Steps to reproduce

Problem 1:

1. gulp build --modules=rtdModule,adagioRtdProvider,pubmaticBidAdapter — an RTD module that measures slot geometry, plus a bid adapter that reads the same slot's rect through the shared helper.
2. Configure realTimeData with the adagio provider and a non-zero auctionDelay.
3. Run an auction on a page tall enough to scroll.
4. Scroll so the ad slot moves, then call pbjs.requestBids() again.
5. Inspect ortb2Imp.ext.data.adg_rtd.adunit_position on the second auction's bid requests, and compare against document.getElementById(divId).getBoundingClientRect().top + window.scrollY read from the console at the same moment.

Problem 2:

1. Same build, page scrolled to a non-zero offset.
2. Scroll back to exactly the top and trigger an auction within 20 ms of arriving there (a programmatic window.scrollTo(0, 0) immediately followed by pbjs.requestBids() reproduces it deterministically).
3. Inspect the reported position: the y component carries the pre-scroll offset instead of 0.

Expected results

• Problem 1: an RTD module measuring a slot during getBidRequestData sees that slot's current geometry, and any derived document-relative coordinate is computed from a single sampling instant.
• Problem 2: scroll offsets used to derive document-relative coordinates reflect the current scroll position.

Actual results

• Problem 1: the RTD module reads a rect cached during the previous auction and adds a live scroll offset to it, producing coordinates that match no actual layout state and that drift by the distance scrolled between auctions.
• Problem 2: at scroll offset 0, a memoized offset up to 20 ms old is substituted for the live one.

Platform details

Prebid.js 11.26.0-pre (master). Not browser- or OS-specific: both defects follow from hook priority and cache invalidation policy rather than any browser behavior. Hook ordering per node_modules/fun-hooks/no-eval/index.js:181,192.

Other information

Why raising the reset hook's priority is not sufficient on its own

Passing a priority above 20 to startAuction.before in boundingClientRect.js fixes problem 1 for RTD but inverts it for bid adapters: because the reset currently runs at the end of RTD's auctionDelay wait, moving it above RTD makes RTD-era reads the values adapters consume after the wait. With auctionDelay: 300, adapters would build requests from geometry up to 300 ms old. Clearing at both ends — registering the clearing hook twice, or having RTD clear after its wait — avoids the trade-off. Note that priority 100 would tie with modules/userId/index.ts:1159, leaving the order decided by module load order.

Suggested direction

Problem 1: decouple the rect cache from the auction lifecycle and give it a layout-accurate epoch. A rect can only change through a layout, and layout is flushed per frame, so clearing on a one-shot requestAnimationFrame scheduled at first cache population gives:

• a coherent snapshot within a frame — the property the per-auction cache approximates, but exact;
• guaranteed freshness across frames, so hook order stops mattering and a 300 ms auctionDelay (~18 frames) cannot serve stale geometry;
• one measurement per element per frame, preserving the forced-reflow reduction that motivated the library (getBoundingClientRect and percent Inview library: replace layout calculations in various adapters #12848, a07bf0854);
• no startAuction import in a leaf geometry library, and no import-time hook registration as a side effect.

Two caveats: within a single frame, code that mutates layout and then reads still gets the pre-mutation rect (narrow — RTD modules and adapters do not insert DOM between reads); and requestAnimationFrame does not fire in background tabs, so it should be paired with a wall-clock backstop, whichever fires first.

Problem 2: the smallest correct fix is to stop memoizing scroll offsets — drop scrollTop / scrollLeft from the CachedApiWrapper spec in winDimensions.js and read them live, since they are the only continuously-varying members of that object. Alternatively, expose a helper that samples a rect and the scroll offset together and returns document-relative coordinates, so callers cannot mix instants by accident; libraries/viewport/viewport.js:6 already reads scrollX/scrollY live and may be the right home. Either way the || guards in adagioRtdProvider.js:541-542 and nextMillenniumBidAdapter.js:347 should become ?? so a legitimate 0 stops falling through.

Note the two fixes are independent: problem 1's fix does not address problem 2, and vice versa. If the rect cache moves to a frame epoch and scroll offsets are read live, both halves of a document-relative computation become frame-coherent by construction, which closes the class rather than the instances.

Two smaller items in the same code, worth folding in:

• libraries/boundingClientRect/boundingClientRect.js:3 uses a strong Map keyed by HTMLElement, so detached ad slot elements are retained until the next auction clears the cache. A WeakMap (replaced on expiry, since it has no clear()) avoids this, and matters more once clearing is no longer tied to startAuction.
• the reset hook is registered as an anonymous arrow, so tests cannot target it via getHooks({hook}).remove().

test/spec/libraries/boundingClientRect_spec.js asserts only that some startAuction call clears the cache, so it does not pin the hook's priority. A time- or frame-based strategy would make its "should not fire getBoundingClientRect twice for the same element" case timing-dependent under a loaded Karma run; it would need an injectable clock, as test/spec/utils_spec.js:1464-1486 does for getWinDimensions.

Related

• Both were found while reviewing Unicorn Bid Provider: add slot position & viewability module #15379. That PR hits problem 1 (working around it with an eslint-disable no-restricted-properties direct read, with a code comment naming the stale-cache reason) and walks straight into problem 2 (it derives document-relative coordinates from a live rect plus getWinDimensions() scroll offsets, with no live source at all). It is cited here as evidence that the trap is easy to fall into, not as the cause; neither defect is that PR's responsibility and both are being tracked here.
• The rect helper and percentInView were introduced in getBoundingClientRect and percent Inview library: replace layout calculations in various adapters #12848 to consolidate layout reads previously duplicated across adapters.

_{This issue was generated by Claude.}