From 08e8c211f95b657615b58519b44750e39acdc0d3 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Wed, 29 Jul 2026 14:59:24 +0100 Subject: [PATCH 01/12] Do not put the session id in the session storage key In non-cookie mode the session storage key was "{{_objName}}_" + sessionId. The server generates a new session id on every request, so every page load got its own namespace, nothing written by a previous page load could ever be found, and the orphaned keys accumulated for the life of the tab. Measured on a live site: eight keys after one page view, sixteen after two, twenty-four after three, each under a different GUID. The cached response at sessionStorage.getItem(sessionKey) was never hit, so the JavaScript properties were re-evaluated and the pipeline re-called on every page view instead of once per tab. Session storage is already scoped to one tab, which is the isolation the session id appeared to provide, so removing it from the key costs nothing. The id is still sent to the server as the session-id parameter. Cookie mode is unchanged. Also tightens the clearCache prefix match. Without the session id the key is short enough that a bare startsWith would match an unrelated key on the same origin, so it now matches the exact key or the key plus a separator. --- JavaScriptResource.mustache | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index b75ed6a..c9363c0 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -3,7 +3,24 @@ fiftyoneDegreesManager = function() { var json = {{&_jsonObject}}; var parameters = {{&_parameters}}; var sessionId = "{{&_sessionId}}"; +{{#_enableCookies}} var sessionKey = "{{_objName}}_" + sessionId; +{{/_enableCookies}} +{{^_enableCookies}} + // The session id is deliberately NOT part of the session storage key. + // + // A new session id is generated by the server on every request, so + // putting it in the key gave every page load its own namespace. Nothing + // written by a previous page load could ever be found, the cached + // response was never hit, and the orphaned keys accumulated for the life + // of the tab. The effect was that the JavaScript properties were + // re-evaluated and the pipeline was re-called on every single page view. + // + // Session storage is already scoped to one tab, which is the isolation + // the session id appeared to be providing, so removing it costs nothing. + // The id is still sent to the server below as the session-id parameter. + var sessionKey = "{{_objName}}"; +{{/_enableCookies}} this.sessionId = sessionId; var sequence = {{&_sequence}}; @@ -50,7 +67,10 @@ fiftyoneDegreesManager = function() { if (sessionStorage) { for (i = 0; i < sessionStorage.length; i++) { key = sessionStorage.key(i); - if (startsWith(key, sessionKey)) { + // Match the exact key or the key plus a separator. Without the + // session id the prefix is short enough that a bare startsWith + // would also match an unrelated key on the same origin. + if (key === sessionKey || startsWith(key, sessionKey + "_")) { sessionStorage.removeItem(key); } } From 7cfad446302919a6fb41f56bddea97d94922b8bf Mon Sep 17 00:00:00 2001 From: YaroslavVlasenko Date: Fri, 31 Jul 2026 11:12:30 +0300 Subject: [PATCH 02/12] Use the same session storage key in cookie mode. Measured on the real stack: with cookies enabled the already-executed flags were still written under a per-request id, so they piled up and scripts without saved values re-ran on every page view. One key shape for both modes removes that and keeps the behaviour uniform. Isolation between pipelines on the same site stays with the object name. --- JavaScriptResource.mustache | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index c9363c0..3998984 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -3,24 +3,9 @@ fiftyoneDegreesManager = function() { var json = {{&_jsonObject}}; var parameters = {{&_parameters}}; var sessionId = "{{&_sessionId}}"; -{{#_enableCookies}} - var sessionKey = "{{_objName}}_" + sessionId; -{{/_enableCookies}} -{{^_enableCookies}} - // The session id is deliberately NOT part of the session storage key. - // - // A new session id is generated by the server on every request, so - // putting it in the key gave every page load its own namespace. Nothing - // written by a previous page load could ever be found, the cached - // response was never hit, and the orphaned keys accumulated for the life - // of the tab. The effect was that the JavaScript properties were - // re-evaluated and the pipeline was re-called on every single page view. - // - // Session storage is already scoped to one tab, which is the isolation - // the session id appeared to be providing, so removing it costs nothing. - // The id is still sent to the server below as the session-id parameter. + // The session id is deliberately not part of the key: it changes on + // every request, and session storage is already scoped to one tab. var sessionKey = "{{_objName}}"; -{{/_enableCookies}} this.sessionId = sessionId; var sequence = {{&_sequence}}; From 8713ba62845359ca92be9f0fcf4a8c3cfe9d11a5 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Fri, 31 Jul 2026 12:15:22 +0200 Subject: [PATCH 03/12] FIX: Enable strict mode 'use-strict' is an expression statement, not the directive, so strict mode has never actually been active. Declaring i and key in clearCache is a prerequisite for the directive taking effect: they were implicit globals, and the assignments throw once it does. --- JavaScriptResource.mustache | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 3998984..332c737 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -1,5 +1,5 @@ fiftyoneDegreesManager = function() { - 'use-strict'; + 'use strict'; var json = {{&_jsonObject}}; var parameters = {{&_parameters}}; var sessionId = "{{&_sessionId}}"; @@ -50,8 +50,8 @@ fiftyoneDegreesManager = function() { var clearCache = function() { if (sessionStorage) { - for (i = 0; i < sessionStorage.length; i++) { - key = sessionStorage.key(i); + for (var i = 0; i < sessionStorage.length; i++) { + var key = sessionStorage.key(i); // Match the exact key or the key plus a separator. Without the // session id the prefix is short enough that a bare startsWith // would also match an unrelated key on the same origin. From f7fc855da493f818bfa100d15ae15d97668586d8 Mon Sep 17 00:00:00 2001 From: Yaroslav Date: Fri, 31 Jul 2026 13:38:51 +0300 Subject: [PATCH 04/12] Update JavaScriptResource.mustache Co-authored-by: Eugene Dorfman --- JavaScriptResource.mustache | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 332c737..821f07e 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -50,7 +50,10 @@ fiftyoneDegreesManager = function() { var clearCache = function() { if (sessionStorage) { - for (var i = 0; i < sessionStorage.length; i++) { + // Iterate backwards: removeItem shifts every later key down one + // index, so a forward loop skips whichever key moves into the slot + // just vacated. + for (var i = sessionStorage.length - 1; i >= 0; i--) { var key = sessionStorage.key(i); // Match the exact key or the key plus a separator. Without the // session id the prefix is short enough that a bare startsWith From ecb82ca2c5b7d987f2d76da7497fd4e4b56ebcc3 Mon Sep 17 00:00:00 2001 From: YaroslavVlasenko Date: Fri, 31 Jul 2026 13:53:29 +0300 Subject: [PATCH 05/12] Fix `evidenceProperties` variable declaration to prevent scope leakage --- JavaScriptResource.mustache | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 821f07e..c7c4160 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -589,7 +589,7 @@ fiftyoneDegreesManager = function() { // Get all values in any 'evidenceproperty' fields on this object // or sub-objects. var getEvidencePropertiesFromObject = function (dataObject) { - evidenceProperties = []; + var evidenceProperties = []; for (var prop in dataObject) { if (dataObject.hasOwnProperty(prop)) { From 500ec66e7a15b16789821ea710949caaae9e8668 Mon Sep 17 00:00:00 2001 From: YaroslavVlasenko Date: Fri, 31 Jul 2026 17:19:10 +0300 Subject: [PATCH 06/12] Improve session storage handling: merge parameters, prevent override of server-supplied values, and handle cached JSON parsing errors. --- JavaScriptResource.mustache | 67 ++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index c7c4160..2d6fcab 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -69,12 +69,41 @@ fiftyoneDegreesManager = function() { if (sessionStorage) { var parametersString = sessionStorage.getItem(sessionKey + "_parameters"); if (parametersString) { - parameters = JSON.parse(parametersString); + // Merge rather than replace: parameters is rendered from this + // page view's own query evidence and must win over anything a + // previous page view stored under the same key. + var storedParameters = JSON.parse(parametersString); + for (var name in storedParameters) { + if (storedParameters.hasOwnProperty(name) && + parameters.hasOwnProperty(name) === false) { + parameters[name] = storedParameters[name]; + } + } } } return parameters; } + // Cached values fill the gaps in the payload rendered for this page view; + // they never override a value the server has just supplied. + var mergeCached = function(cachedJson) { + Object.getOwnPropertyNames(cachedJson).forEach(function(group) { + if (typeof cachedJson[group] !== 'object' || cachedJson[group] === null) { + return; + } + if (json[group] === undefined) { + json[group] = cachedJson[group]; + return; + } + for (var name in cachedJson[group]) { + if (json[group][name] === null || json[group][name] === undefined) { + json[group][name] = cachedJson[group][name]; + } + } + }); + return json; + } + var saveParameters = function(sourceParams) { if (sourceParams) { parameters = sourceParams @@ -221,7 +250,9 @@ fiftyoneDegreesManager = function() { toProcess++; } - var isCached = sessionStorage && sessionStorage.getItem(sessionKey + "_property_" + name); + var isCached = sessionStorage && + sessionStorage.getItem(sessionKey) && + sessionStorage.getItem(sessionKey + "_property_" + name); // If the property has already been processed then skip it. if (isCached) { @@ -301,8 +332,16 @@ fiftyoneDegreesManager = function() { if ((cached === toProcess || started === 0) && sessionStorage) { var cachedResponse = sessionStorage.getItem(sessionKey); if (cachedResponse) { - loadJSON(resolve, reject, cachedResponse); - executeCallback = false; + var cachedJson = null; + try { + cachedJson = JSON.parse(cachedResponse); + } catch (err) { + clearCache(); + } + if (cachedJson) { + loadParsedJSON(resolve, reject, mergeCached(cachedJson)); + executeCallback = false; + } } } @@ -379,13 +418,20 @@ fiftyoneDegreesManager = function() { {{#_updateEnabled}} // Process the response as json and call the resolve method. var loadJSON = function(resolve, reject, responseText) { + var newJson; try { - json = JSON.parse(responseText); + newJson = JSON.parse(responseText); } catch(err) { clearCache(); reject(new Error("Invalid JSON - the endpoint is likely setup incorrectly", { cause: err })); return; } + loadParsedJSON(resolve, reject, newJson); + } + + // Continues with an already parsed payload. + var loadParsedJSON = function(resolve, reject, newJson) { + json = newJson; if (hasJSFunctions()) { // json updated so fire 'on change' functions @@ -460,6 +506,9 @@ fiftyoneDegreesManager = function() { body: postBody }) .then(response => { + if (!response.ok) { + throw new Error('Request failed with status ' + response.status); + } return response.text(); }) .then(responseText => { @@ -481,6 +530,14 @@ fiftyoneDegreesManager = function() { xhr.onload = function () { + // An error status still reaches onload, and error details come back + // as a JSON body, so they must not be treated as data or cached. + if (xhr.status < 200 || xhr.status > 299) { + clearCache(); + reject(new Error('Request failed with status ' + xhr.status)); + return; + } + // Get the response body from the request. var responseText = xhr.responseText; From 0617eeaee79843a2f50c082f874c22b45c27c75e Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 11:54:36 +0200 Subject: [PATCH 07/12] Fix the defects the stable session storage key makes reachable mergeCached read through a fresh group that was null or a primitive, so a payload where an aspect is null and the cache has an object for it threw. In the promises render that surfaced as a rejection; in the non-promises render it killed the constructor and left the object undefined. It also index-merged arrays, which extended javascriptProperties with names the server no longer sends and grafted a cached errors array onto a payload that had none. Only aspect objects are merged now, and a value the server has just supplied is never merged into. An unparseable cached entry was cleared after the property flags had already been read, so the page view completed having neither run the snippets nor made a request, and an entry that parsed to null was never cleared at all. The payload is now read and validated once, before the flags, so a bad entry sends this page view down the request path. On a cache hit the flow resolved inside the constructor, before any onChange handler could be registered, so those handlers silently never fired on the second and subsequent page views. The cached continuation is deferred by a tick, which is what the request path gets for free. device.javascripthardwareprofile wrote its flag before the empty-profile back-out and the back-out did not remove it, so once any response was cached the property was never attempted again for the life of the tab. The flag is now removed with the back-out. The splice it replaces was indexing jsPropertiesStarted with the position in jsProperties. hasJSFunctions self-initialised its counter and compared it against the array rather than its length, so it always returned false and the re-process branch was dead. It now walks the list under the same conditions processJsProperties runs a snippet under - not already started, not delayed, non-empty body - and properties skipped as cached are recorded as started, so the two agree and the branch cannot spin. loadParsedJSON moved out of the update section: the cached branch calls it unconditionally, and a stable key means a payload cached by an update-enabled render can be present when an update-disabled one with the same object name is constructed. --- JavaScriptResource.mustache | 157 +++++++++++++++++++++++++----------- 1 file changed, 111 insertions(+), 46 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 2d6fcab..ed07217 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -84,20 +84,61 @@ fiftyoneDegreesManager = function() { return parameters; } + // Read the cached payload, or null when there is none or it cannot be used. + // An entry that does not parse into an object is cleared here rather than + // ignored: this runs before the '_property_' flags are read, so a bad entry + // makes this page view execute the snippets again instead of completing + // with neither the cached values nor a request. + var getCachedJson = function() { + if (!sessionStorage) { + return null; + } + var cachedResponse = sessionStorage.getItem(sessionKey); + if (!cachedResponse) { + return null; + } + var cachedJson = null; + try { + cachedJson = JSON.parse(cachedResponse); + } catch (err) { + // Handled by the check below, which also covers a payload that + // parses to null or to a value that is not an object. + } + if (typeof cachedJson !== 'object' || cachedJson === null) { + clearCache(); + return null; + } + return cachedJson; + } + // Cached values fill the gaps in the payload rendered for this page view; // they never override a value the server has just supplied. var mergeCached = function(cachedJson) { Object.getOwnPropertyNames(cachedJson).forEach(function(group) { - if (typeof cachedJson[group] !== 'object' || cachedJson[group] === null) { + var cachedGroup = cachedJson[group]; + // Only aspect objects carry cached values. The arrays at this level + // (javascriptProperties, errors, warnings) describe the request that + // produced them, so taking them from an earlier page view would + // resurrect a stale error or a snippet list the server no longer + // sends. + if (typeof cachedGroup !== 'object' || + cachedGroup === null || + Array.isArray(cachedGroup)) { + return; + } + var freshGroup = json[group]; + if (freshGroup === undefined || freshGroup === null) { + json[group] = cachedGroup; return; } - if (json[group] === undefined) { - json[group] = cachedJson[group]; + // Anything the server has just supplied as a non-object wins, and + // is not something values can be merged into. + if (typeof freshGroup !== 'object' || Array.isArray(freshGroup)) { return; } - for (var name in cachedJson[group]) { - if (json[group][name] === null || json[group][name] === undefined) { - json[group][name] = cachedJson[group][name]; + for (var name in cachedGroup) { + if (freshGroup[name] === null || freshGroup[name] === undefined) { + freshGroup[name] = cachedGroup[name]; } } }); @@ -224,6 +265,7 @@ fiftyoneDegreesManager = function() { var started = 0; var cached = 0; var toProcess = 0; + var cachedJson = getCachedJson(); // If there is no cached response and there are JavaScript code snippets // then process them and perform any call-backs required. @@ -250,13 +292,17 @@ fiftyoneDegreesManager = function() { toProcess++; } - var isCached = sessionStorage && - sessionStorage.getItem(sessionKey) && + var isCached = cachedJson && sessionStorage.getItem(sessionKey + "_property_" + name); // If the property has already been processed then skip it. if (isCached) { cached++; + // Record it as started so that hasJSFunctions agrees this + // snippet will not run in this page view. If it disagreed, + // loadParsedJSON would hand back to a call to process that + // does nothing and returns to loadParsedJSON again. + jsPropertiesStarted.push(name); continue; } @@ -316,10 +362,15 @@ fiftyoneDegreesManager = function() { if (name === "device.javascripthardwareprofile") { var hrw = getFodSavedValues(); if (hrw && !hrw["51D_ProfileIds"]) { - // find and remove name from jsPropertiesStarted - var propIndex = jsPropertiesStarted.indexOf(name); - if (propIndex > -1) { - jsPropertiesStarted.splice(index, 1); + // The snippet produced no profile, so the flag it + // has just written must not survive. With the key + // stable it would otherwise mark the property as + // done for the rest of the tab session and it would + // never be attempted again. The name stays in + // jsPropertiesStarted so that only the next page + // view retries it, not this one. + if (sessionStorage) { + sessionStorage.removeItem(sessionKey + "_property_" + name); } started--; toProcess--; @@ -329,20 +380,17 @@ fiftyoneDegreesManager = function() { } } - if ((cached === toProcess || started === 0) && sessionStorage) { - var cachedResponse = sessionStorage.getItem(sessionKey); - if (cachedResponse) { - var cachedJson = null; - try { - cachedJson = JSON.parse(cachedResponse); - } catch (err) { - clearCache(); - } - if (cachedJson) { - loadParsedJSON(resolve, reject, mergeCached(cachedJson)); - executeCallback = false; - } - } + if ((cached === toProcess || started === 0) && cachedJson) { + var merged = mergeCached(cachedJson); + // Deferred so that handlers registered through onChange after the + // constructor returns are in place before the change is announced. + // The request path is asynchronous and gets that for free; a cache + // hit resolves inside the constructor, where changeFuncs is still + // empty. + setTimeout(function() { + loadParsedJSON(resolve, reject, merged); + }, 0); + executeCallback = false; } if (started === 0) { @@ -390,13 +438,27 @@ fiftyoneDegreesManager = function() { {{/_supportsFetch}} {{/_updateEnabled}} - // Check if the JSON object still has any JavaScript snippets to run. + // Check if the JSON object still has any JavaScript snippets to run. The + // conditions here must match the ones processJsProperties runs a snippet + // under: if this says yes and processJsProperties then does nothing, + // loadParsedJSON calls process and is called back into forever. var hasJSFunctions = function() { - for (var i = i; i < json.javascriptProperties; i++) { - var body = getFromJson(json.javascriptProperties[i]); - if (body !== undefined && body.length > 0) { + var jsProperties = json.javascriptProperties; + if (jsProperties === undefined || jsProperties === null) { + return false; + } + for (var i = 0; i < jsProperties.length; i++) { + var name = jsProperties[i]; + if (jsPropertiesStarted.indexOf(name) !== -1) { + continue; + } + if (getFromJson(name + 'delayexecution', false, true) === true) { + continue; + } + var body = getFromJson(name); + if (typeof body === "string" && body.length > 0) { return true; - } + } } return false; } @@ -415,21 +477,10 @@ fiftyoneDegreesManager = function() { } } -{{#_updateEnabled}} - // Process the response as json and call the resolve method. - var loadJSON = function(resolve, reject, responseText) { - var newJson; - try { - newJson = JSON.parse(responseText); - } catch(err) { - clearCache(); - reject(new Error("Invalid JSON - the endpoint is likely setup incorrectly", { cause: err })); - return; - } - loadParsedJSON(resolve, reject, newJson); - } - - // Continues with an already parsed payload. + // Continues with an already parsed payload. Defined outside the update + // section because the cached branch of processJsProperties calls it, and + // with the key no longer carrying the session id a payload cached by an + // update-enabled render can still be present when this one is rendered. var loadParsedJSON = function(resolve, reject, newJson) { json = newJson; @@ -450,6 +501,20 @@ fiftyoneDegreesManager = function() { } } +{{#_updateEnabled}} + // Process the response as json and call the resolve method. + var loadJSON = function(resolve, reject, responseText) { + var newJson; + try { + newJson = JSON.parse(responseText); + } catch(err) { + clearCache(); + reject(new Error("Invalid JSON - the endpoint is likely setup incorrectly", { cause: err })); + return; + } + loadParsedJSON(resolve, reject, newJson); + } + // Sends a POST request to the call-back URL to retrieve and updated // JSON payload from the cloud service. A POST request is used so that // parameters can be passed in the request body, this is to get around From eb741af2ce1b27107d280651ded993ebbd74a170 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 12:13:49 +0200 Subject: [PATCH 08/12] Catch up an onChange handler registered after a cache hit The one-tick deferral was not enough: page code registers its handlers from the load event, which fires long after a setTimeout scheduled during the include. Handing the current payload to a handler registered once the flow has already finished is what complete already does, and it works whenever the registration happens. --- JavaScriptResource.mustache | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index ed07217..604bc91 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -381,15 +381,7 @@ fiftyoneDegreesManager = function() { } if ((cached === toProcess || started === 0) && cachedJson) { - var merged = mergeCached(cachedJson); - // Deferred so that handlers registered through onChange after the - // constructor returns are in place before the change is announced. - // The request path is asynchronous and gets that for free; a cache - // hit resolves inside the constructor, where changeFuncs is still - // empty. - setTimeout(function() { - loadParsedJSON(resolve, reject, merged); - }, 0); + loadParsedJSON(resolve, reject, mergeCached(cachedJson)); executeCallback = false; } @@ -750,6 +742,15 @@ fiftyoneDegreesManager = function() { this.onChange = function(resolve) { changeFuncs.push(resolve); + // A page view served from the session storage cache finishes inside + // the constructor, before page code can get here, so a handler + // registered afterwards would never hear about a change that has + // already happened. Catching it up is what 'complete' already does. + if ((completed || failed) && + typeof resolve === 'function' && + resolve.length === 1) { + resolve(json); + } } this.complete = function(resolve, properties) { From fd91a48f1c7f422d3f6d5558999f2585ace1fb98 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 13:01:38 +0200 Subject: [PATCH 09/12] Stop storing the parameters object in session storage The object is rendered from one page view's query evidence, so it is already exactly what that page view's refresh should report. Storing it and reading it back on the next page view can only add query parameters that belong to a different page: a campaign or tracking value present on the first page is replayed on every refresh for the rest of the tab session, and the merge added before this only decided which page won a name they both had. The values the snippets produce are the one thing that has to carry over, and they never came from here - getParametersFromStorage reads them from the 51D_ cookies or the _data_ keys, and processRequest merges them in on its own. So the stored copy was duplicating a mechanism that already works and putting stale evidence on top. saveParameters' sourceParams branch went with it: nothing has ever called it with an argument. --- JavaScriptResource.mustache | 38 +++++-------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 604bc91..2ae20f1 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -65,25 +65,6 @@ fiftyoneDegreesManager = function() { } } - var loadParameters = function() { - if (sessionStorage) { - var parametersString = sessionStorage.getItem(sessionKey + "_parameters"); - if (parametersString) { - // Merge rather than replace: parameters is rendered from this - // page view's own query evidence and must win over anything a - // previous page view stored under the same key. - var storedParameters = JSON.parse(parametersString); - for (var name in storedParameters) { - if (storedParameters.hasOwnProperty(name) && - parameters.hasOwnProperty(name) === false) { - parameters[name] = storedParameters[name]; - } - } - } - } - return parameters; - } - // Read the cached payload, or null when there is none or it cannot be used. // An entry that does not parse into an object is cleared here rather than // ignored: this runs before the '_property_' flags are read, so a bad entry @@ -145,17 +126,6 @@ fiftyoneDegreesManager = function() { return json; } - var saveParameters = function(sourceParams) { - if (sourceParams) { - parameters = sourceParams - } - - if (sessionStorage) { - var parametersString = JSON.stringify(parameters); - sessionStorage.setItem(sessionKey + "_parameters", parametersString); - } - } - // Get stored values with the '51D_' prefix that have been added to the request // and return the data as key value pairs. This method is needed to extract // stored values for inclusion in the GET or POST request for situations @@ -521,7 +491,11 @@ fiftyoneDegreesManager = function() { // returned with a success status code. If there was a problem then the // session storage items are invalidated and reject is called. var processRequest = function(resolve, reject){ - loadParameters(); + // parameters is rendered from this page view's own query evidence, so + // it is already what the refresh should report and nothing from an + // earlier page view belongs in it. The values the snippets produce are + // the one thing that does carry over, and they come from their own + // storage below rather than from a stored copy of this object. // Get additional parameters in case they are not sent // by the browser. @@ -531,8 +505,6 @@ fiftyoneDegreesManager = function() { parameters[parts[0]] = parts[1]; } - saveParameters(); - var params = []; for (var param in parameters) { if (parameters.hasOwnProperty(param)) { From 6605600b23733f9cc329a90325291aa46109c0d4 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 15:17:59 +0200 Subject: [PATCH 10/12] Run a consumer's browser tests against the template under review Nothing in this repository can be executed on its own: the templates are consumed as a submodule and rendered into an inline script, so their behaviour is only visible in an SDK that embeds one and drives it in a browser. Until now a change here reached every SDK with no check at all and was found, if at all, by whichever consumer bumped the submodule first. The job checks out pipeline-dotnet, copies this revision over the one it pins, and runs the JavaScript builder tests. It reports when the copy changed nothing, because the templates are embedded resources and an unchanged working tree would produce a pass that says nothing about the pull request. pipeline-dotnet is tested at main, which is what the change has to land against. Use the workflow_dispatch input to point it at a branch when the tests for a change are still in review themselves. --- .github/workflows/consumer-tests.yml | 82 ++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 .github/workflows/consumer-tests.yml diff --git a/.github/workflows/consumer-tests.yml b/.github/workflows/consumer-tests.yml new file mode 100644 index 0000000..f00235d --- /dev/null +++ b/.github/workflows/consumer-tests.yml @@ -0,0 +1,82 @@ +name: Consumer tests + +# The templates are consumed as a submodule, so a change here is only ever +# exercised by the SDKs that embed it. This builds one of them against the +# revision under review and runs the tests that drive the template in a real +# browser, which is the only place its behaviour is visible. + +on: + pull_request: + paths: + - '**.mustache' + - '.github/workflows/consumer-tests.yml' + workflow_dispatch: + inputs: + pipeline-dotnet-ref: + description: Branch, tag or SHA of pipeline-dotnet to test against + required: false + default: main + +permissions: + contents: read + +jobs: + pipeline-dotnet: + name: pipeline-dotnet JavaScript builder + runs-on: ubuntu-latest + steps: + - name: Check out the template under review + uses: actions/checkout@v4 + with: + path: template + + - name: Check out pipeline-dotnet + uses: actions/checkout@v4 + with: + repository: 51Degrees/pipeline-dotnet + ref: ${{ inputs.pipeline-dotnet-ref || 'main' }} + submodules: recursive + path: consumer + + - name: Swap in the template under review + run: | + set -euo pipefail + dest=consumer/FiftyOne.Pipeline.Elements/FiftyOne.Pipeline.JavaScriptBuilderElement/Templates + if [ ! -d "$dest" ]; then + echo "::error::$dest is missing - the submodule has moved and this workflow needs updating" + exit 1 + fi + cp template/*.mustache "$dest/" + # The templates are embedded resources. If the copy changed nothing + # then the run would exercise the revision pipeline-dotnet pins and + # report a pass that says nothing about this pull request. + if git -C "$dest" diff --quiet; then + echo "::notice::identical to the revision pipeline-dotnet pins, nothing swapped in" + else + git -C "$dest" --no-pager diff --stat + fi + + - uses: actions/setup-dotnet@v4 + with: + dotnet-version: '8.0.x' + + - name: Report the browser under test + run: | + google-chrome --version || echo "::warning::no google-chrome on the runner" + chromedriver --version || echo "::warning::no chromedriver on the runner" + + - name: Run the JavaScript builder tests + working-directory: consumer + run: > + dotnet test -c Release + FiftyOne.Pipeline.Elements/FiftyOne.Pipeline.JavaScriptBuilderElementTests/FiftyOne.Pipeline.JavaScriptBuilderElementTests.csproj + --logger "console;verbosity=normal" + --logger "trx;LogFileName=results.trx" + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: pipeline-dotnet-results + path: consumer/**/TestResults/*.trx + if-no-files-found: warn From 8ad3fbdeac253ae4372b2f6b9db08209626f91a6 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 15:21:25 +0200 Subject: [PATCH 11/12] Move the consumer test workflow to its own change It lands ahead of this one, so carrying it here as well would mean this pull request deleting it again on merge. Nothing else here depends on it. --- .github/workflows/consumer-tests.yml | 82 ---------------------------- 1 file changed, 82 deletions(-) delete mode 100644 .github/workflows/consumer-tests.yml diff --git a/.github/workflows/consumer-tests.yml b/.github/workflows/consumer-tests.yml deleted file mode 100644 index f00235d..0000000 --- a/.github/workflows/consumer-tests.yml +++ /dev/null @@ -1,82 +0,0 @@ -name: Consumer tests - -# The templates are consumed as a submodule, so a change here is only ever -# exercised by the SDKs that embed it. This builds one of them against the -# revision under review and runs the tests that drive the template in a real -# browser, which is the only place its behaviour is visible. - -on: - pull_request: - paths: - - '**.mustache' - - '.github/workflows/consumer-tests.yml' - workflow_dispatch: - inputs: - pipeline-dotnet-ref: - description: Branch, tag or SHA of pipeline-dotnet to test against - required: false - default: main - -permissions: - contents: read - -jobs: - pipeline-dotnet: - name: pipeline-dotnet JavaScript builder - runs-on: ubuntu-latest - steps: - - name: Check out the template under review - uses: actions/checkout@v4 - with: - path: template - - - name: Check out pipeline-dotnet - uses: actions/checkout@v4 - with: - repository: 51Degrees/pipeline-dotnet - ref: ${{ inputs.pipeline-dotnet-ref || 'main' }} - submodules: recursive - path: consumer - - - name: Swap in the template under review - run: | - set -euo pipefail - dest=consumer/FiftyOne.Pipeline.Elements/FiftyOne.Pipeline.JavaScriptBuilderElement/Templates - if [ ! -d "$dest" ]; then - echo "::error::$dest is missing - the submodule has moved and this workflow needs updating" - exit 1 - fi - cp template/*.mustache "$dest/" - # The templates are embedded resources. If the copy changed nothing - # then the run would exercise the revision pipeline-dotnet pins and - # report a pass that says nothing about this pull request. - if git -C "$dest" diff --quiet; then - echo "::notice::identical to the revision pipeline-dotnet pins, nothing swapped in" - else - git -C "$dest" --no-pager diff --stat - fi - - - uses: actions/setup-dotnet@v4 - with: - dotnet-version: '8.0.x' - - - name: Report the browser under test - run: | - google-chrome --version || echo "::warning::no google-chrome on the runner" - chromedriver --version || echo "::warning::no chromedriver on the runner" - - - name: Run the JavaScript builder tests - working-directory: consumer - run: > - dotnet test -c Release - FiftyOne.Pipeline.Elements/FiftyOne.Pipeline.JavaScriptBuilderElementTests/FiftyOne.Pipeline.JavaScriptBuilderElementTests.csproj - --logger "console;verbosity=normal" - --logger "trx;LogFileName=results.trx" - - - name: Upload test results - if: always() - uses: actions/upload-artifact@v4 - with: - name: pipeline-dotnet-results - path: consumer/**/TestResults/*.trx - if-no-files-found: warn From e7d6b4149da572e70c559aa44f6e176d768f5eb2 Mon Sep 17 00:00:00 2001 From: Eugene Dorfman Date: Mon, 3 Aug 2026 22:10:07 +0200 Subject: [PATCH 12/12] Tie the cached response and the property flags to one another Two states remained in which a page view could complete from the session storage cache while the evidence it holds had not reached the server. The cached === toProcess disjunct was reachable with a snippet in flight: toProcess only counts a property that has a body in the freshly rendered payload, cached counts one whose flag is set regardless, so a property the server has already resolved raises cached without raising toProcess. With the counts equal the cached payload was loaded, the promise resolved and the request suppressed, discarding the evidence the running snippet had just produced. started === 0 alone is the condition that was meant: a snippet that runs raises started, so every legitimate all-cached case already satisfies it. cached and toProcess have no other reader. A property flag was written when its snippet started but the response only when the request returned, so leaving the page in between left the flag behind. Any older cached payload then validated it, and from the next page view on the property counted as done, started stayed at 0 and no request was ever made again for the life of the tab. The flags are now written with the response they belong to, from a pending list, so neither can be present without the other. --- JavaScriptResource.mustache | 50 +++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/JavaScriptResource.mustache b/JavaScriptResource.mustache index 2ae20f1..960afbb 100644 --- a/JavaScriptResource.mustache +++ b/JavaScriptResource.mustache @@ -38,6 +38,12 @@ fiftyoneDegreesManager = function() { // Array of JavaScript properties that have started evaluation. var jsPropertiesStarted = []; + // Properties whose snippets have run but whose evidence has not yet reached + // the server. They are flagged in session storage only once the response + // they feed is cached, so a flag can never be present without the payload + // it belongs to. + var jsPropertiesPending = []; + // startsWith polyfill. var startsWith = function(source, searchValue) { return source.lastIndexOf(searchValue, 0) === 0; @@ -233,8 +239,6 @@ fiftyoneDegreesManager = function() { var processJsProperties = function(resolve, reject, jsProperties, ignoreDelayFlag) { var executeCallback = true; var started = 0; - var cached = 0; - var toProcess = 0; var cachedJson = getCachedJson(); // If there is no cached response and there are JavaScript code snippets @@ -257,17 +261,11 @@ fiftyoneDegreesManager = function() { } var body = getFromJson(name); - // If there is a body then this property should be processed. - if (body) { - toProcess++; - } - var isCached = cachedJson && sessionStorage.getItem(sessionKey + "_property_" + name); // If the property has already been processed then skip it. if (isCached) { - cached++; // Record it as started so that hasJSFunctions agrees this // snippet will not run in this page view. If it disagreed, // loadParsedJSON would hand back to a call to process that @@ -321,9 +319,12 @@ fiftyoneDegreesManager = function() { func(); } - if (sessionStorage) { - sessionStorage.setItem(sessionKey + "_property_" + name, true) - } + // Held until the response this snippet feeds is cached. + // Flagging it here instead would leave the flag behind when + // the page is left before the request completes, and an + // older cached payload would then validate it. Within this + // page view jsPropertiesStarted already stops it re-running. + jsPropertiesPending.push(name); // If the property is `javascripthardwareprofile` then check if the // profile has been set. If not then remove current property from @@ -332,25 +333,28 @@ fiftyoneDegreesManager = function() { if (name === "device.javascripthardwareprofile") { var hrw = getFodSavedValues(); if (hrw && !hrw["51D_ProfileIds"]) { - // The snippet produced no profile, so the flag it - // has just written must not survive. With the key - // stable it would otherwise mark the property as + // The snippet produced no profile, so it must not be + // flagged when the response is cached. With the key + // stable a flag would otherwise mark the property as // done for the rest of the tab session and it would // never be attempted again. The name stays in // jsPropertiesStarted so that only the next page // view retries it, not this one. - if (sessionStorage) { - sessionStorage.removeItem(sessionKey + "_property_" + name); + var pendingIndex = jsPropertiesPending.indexOf(name); + if (pendingIndex > -1) { + jsPropertiesPending.splice(pendingIndex, 1); } started--; - toProcess--; } } } } } - if ((cached === toProcess || started === 0) && cachedJson) { + // Only when nothing has started: a snippet that has run has evidence the + // cached payload cannot account for, so completing from the cache here + // would suppress the request that carries it. + if (started === 0 && cachedJson) { loadParsedJSON(resolve, reject, mergeCached(cachedJson)); executeCallback = false; } @@ -571,10 +575,18 @@ fiftyoneDegreesManager = function() { var responseText = xhr.responseText; {{/_supportsFetch}} - // Cache the response text. + // Cache the response text, and flag the snippets it accounts for at + // the same moment, so that the two cannot disagree. Cleared before + // loadJSON, which can re-enter processJsProperties and queue names + // for the next request. if (sessionStorage) { sessionStorage.setItem(sessionKey, responseText); + for (var pending = 0; pending < jsPropertiesPending.length; pending++) { + sessionStorage.setItem( + sessionKey + "_property_" + jsPropertiesPending[pending], true); + } } + jsPropertiesPending = []; // Load the JSON object from the response text loadJSON(resolve, reject, responseText); // Increment the sequence on a successful request