stripCustomNsAttrs performance #14928

Wintermoose · 2016-07-18T09:33:59Z

Performance suggestion

stripCustomNsAttrs is currently using recursive to walk the siblings in the DOM tree, which can easily cause it to run out of stack space in Internet Explorer. That happens to us when sanitizing a string containing cca 3000 DOM elements (which we do as performance optimization over ng-repeat, normally we of course bind much smaller strings).

It seems rewriting the code to use iteration should be safe, and resolves the problem entirely:

function stripCustomNsAttrs(node) {
  while(node) {
      if (node.nodeType === window.Node.ELEMENT_NODE) {
        var attrs = node.attributes;
        for (var i = 0, l = attrs.length; i < l; i++) {
          var attrNode = attrs[i];
          var attrName = attrNode.name.toLowerCase();
          if (attrName === 'xmlns:ns1' || attrName.lastIndexOf('ns1:', 0) === 0) {
            node.removeAttributeNode(attrNode);
            i--;
            l--;
          }
        }
      }

      var nextNode = node.firstChild;
      if (nextNode) {
        stripCustomNsAttrs(nextNode);
      }

      node = node.nextSibling;
  }
}

We experience the issue in Angular 1.5.7, Internet Explorer 11. The string bound is something like
<div class="entry"><div class="icon icontype"></div><span>Name</span></div> repeated 3000x

The text was updated successfully, but these errors were encountered:

Narretz · 2016-07-18T09:42:39Z

Thanks for raising this issue.
Can you create a runnable demo that shows this problem? Does IE crash or freeze when doing this? You can also open a PR with your suggested fix.

Wintermoose · 2016-07-18T10:43:26Z

Hello. IE throws exception:
Error: Out of stack space
at stripCustomNsAttrs (https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular-sanitize.js:492:3)
at stripCustomNsAttrs (https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular-sanitize.js:507:5)
at stripCustomNsAttrs (https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular-sanitize.js:512:5)
at stripCustomNsAttrs (https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular-sanitize.js:512:5)
...

The test below should trip it reliably (it does on both of my test system, but feel free to increase the number of elements added in the loop);

<!doctype html>
<html>
    <head>
        <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
        <title>IE11 ngSanitize test</title>
        <script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular.min.js"></script>
        <script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.5.7/angular-sanitize.min.js"></script>
        <script type="text/javascript">
            function testerFn($scope) {
                var list = [];
                for (var i = 0; i < 3000; ++i) {
                    list.push('<div class="entry">');
                    list.push('<icon class="icon"></div>');
                    list.push('<span>Name</span>');
                    list.push('</div>');
                }
                $scope.source = list.join('');
            }
            testerFn.$inject = ["$scope"];

            angular.module("test", ["ngSanitize"])
            .controller("tester", testerFn);
        </script>
    </head>
    <body>
        <h1>stripCustomNsAttrs test</h1>
        <div ng-app="test" ng-controller="tester">
            <div ng-bind-html="source"></div>
        </div>
    </body>
</html>

For reference, Edge seems to manage just fine even with 8x the amount: I am not sure if they allocate bigger stack, or Chakra implements tail-recursion optimization

gkalpak · 2016-07-18T11:02:55Z

Fwiw, it seems to indeed crash on IE11, but only with DevTools open. If I close the DevTools, it works fine.

Wintermoose · 2016-07-18T11:08:27Z

When you say "works fine", does it actually render the list (long column of "name" lines)? Because the exception is normally silent, but aborts the bind operation

gkalpak · 2016-07-18T11:21:12Z

does it actually render the list (long column of "name" lines)?

It does.
(To be clear, I still believe this should be fixed - especially, if the fix is a simple refactoring.)

BobChao87 · 2016-07-28T09:07:13Z

I used the script mentioned here along with something similar to the fix mentioned here and increased IE 11 from breaking on ~3k records to breaking on ~200k records. A grand improvement to be sure!

I am worried a bit about over-optimizing though, since the script and fix are heavily biased towards sibling nodes. I am wondering if that is something I should be concerned about. I have an idea bouncing around that could potentially address both child and sibling nodes, but if we are much more concerned with one than the other, the solution is significantly simpler to optimize for only one.

Chrome doesn't seem much affected either way by this script, managing ~1M records without too much issue.

Wintermoose · 2016-07-28T09:28:56Z

The whole code could be changed to walk the tree up/down the same way as the digest loop does; but I am not really sure it's worth the effort - the stack depth should now correspond to the depth of the DOM tree, and I wouldn't really expect that to be an issue.

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive Reduce stack height of function causing out of space error Closes angular#14928

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes #14928 Closes #15030

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes angular#14928 Closes angular#15030

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive. Reduce stack height of function causing out of stack space error. Closes #14928 Closes #15030

Narretz added component: ngSanitize type: perf browser: IE11 labels Jul 18, 2016

Narretz added this to the Backlog milestone Jul 18, 2016

BobChao87 pushed a commit to BobChao87/angular.js that referenced this issue Aug 11, 2016

fix(): reduce stack height

146f5ca

Update Internet Explorer-only helper function stripCustomNsAttrs to be less recursive Reduce stack height of function causing out of space error Closes angular#14928

BobChao87 mentioned this issue Aug 16, 2016

Patch strip custom ns attrs issue 14928 #15030

Closed

2 tasks

Narretz closed this as completed in 45129cf Sep 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stripCustomNsAttrs performance #14928

stripCustomNsAttrs performance #14928

Wintermoose commented Jul 18, 2016

Narretz commented Jul 18, 2016

Wintermoose commented Jul 18, 2016

gkalpak commented Jul 18, 2016

Wintermoose commented Jul 18, 2016

gkalpak commented Jul 18, 2016

BobChao87 commented Jul 28, 2016

Wintermoose commented Jul 28, 2016

stripCustomNsAttrs performance #14928

stripCustomNsAttrs performance #14928

Comments

Wintermoose commented Jul 18, 2016

Narretz commented Jul 18, 2016

Wintermoose commented Jul 18, 2016

gkalpak commented Jul 18, 2016

Wintermoose commented Jul 18, 2016

gkalpak commented Jul 18, 2016

BobChao87 commented Jul 28, 2016

Wintermoose commented Jul 28, 2016