InternalError: Too much recursion
InternalError: Too much recursion
Downloading a Tableau workbook from Tableau Public generally results in infinite recursion, see attached screenshot showing modifyWindowOpenMethod from patchWindow.js. This happens with 11.2.8 and 11.2.9rc1.
screenshot: https://imgur.com/a/TjMXuMF
Repro steps:
visit https://public.tableau.com/en-us/galler ... bage-patch
scroll down, click on one of the charts, such as the bar chart ("Frequency of Polymer Type")
at the bottom of the dashboard (the colored pane), there's a toolbar with icons.
click on the icon that looks like a download icon
in the popup list, select Tableau Workbook
in the popup dialog, click Download
generally this will produce the error condition seen in the screenshot
Firefox 78.11.0 ESR
screenshot: https://imgur.com/a/TjMXuMF
Repro steps:
visit https://public.tableau.com/en-us/galler ... bage-patch
scroll down, click on one of the charts, such as the bar chart ("Frequency of Polymer Type")
at the bottom of the dashboard (the colored pane), there's a toolbar with icons.
click on the icon that looks like a download icon
in the popup list, select Tableau Workbook
in the popup dialog, click Download
generally this will produce the error condition seen in the screenshot
Firefox 78.11.0 ESR
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
Investigating, thank you.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:89.0) Gecko/20100101 Firefox/89.0
-
- Master Bug Buster
- Posts: 244
- Joined: Wed Jan 10, 2018 7:37 am
Re: InternalError: Too much recursion
Was wondering if this would turn out to be a problem...
Since the patches should not be detectable (in theory), there is no practical way to tell if a window has been already patched. Which means the patch needs to be applied every time the window is accessed through the methods that need patching. Too many calls and the patches form a chain that Firefox will consider as infinite loop and throws an error.
Solution: WeakSet of the patched windows so a window can be tested if it has already been patched.
Caveat: This would prevent calling patchWindow separately for webgl, prefetchCSSResources and media. What is needed is separate function to "register" the patches and another to apply them as one patch.
I'll be posting the code later when I have tested it a bit.
Since the patches should not be detectable (in theory), there is no practical way to tell if a window has been already patched. Which means the patch needs to be applied every time the window is accessed through the methods that need patching. Too many calls and the patches form a chain that Firefox will consider as infinite loop and throws an error.
Solution: WeakSet of the patched windows so a window can be tested if it has already been patched.
Caveat: This would prevent calling patchWindow separately for webgl, prefetchCSSResources and media. What is needed is separate function to "register" the patches and another to apply them as one patch.
I'll be posting the code later when I have tested it a bit.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
-
- Master Bug Buster
- Posts: 244
- Joined: Wed Jan 10, 2018 7:37 am
Re: InternalError: Too much recursion
patchWindow.js
Quick tour of the changes:
patchWindow.js:
- Added const patches = []; at the beginning to store the patching callbacks and envs.
- patchWindow() is now used to "register" the callbacks (feel free to rename it more appropriately).
- Moved modifyWindow() and rest to the new applyPatches() and left patches.push([patchingCallback, env]); at the end of the patchWindow().
- modifyWindow() now takes an array of callbacks (and envs) and loops through them.
Needed a way to run applyPatches() after all the patchWindow() calls were done. This seems to do the trick:
Which I positioned at the end of content.js but that should matter little. Feel free to put in more appropriate file.
Also need to actually fire the event after "capabilities" have fired, so...
staticNS.js: 113
Might also want to encapsulate the patchWindow.js a bit so that the patches don't collide with anything.
Plus reminding that this viewtopic.php?f=10&t=26316#p103952 needs to be rolled back as it breaks things unless there is a compelling reason not to.
Code: Select all
"use strict";
/**
* Injects code into page context in a cross-browser way, providing it
* with tools to wrap/patch the DOM and the JavaScript environment
* and propagating the changes to child windows created on the fly in order
* to prevent the modifications to be cancelled by hostile code.
*
* @param function patchingCallback
* the (semi)privileged wrapping code to be injected.
* Warning: this is not to be considered a closure, since Chromium
* injection needs it to be reparsed out of context.
* Use the env argument to propagate parameters.
* It will be called as patchingCallback(unwrappedWindow, env).
* @param object env
* a JSON-serializable object to made available to patchingCallback as
* its second argument*
* @returns object port
* A Port object to be used to communicate with the privileged content
* script, by using port.postMessage(msg, [event]) and
* the port.onMessage(msg, event) user-defined callback.
*/
const patches = [];
function patchWindow(patchingCallback, env = {}) {
let eventId = this && this.eventId || `windowPatchMessages:${uuid()}`;
let { dispatchEvent, addEventListener } = window;
function Port(from, to) {
// we need a double dispatching dance and maintaining a stack of
// return values / thrown errors because Chromium seals the detail object
// (on Firefox we could just append further properties to it...)
let retStack = [];
function fire(e, detail, target = window) {
dispatchEvent.call(target, new CustomEvent(`$eventId:$e`, {detail, composed: true}));
}
this.postMessage = function(msg, target = window) {
retStack.push({});
let detail = msg;
fire(to, detail, target);
let ret = retStack.pop();
if (ret.error) throw ret.error;
return ret.value;
};
addEventListener.call(window, `$eventId:$from`, event => {
if (typeof this.onMessage === "function" && event.detail) {
let ret = {};
try {
ret.value = this.onMessage(event.detail.msg, event);
} catch (error) {
ret.error = error;
}
fire(`return:$to`, ret);
}
}, true);
addEventListener.call(window, `$eventId:return:$from`, event => {
let detail = event;
if (detail && retStack.length) {
retStack[retStack.length -1] = detail;
}
}, true);
this.onMessage = null;
}
let port = new Port("extension", "page");
let nativeExport = this && this.exportFunction || typeof exportFunction == "function";
if (!nativeExport) {
// Chromium
let exportFunction = (func, targetObject, {defineAs, original}) => {
try {
let [propDef, getOrSet, propName] = defineAs && /^([gs]et)(?:\s+(\w+))$/.exec(defineAs) || [null, null, defineAs];
let propDes = propName && Object.getOwnPropertyDescriptor(targetObject, propName);
if (!original) original = propDef && propDes ? propDes[getOrSet] : targetObject[defineAs];
let proxy = new Proxy(original, {
apply(target, thisArg, args) {
return func.apply(thisArg, args);
}
});
if (propName) {
if (!propDes) {
targetObject[propName] = proxy;
} else {
if (getOrSet) {
propDes[getOrSet] = proxy;
} else {
if ("value" in propDes) {
propDes.value = proxy;
} else {
return exportFunction(() => proxy, targetObject, `get $propName`);
}
}
Object.defineProperty(targetObject, propName, propDes);
}
}
return proxy;
} catch (e) {
console.error(e, `setting $targetObject.${defineAs || original}`, func);
}
return null;
};
let cloneInto = (obj, targetObject) => {
return obj; // dummy for assignment
};
let script = document.createElement("script");
script.text = `
(() => {
let patchWindow = $patchWindow;
let cloneInto = $cloneInto;
let exportFunction = $exportFunction;
let env = ${JSON.stringify(env)};
let eventId = ${JSON.stringify(eventId)};
env.port = new ($Port)("page", "extension");
({
patchWindow,
exportFunction,
cloneInto,
eventId,
}).patchWindow($patchingCallback, env);
})();
`;
document.documentElement.insertBefore(script, document.documentElement.firstChild);
script.remove();
return port;
}
env.port = new Port("page", "extension");
patches.push([patchingCallback, env]);
return port;
}
function applyPatches() {
const patchedWindows = new WeakSet();
function modifyWindow(win, modifyTarget) {
try {
win = win.wrappedJSObject || win;
if (patchedWindows.has(win)) return;
modifyTarget.forEach(([callback, env]) => callback(win, env));
modifyWindowOpenMethod(win, modifyTarget);
modifyFramingElements(win, modifyTarget);
// we don't need to modify win.opener, read skriptimaahinen notes
// at https://forums.informaction.com/viewtopic.php?p=103754#p103754
patchedWindows.add(win);
} catch (e) {
if (e instanceof DOMException && e.name === "SecurityError") {
// In case someone tries to access SOP restricted window.
// We can just ignore this.
} else throw e;
}
}
function modifyWindowOpenMethod(win, modifyTarget) {
let windowOpen = win.open;
exportFunction(function(...args) {
let newWin = windowOpen.call(this, ...args);
if (newWin) modifyWindow(newWin, modifyTarget);
return newWin;
}, win, {defineAs: "open"});
}
function modifyFramingElements(win, modifyTarget) {
for (let property of ["contentWindow", "contentDocument"]) {
for (let iface of ["Frame", "IFrame", "Object"]) {
let proto = win[`HTML$ifaceElement`].prototype;
modifyContentProperties(proto, property, modifyTarget)
}
}
}
function modifyContentProperties(proto, property, modifyTarget) {
let descriptor = Object.getOwnPropertyDescriptor(proto, property);
let origGetter = descriptor.get;
let replacementFn;
if (property === "contentWindow") { replacementFn = function() {
let win = origGetter.call(this);
if (win) modifyWindow(win, modifyTarget);
return win;
}}
if (property === "contentDocument") { replacementFn = function() {
let document = origGetter.call(this);
if (document && document.defaultView) modifyWindow(document.defaultView, modifyTarget);
return document;
}}
descriptor.get = exportFunction(replacementFn, proto, {defineAs: `get $property`});
Object.defineProperty(proto, property, descriptor);
}
modifyWindow(window, patches);
}
patchWindow.js:
- Added const patches = []; at the beginning to store the patching callbacks and envs.
- patchWindow() is now used to "register" the callbacks (feel free to rename it more appropriately).
- Moved modifyWindow() and rest to the new applyPatches() and left patches.push([patchingCallback, env]); at the end of the patchWindow().
- modifyWindow() now takes an array of callbacks (and envs) and loops through them.
Needed a way to run applyPatches() after all the patchWindow() calls were done. This seems to do the trick:
Code: Select all
ns.on("after_capabilities", event => {
applyPatches();
});
Also need to actually fire the event after "capabilities" have fired, so...
staticNS.js: 113
Code: Select all
this.fire("capabilities");
this.fire("after_capabilities");
Plus reminding that this viewtopic.php?f=10&t=26316#p103952 needs to be rolled back as it breaks things unless there is a compelling reason not to.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
Thank you for examining this and trying to patch it!
I don't feel comfortable with the two-steps approach, though: I fully understand it rationale, but unless applying all the patches in one shot at the right time can be automated and decoupled from NoScript's internal event firing machinery (which I doubt can be done reliably) I'm afraid it can be a serious hit at NSCL's (re)usability.
I'm currently trying to adapt your approach so that the patches can be applied to the current window as soon as patchWindow is invoked, but collected for deferred one-shot injection in dependent windows.
Thanks again.
I don't feel comfortable with the two-steps approach, though: I fully understand it rationale, but unless applying all the patches in one shot at the right time can be automated and decoupled from NoScript's internal event firing machinery (which I doubt can be done reliably) I'm afraid it can be a serious hit at NSCL's (re)usability.
I'm currently trying to adapt your approach so that the patches can be applied to the current window as soon as patchWindow is invoked, but collected for deferred one-shot injection in dependent windows.
Thanks again.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
-
- Master Bug Buster
- Posts: 244
- Joined: Wed Jan 10, 2018 7:37 am
Re: InternalError: Too much recursion
You mean something like this?
(edit: fix broken code)
Yes, I think this should work too and it is more concise. Probably better approach. Need to test it a bit more though.
However, applyPatches is not dependent on NS events. It can be called whenever the library user has determined that all patches have been registered. Be it right after the patchWindow calls in the same function or as in NoScripts case, in a event that's fired right after the event where last of the patchWindow calls are.
Code: Select all
"use strict";
/**
* Injects code into page context in a cross-browser way, providing it
* with tools to wrap/patch the DOM and the JavaScript environment
* and propagating the changes to child windows created on the fly in order
* to prevent the modifications to be cancelled by hostile code.
*
* @param {function} patchingCallback
* the (semi)privileged wrapping code to be injected.
* Warning: this is not to be considered a closure, since Chromium
* injection needs it to be reparsed out of context.
* Use the env argument to propagate parameters.
* It will be called as patchingCallback(unwrappedWindow, env).
* @param {object} env
* a JSON-serializable object to made available to patchingCallback as
* its second argument*
* @returns {object} port
* A Port object to be used to communicate with the privileged content
* script, by using port.postMessage(msg, [event]) and
* the port.onMessage(msg, event) user-defined callback.
*/
function patchWindow(patchingCallback, env = {}) {
let eventId = this && this.eventId || `windowPatchMessages:${uuid()}`;
let { dispatchEvent, addEventListener } = window;
function Port(from, to) {
// we need a double dispatching dance and maintaining a stack of
// return values / thrown errors because Chromium seals the detail object
// (on Firefox we could just append further properties to it...)
let retStack = [];
function fire(e, detail, target = window) {
dispatchEvent.call(target, new CustomEvent(`${eventId}:${e}`, {detail, composed: true}));
}
this.postMessage = function(msg, target = window) {
retStack.push({});
let detail = {msg};
fire(to, detail, target);
let ret = retStack.pop();
if (ret.error) throw ret.error;
return ret.value;
};
addEventListener.call(window, `${eventId}:${from}`, event => {
if (typeof this.onMessage === "function" && event.detail) {
let ret = {};
try {
ret.value = this.onMessage(event.detail.msg, event);
} catch (error) {
ret.error = error;
}
fire(`return:${to}`, ret);
}
}, true);
addEventListener.call(window, `${eventId}:return:${from}`, event => {
let {detail} = event;
if (detail && retStack.length) {
retStack[retStack.length -1] = detail;
}
}, true);
this.onMessage = null;
}
let port = new Port("extension", "page");
let nativeExport = this && this.exportFunction || typeof exportFunction == "function";
if (!nativeExport) {
// Chromium
let exportFunction = (func, targetObject, {defineAs, original}) => {
try {
let [propDef, getOrSet, propName] = defineAs && /^([gs]et)(?:\s+(\w+))$/.exec(defineAs) || [null, null, defineAs];
let propDes = propName && Object.getOwnPropertyDescriptor(targetObject, propName);
if (!original) original = propDef && propDes ? propDes[getOrSet] : targetObject[defineAs];
let proxy = new Proxy(original, {
apply(target, thisArg, args) {
return func.apply(thisArg, args);
}
});
if (propName) {
if (!propDes) {
targetObject[propName] = proxy;
} else {
if (getOrSet) {
propDes[getOrSet] = proxy;
} else {
if ("value" in propDes) {
propDes.value = proxy;
} else {
return exportFunction(() => proxy, targetObject, `get ${propName}`);
}
}
Object.defineProperty(targetObject, propName, propDes);
}
}
return proxy;
} catch (e) {
console.error(e, `setting ${targetObject}.${defineAs || original}`, func);
}
return null;
};
let cloneInto = (obj, targetObject) => {
return obj; // dummy for assignment
};
let script = document.createElement("script");
script.text = `
(() => {
let patchWindow = ${patchWindow};
let cloneInto = ${cloneInto};
let exportFunction = ${exportFunction};
let env = ${JSON.stringify(env)};
let eventId = ${JSON.stringify(eventId)};
env.port = new (${Port})("page", "extension");
({
patchWindow,
exportFunction,
cloneInto,
eventId,
}).patchWindow(${patchingCallback}, env);
})();
`;
document.documentElement.insertBefore(script, document.documentElement.firstChild);
script.remove();
return port;
}
env.port = new Port("page", "extension");
const patches = [];
const patchedWindows = new WeakSet();
function modifyWindow(win, modifyTarget) {
try {
win = win.wrappedJSObject || win;
modifyTarget.forEach(([callback, env]) => callback(win, env));
if (patchedWindows.has(win)) return;
modifyWindowOpenMethod(win);
modifyFramingElements(win);
// we don't need to modify win.opener, read skriptimaahinen notes
// at https://forums.informaction.com/viewtopic.php?p=103754#p103754
patchedWindows.add(win);
} catch (e) {
if (e instanceof DOMException && e.name === "SecurityError") {
// In case someone tries to access SOP restricted window.
// We can just ignore this.
} else throw e;
}
}
function modifyWindowOpenMethod(win) {
let windowOpen = win.open;
exportFunction(function(...args) {
let newWin = windowOpen.call(this, ...args);
if (newWin) modifyWindow(newWin, patches);
return newWin;
}, win, {defineAs: "open"});
}
function modifyFramingElements(win) {
for (let property of ["contentWindow", "contentDocument"]) {
for (let iface of ["Frame", "IFrame", "Object"]) {
let proto = win[`HTML${iface}Element`].prototype;
modifyContentProperties(proto, property)
}
}
}
function modifyContentProperties(proto, property) {
let descriptor = Object.getOwnPropertyDescriptor(proto, property);
let origGetter = descriptor.get;
let replacementFn;
if (property === "contentWindow") { replacementFn = function() {
let win = origGetter.call(this);
if (win) modifyWindow(win, patches);
return win;
}}
if (property === "contentDocument") { replacementFn = function() {
let document = origGetter.call(this);
if (document && document.defaultView) modifyWindow(document.defaultView, patches);
return document;
}}
descriptor.get = exportFunction(replacementFn, proto, {defineAs: `get ${property}`});
Object.defineProperty(proto, property, descriptor);
}
patches.push([patchingCallback, env]);
modifyWindow(window, [[patchingCallback, env]]);
return port;
}
Yes, I think this should work too and it is more concise. Probably better approach. Need to test it a bit more though.
However, applyPatches is not dependent on NS events. It can be called whenever the library user has determined that all patches have been registered. Be it right after the patchWindow calls in the same function or as in NoScripts case, in a event that's fired right after the event where last of the patchWindow calls are.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
Not sure why, but something really nasty happen to code when you post it here
Especially interpolated strings with variables inside get really messed up.
Could you please send me as a text file or a link pastebin somewhere on the web, if you really prefer not to create a proper PR on Github?
Thank you!
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
BTW, I'm almost done with my version (which fixes a couple of issues with your latest iteration, e.g. only the first patchWindow() call gets to actually modify the window), so in this instance I'd probably prefer you to review my incoming commit. Thanks!
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
And here it is.Giorgio Maone wrote: ↑Sun Jun 13, 2021 7:37 pm I'd probably prefer you to review my incoming commit. Thanks!
I've removed the patch accumulator because I couldn't figure out any way to make it work with the Chromium injection code path, which is unavoidably stateless. Of course this means that every patchWindow call creates to a new interception layer around window spawning related stuff, but this shouldn't be an issue (and is not the cause of the bug at hand, anyway).
Thanks again for any further insight.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
-
- Master Bug Buster
- Posts: 244
- Joined: Wed Jan 10, 2018 7:37 am
Re: InternalError: Too much recursion
Your version seems to work quite well. In hindsight, it was quite unnecessary to try to cram all the patches into one call. Preferring your approach.
Yes, should not be a problem. patchedWindows should make sure it doesn't blow out of hands anymore even if some script gets spammy accessing the windows.Giorgio Maone wrote: ↑Sun Jun 13, 2021 8:51 pm Of course this means that every patchWindow call creates to a new interception layer around window spawning related stuff, but this shouldn't be an issue (and is not the cause of the bug at hand, anyway)
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
-
- Master Bug Buster
- Posts: 244
- Joined: Wed Jan 10, 2018 7:37 am
Re: InternalError: Too much recursion
Oh, so they seem to. Need to be vary of that. You would prefer submits through GitHub?Giorgio Maone wrote: ↑Sun Jun 13, 2021 6:39 pm Not sure why, but something really nasty happen to code when you post it here
Especially interpolated strings with variables inside get really messed up.
Could you please send me as a text file or a link pastebin somewhere on the web, if you really prefer not to create a proper PR on Github?
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
- Giorgio Maone
- Site Admin
- Posts: 9455
- Joined: Wed Mar 18, 2009 11:22 pm
- Location: Palermo - Italy
- Contact:
Re: InternalError: Too much recursion
If it's not too much of an hassle for you, that would be great, thanks!skriptimaahinen wrote: ↑Tue Jun 15, 2021 6:48 am Oh, so they seem to. Need to be vary of that. You would prefer submits through GitHub?
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
Re: InternalError: Too much recursion
skriptimaahinen, would you mind editing your post viewtopic.php?p=104124#p104124 and trying again to post that code there again in light of this? - viewtopic.php?f=14&t=26364
Thanks Giorgio for fixing that, much better now
*Always* check the changelogs BEFORE updating that important software!
-