Core: add public removeUserSyncs() to remove user sync iframes - #15380
Core: add public removeUserSyncs() to remove user sync iframes#15380Pubstream wants to merge 3 commits into
Conversation
Barecheck - Code coverage reportTotal: 96.59%Your code coverage diff: 0.00% ▴ Uncovered files and lines
|
we set our sync frame limits to one, do you do the same? it seems like a bug they are accumulating indefinitely do you think we should automatically clean up the dom? |
| * @return {Number} the number of iframes that were removed | ||
| */ | ||
| export function removeUserSyncIframes() { | ||
| const iframes = document.querySelectorAll(`iframe[${USERSYNC_ATTR}]`); |
There was a problem hiding this comment.
This will cause side effects when multiple pbjs instances are on the same page.
There was a problem hiding this comment.
Sync iframes are now tagged with the name of the Prebid global that inserted them
(data-pb-usersync="pbjs"), and removeUserSyncs() only removes iframes matching its own
instance, so pbjs and pbjs2 no longer interfere with each other. Test added for it.
Tag each sync iframe with the name of the Prebid global that inserted it, so that removeUserSyncs() only removes its own iframes when several instances run on the same page.
patmmccann
changed the title
patmmccann
changed the title
|
This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:
The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers. |
| // the attribute value is not necessarily a valid CSS identifier, so it's matched here instead of | ||
| // in the selector | ||
| const iframes = Array.from(document.querySelectorAll(`iframe[${USERSYNC_ATTR}]`)) | ||
| .filter(iframe => iframe.getAttribute(USERSYNC_ATTR) === getGlobalVarName()); |
There was a problem hiding this comment.
it's possible for this to remove the wrong frames - if one builds prebid with defineGlobal: false, getGlobalVarName() still returns something - most likely the default pbjs - which could be used by another prebid instance on the page.
Would keeping track of the iframe elements themselves with a WeakSet work? if not, it'd be safer to generate a random identifier on startup to use as a tag.
There was a problem hiding this comment.
Alright, changed to WeakSet which track the real iframe elements
|
please pull master, your e2e tests ran on the wrong safari |
I’m not sure what you mean by "wrong Safari", how can I control that? And which one would be the “right” Safari? |
|
if you pull master, the e2e tests will run on a different version of safari bc the location of the safari binary it was trying to download for test runs failed |
|
see #15422 |
4 participants
Type of change
Description of change
User sync iframes are appended to and never cleaned up, so in a single page application they keep accumulating for as long as the user stays on the page without a reload.
Every sync iframe is now marked with a
data-pb-usersyncattribute when it is inserted, andpbjs.removeUserSyncs()removes all of them from the document, returning how many were removed. AsinsertUserSyncIframeis the single entry point for sync iframes, this also covers the syncs fired by the Prebid Server adapter and by modules inserting their own frames.Removal is a pure DOM cleanup: the sync queue and the userSync config are left untouched, so syncs registered afterwards insert new iframes as usual and the call can be repeated on every route change. Image pixels are unaffected, they never enter the DOM.