Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearing instrumented intervals in 3p environmented frames cancels timers #40235

Open
PsychodelEKS opened this issue Jan 28, 2025 · 3 comments
Open

Comments

@PsychodelEKS
Copy link

PsychodelEKS commented Jan 28, 2025

Developing an amp-ad integration I faced a problem with some of my timeouts not firing up correctly (at all actually), after some extensive investigation I found the native clearInterval call in the instrumented method to be the culprit. In was added in this commit.

In native timers/intervals browser shares the same ids pool between those, so in any arbitrary point in time a pair of a timer and an interval with the same id value can not exist. Thus native clearTimeout and clearInterval native methods do the same - cancel the delayed call by the timerId passed regardless of the timer/interval type, and either a timer or an interval can be cancelled with either of those (the difference of the methods is only in names).

In the AMP lib the instrumented version of intervals uses own iterator to get/set ids for the created intervals and this numbering intersects with the browser built in used for timers. So the introduced call to the native clearInterval method for an interval id for which a native timer was previously created will cancel this timer as a unintended consequence.

Here is a test code I used to demonstrate this (to test locally, I`ve created a fake adm-ad provider and added it to the ads.amp.html page)

/**
 * @param {!Window} global
 * @param {!Object} data
 */
export function timerstest(global, data) {
  let tmr1 = setTimeout(function() { console.log("-- timer 1 fired ("+tmr1+")"); }, 3000);
  console.log("-- timer 1 created ("+tmr1+")");

  let tmr2 = setTimeout(function() { console.log("-- timer 2 fired ("+tmr2+")"); }, 3000);
  console.log("-- timer 2 created ("+tmr2+")");

  let int1Cnt = 1;
  let int1 = setInterval(function() {
    console.log("interval 1 fired ("+int1+", "+(int1Cnt++)+")");

    if (int1Cnt>4) {
      console.log("clearing interval 1 ("+int1+", "+(int1Cnt++)+")");
      global.clearInterval(int1);
    }
  }, 500);
  console.log("interval 1 created ("+int1+")");
}

The console output (the timer is actually never called as it is cancelled by the clearInterval call):

Image

I guess either the timers must share the "instrumented" ids and be cleared the same way as intervals, or the native clearInterval call must be removed from the instrumented method (direct cancelling of intervals from outside of the frame seems to be "strange" practice imo).

@andres75125
Copy link

// Funciones de utilidad para gestionar integraciones de terceros en un iframe

import { rethrowAsync } from '#core/error';
import { isArray } from '#core/types';
import { hasOwn, map } from '#core/types/object';
import { devAssert, userAssert } from '#utils/log';

/** Almacena las integraciones de terceros registradas */
const registrations = {};

/** Lleva un registro de las cargas de scripts síncronas */
let syncScriptLoads = 0;

/**

  • Obtiene el mapa de registros.
  • @return {Object<string, function>} El mapa de registros.
    */
    export function getRegistrations() {
    return registrations;
    }

/**

  • Registra una integración de terceros.
  • @param {string} id - El identificador de la integración.
  • @param {function} draw - La función que inicializa la integración.
    */
    export function register(id, draw) {
    if (registrations[id]) {
    throw new Error(La integración '${id}' ya está registrada.);
    }
    registrations[id] = draw;
    }

/**

  • Ejecuta una integración registrada.
  • @param {string} id - El identificador de la integración.
  • @param {Window} win - El objeto ventana del navegador.
  • @param {Object} data - Los datos necesarios para la inicialización.
    */
    export function run(id, win, data) {
    const fn = registrations[id];
    if (!fn) {
    throw new Error(Integración desconocida: '${id}');
    }
    fn(win, data);
    }

/**

  • Carga un script de manera asíncrona.
  • @param {Window} win - La ventana del navegador.
  • @param {string} url - La URL del script.
  • @param {function=} onLoad - Callback cuando el script se carga correctamente.
  • @param {function=} onError - Callback cuando el script falla en cargar.
    */
    export function loadScript(win, url, onLoad, onError) {
    const script = document.createElement('script');
    script.src = url;
    script.async = true;
    script.onload = onLoad || (() => {});
    script.onerror = onError || (() => {});
    win.document.body.appendChild(script);
    }

/**

  • Ejecuta una función en la próxima microtarea.
  • @param {Window} win - La ventana del navegador.
  • @param {function} fn - La función a ejecutar.
    */
    export function nextTick(win, fn) {
    if (win.Promise) {
    win.Promise.resolve().then(fn);
    } else {
    win.setTimeout(fn, 0);
    }
    }

/**

  • Valida si la URL src comienza con un prefijo permitido.
  • @param {string|string[]} prefix - Prefijos permitidos.
  • @param {string} src - La URL a validar.
    */
    export function validateSrcPrefix(prefix, src) {
    const prefixes = Array.isArray(prefix) ? prefix : [prefix];
    if (!prefixes.some(p => src.startsWith(p))) {
    throw new Error(URL no válida: ${src});
    }
    }

/**

  • Valida los campos obligatorios en un objeto de datos.
  • @param {Object} data - El objeto de datos.
  • @param {string[]} requiredFields - Lista de campos obligatorios.
  • @param {string[]=} optionalFields - Lista de campos opcionales.
    */
    export function validateData(data, requiredFields, optionalFields = []) {
    requiredFields.forEach(field => {
    if (!data[field]) {
    throw new Error(Falta el campo obligatorio: ${field});
    }
    });

const allFields = new Set([...requiredFields, ...optionalFields]);
Object.keys(data).forEach(field => {
if (!allFields.has(field)) {
console.warn(Campo desconocido en los datos: ${field});
}
});
}

/**

  • Almacena los estados de experimentos.
    */
    const experimentToggles = {};

/**

  • Verifica si un experimento está habilitado.
  • @param {string} experimentId - Identificador del experimento.
  • @return {boolean} true si el experimento está activado, de lo contrario false.
    */
    export function isExperimentOn(experimentId) {
    return !!experimentToggles[experimentId];
    }

/**

  • Configura los estados de los experimentos.
  • @param {Object<string, boolean>} toggles - Estados de los experimentos.
    */
    export function setExperimentToggles(toggles) {
    Object.assign(experimentToggles, toggles);
    }

@powerivq
Copy link
Contributor

powerivq commented Feb 3, 2025

@PsychodelEKS We just committed a fix to temporarily address your problem. #40238 It will be expected to go out to stable in 2 weeks.

@PsychodelEKS
Copy link
Author

PsychodelEKS commented Feb 4, 2025

@PsychodelEKS We just committed a fix to temporarily address your problem. #40238 It will be expected to go out to stable in 2 weeks.

Thanks, I like the approach with 100500 id ))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants