Make the session storage cache work across page views - #16
Make the session storage cache work across page views#16YaroslavVlasenko wants to merge 12 commits into
Conversation
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.
… 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.
There was a problem hiding this comment.
Removing the session id from the key is the right diagnosis. What it also does is turn a set of sessionStorage writes that were previously unreachable into live code: nothing could read them back before, so the caching path has effectively never run. Several things need to hold before a cached response can be trusted on a later page view, and they are worth resolving here rather than afterwards, since this reaches every language SDK through the submodule.
The first two below are the ones that will bite in production.
An HTTP error response is cached and replayed for the rest of the tab's life
Lines 459-492. fetch only rejects on network failure, and xhr.onload fires for 4xx and 5xx as well, so neither error path reaches the clearCache() in the .catch at line 495 or in onerror at line 504. The cloud returns error details as a JSON body, so a transient 429 or 5xx parses cleanly, resolves as though it were data, and is written to session storage under sessionKey. Every later page view in that tab then finds it, replaces the freshly rendered payload with the error body, and reports the flow as complete. Under the previous per-request key the entry was never read back, so the missing status check had no observable effect.
The block comment at line 415 already states the intended behaviour: "The new JSON is then loaded if the request is returned with a success status code".
For the fetch path, at line 459:
.then(response => {
if (!response.ok) {
throw new Error('Request failed with status ' + response.status);
}
return response.text();
})and for the XHR path, at the top of xhr.onload on line 479:
// 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;
}A property flag can outlive the page view without the response it belongs to
The _property_ flag is written at line 275 when a snippet starts, but the response is only cached at line 487 once the POST comes back. Now that the flags survive a navigation there is no way back out of the gap between those two writes: if the user leaves the page before the POST completes, the next page view finds the flags set and no cached response, skips every snippet at line 224, and then started === 0 at line 306 marks the flow complete without ever issuing a request. Client-evidence properties stay null for the rest of the tab.
Treating a flag as meaningful only when the response it belongs to is present closes that, at line 221:
var isCached = sessionStorage &&
sessionStorage.getItem(sessionKey) &&
sessionStorage.getItem(sessionKey + "_property_" + name);Re-execution within a single page view is already prevented by the jsPropertiesStarted check at line 211, so this only changes how flags inherited from an earlier page view are treated.
A cached response overrides the payload rendered for the current page view
Lines 298-304. On a cache hit loadJSON assigns straight over json, so a response cached earlier in the tab replaces what the server rendered for this page view. That is backwards: the embedded JSON is newer by construction. If the data file, the consent state, or the set of client hints the browser sends has changed since the cached response was stored, the fresher server result is discarded, and it stays discarded for the lifetime of the tab.
The cache's job here is narrower: supply the properties that need client-side evidence, so that the extra POST can be skipped. Overlaying it onto the fresh payload keeps that benefit without losing anything the server has just sent:
// 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;
}This needs the same try/catch around the parse that loadJSON has at line 379, and a decision on what should happen when the cached entry turns out to be unparseable.
One consequence worth knowing either way: on a cache hit the whole flow resolves synchronously inside the constructor, before this.onChange has been assigned at line 626, so fireChangeFuncs runs against an empty changeFuncs. A handler registered by page code through onChange is silently never invoked on the second and subsequent page views. complete() is unaffected, because it checks the completed flag before falling back to onChange.
The previous page view's query evidence replaces the current page's
Lines 65-73. parameters is not static configuration: it is rendered from the query.* evidence of the request that produced this page, so it is effectively this page's own query string. Replacing it wholesale means that on any later page view where a request is actually made, the page's own query evidence is discarded in favour of whatever the first page view stored, and then written back over it by saveParameters() at line 428.
Under the previous per-request key this getItem always missed, so the replacement has never taken effect. Merging preserves the carry-over while letting the current page win:
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;
}The 51D_* saved values are unaffected either way, since they are added separately from getParametersFromStorage() at line 422.
Strict mode has never been enabled
Line 2 reads 'use-strict', which is not the directive, just an expression statement. It should be 'use strict';. Apply it together with the clearCache suggestion below and not on its own: clearCache currently assigns to an undeclared i and key, and those assignments start throwing the moment strict mode really engages.
'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.
Co-authored-by: Eugene Dorfman <eugene.dorfman@gmail.com>
|
Four points from the earlier review are still open on the current head:
Worth noting that none of these are introduced here. Every one of those sites is identical on So they are defects this change makes live rather than defects it creates, which is the reason to resolve them alongside it: merged as it stands, they reach every language SDK through the submodule. Suggested fixes for all four are in the earlier review body. If you would rather keep this PR to the key change, raising them as separate issues works just as well, provided they land with it rather than after. |
…of server-supplied values, and handle cached JSON parsing errors.
All four are in, applied as suggested: the status checks on both the fetch and XHR paths, the property flag only counting when its response is present, mergeCached overlaying the cache onto the fresh payload (wired through a small loadJSON split so the cached branch reuses the same continuation, with a graceful clear when the cached text does not parse), and the parameter merge with the current page winning. The existing browser suite in pipeline-dotnet stays green against this revision (57/57). Dedicated red/green tests for each of the four are coming to the pipeline-dotnet PR next, together with the submodule bump. |
YaroslavVlasenko
changed the title
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.
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.
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.
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.
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.
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.
3 participants
The session storage key included the per-request session id, so nothing written by one page view could be read by the next: the cache was never hit, orphaned keys piled up for the life of the tab, and the JavaScript properties re-ran with a json refresh on every page view. The key is now just the object name. Session storage is already scoped to one tab, and the id still goes to the server as the session-id parameter. Isolation between integrations on one site stays with the object name.
With the key stable, the paths behind it become reachable for the first time, and are made safe here:
clearCacheiterates backwards and declares its variables, so a clear removes every matching entry instead of every other one.'use-strict'was an expression, not the directive) andevidencePropertiesis declared.hasJSFunctionsno longer returns false unconditionally, and agrees withprocessJsPropertieson which snippets will run.Covered by the browser suite in pipeline-dotnet (
test/session-storage-cache, PR #357), which is green against this revision.