InternalError: Too much recursion

Bug reports and enhancement requests
Post Reply
jdigital
Posts: 10
Joined: Wed Mar 19, 2014 6:55 pm

InternalError: Too much recursion

Post by jdigital »

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
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

Investigating, thank you.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:89.0) Gecko/20100101 Firefox/89.0
skriptimaahinen
Master Bug Buster
Posts: 244
Joined: Wed Jan 10, 2018 7:37 am

Re: InternalError: Too much recursion

Post by skriptimaahinen »

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.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
skriptimaahinen
Master Bug Buster
Posts: 244
Joined: Wed Jan 10, 2018 7:37 am

Re: InternalError: Too much recursion

Post by skriptimaahinen »

patchWindow.js

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);
}
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:

Code: Select all

ns.on("after_capabilities", event => {
  applyPatches();
});
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

Code: Select all

      this.fire("capabilities");
      this.fire("after_capabilities");
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.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

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.
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
skriptimaahinen
Master Bug Buster
Posts: 244
Joined: Wed Jan 10, 2018 7:37 am

Re: InternalError: Too much recursion

Post by skriptimaahinen »

You mean something like this?

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;
}

(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.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

skriptimaahinen wrote: Sun Jun 13, 2021 1:20 pm You mean something like this?
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
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

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
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

Giorgio Maone wrote: Sun Jun 13, 2021 7:37 pm I'd probably prefer you to review my incoming commit. Thanks!
And here it is.
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
skriptimaahinen
Master Bug Buster
Posts: 244
Joined: Wed Jan 10, 2018 7:37 am

Re: InternalError: Too much recursion

Post by skriptimaahinen »

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.
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)
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.
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
skriptimaahinen
Master Bug Buster
Posts: 244
Joined: Wed Jan 10, 2018 7:37 am

Re: InternalError: Too much recursion

Post by skriptimaahinen »

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?
Oh, so they seem to. Need to be vary of that. You would prefer submits through GitHub?
Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Firefox/78.0
User avatar
Giorgio Maone
Site Admin
Posts: 9454
Joined: Wed Mar 18, 2009 11:22 pm
Location: Palermo - Italy
Contact:

Re: InternalError: Too much recursion

Post by Giorgio Maone »

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?
If it's not too much of an hassle for you, that would be great, thanks!
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0
barbaz
Senior Member
Posts: 10841
Joined: Sat Aug 03, 2013 5:45 pm

Re: InternalError: Too much recursion

Post by barbaz »

skriptimaahinen wrote: Tue Jun 15, 2021 6:48 am Oh, so they seem to. Need to be vary of that.
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 8-)
*Always* check the changelogs BEFORE updating that important software!
-
Post Reply