Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ng-repeat throws a 'isecdom' error when iterating over an array with a "on" property #4805

Closed
andersekdahl opened this issue Nov 6, 2013 · 6 comments

Comments

@andersekdahl
Copy link

Using AngularJS 1.2.0-rc3, the following throws the error "'Referencing DOM nodes in Angular expressions is disallowed!" in ensureSafeObject(). It seems to misinterpret the array as a DOM node because it has both a "find" and an "on" property.

Controller code:

var array = [1, 2, 3];
array.on = true;
$scope.array = array;

Template:

<li ng-repeat="el in array">{{el}}</li>
@colinkahn
Copy link
Contributor

I'm running into a similar issue when trying to use Backbone Collection methods in my templates because they also have find and on methods and most of their method calls return a reference to themselves.

Is there a way to make checking for DOM nodes more precise?

@IgorMinar
Copy link
Contributor

we'll take a look at this next week. we are using heuristics to determine if objects are DOM nodes because there isn't a good standard way of doing this (one that works across browsers and iframes). if you have suggestions please let us know.

the current check is here:

function ensureSafeObject(obj, fullExpression) {
// nifty check if obj is Function that is fast and works across iframes and other contexts
if (obj) {
if (obj.constructor === obj) {
throw $parseMinErr('isecfn',
'Referencing Function in Angular expressions is disallowed! Expression: {0}',
fullExpression);
} else if (// isWindow(obj)
obj.document && obj.location && obj.alert && obj.setInterval) {
throw $parseMinErr('isecwindow',
'Referencing the Window in Angular expressions is disallowed! Expression: {0}',
fullExpression);
} else if (// isElement(obj)
obj.children && (obj.nodeName || (obj.on && obj.find))) {
throw $parseMinErr('isecdom',
'Referencing DOM nodes in Angular expressions is disallowed! Expression: {0}',
fullExpression);
}
}
return obj;
}

@ghost ghost assigned caitp Jan 6, 2014
@caitp
Copy link
Contributor

caitp commented Jan 7, 2014

I wrote some tests which might be a bit more reasonable, but also a fair bit more expensive: http://plnkr.co/edit/34DkkeB53HqpvR5SF7Le?p=preview

Something that might be doable:

if (obj && typeof obj === 'object') {
  if (obj instanceof JQLite ||
      obj instanceof Node ||
      (jQuery && obj instanceof jQuery) {
    return true;
  }
  /** 
   * Otherwise, test for some more peculiar jQuery functions which are
   * present in all of the various cloned APIs or node properties... It isn't
   * totally necessary to test their type (and this may have perf implications),
   * but it's   probably a better way to get an idea that they are an actual node.
   * Keep in mind that the above code should cover most cases where each
   * of these tests would be run --- in a falsy cases, this ends up being just
   * two tests, plus the initial 3 or 4, plus the outter two.
   */
  return (typeof obj.prop === 'function' && typeof obj.attr === 'function' &&
         typeof obj.bind === 'function') ||
         (typeof obj.nodeName === 'string' && typeof obj.nodeType === 'number');
}
return false;

The instanceof shorthand might not be necessary and could hurt performance for false checks, which in $parse is probably the majority of the time. But it's still the most accurate way to do this, I believe, and much better than looking at specific properties.

It would be good to write some benchmarks just to see how bad these are, or if it would make more sense to just look at using more node-specific function names.

@caitp
Copy link
Contributor

caitp commented Jan 7, 2014

I've run some perf tests, using typeof <property> === 'function' gets a nice perf boost in Chrome, but seems to significantly hurt Safari and Firefox (have not tested in IE). I think we get a more realistic result using typeof ... but maybe the perf speaks for itself.

Anyways, instanceof Node ends up crawling up the prototype chain and that kind of sucks, so it's not very helpful perf-wise.... I guess the thing to do is replace

obj.children && (obj.nodeName || (obj.on && obj.find)))

with

obj.children && (obj.nodeName || (obj.prop && obj.attr && obj.bind)))

(maybe .bind isn't needed, but it seems good to test more rather than fewer, and it shouldn't hurt significantly)

I guess I'll write this change and add some tests to make sure it doesn't break commonly used things, but keep in mind it is still possible to get false positives here.

caitp added a commit to caitp/angular.js that referenced this issue Jan 7, 2014
There are always going to be false positives here, unfortunately. But
testing different properties will hopefully reduce the number of false
positives in a meaningful way, without harming performance too much.

Closes angular#4805
caitp added a commit to caitp/angular.js that referenced this issue Jan 7, 2014
There are always going to be false positives here, unfortunately. But
testing different properties will hopefully reduce the number of false
positives in a meaningful way, without harming performance too much.

Closes angular#4805
@tbosch tbosch modified the milestones: 1.2.12, 1.2.11, 1.2.13 Feb 3, 2014
@btford btford modified the milestones: 1.2.14, 1.2.13 Feb 15, 2014
@caitp caitp closed this as completed in 5fe1f39 Feb 21, 2014
@Petah
Copy link

Petah commented Nov 12, 2016

:( I have a "tree" object that has both children and nodeName, I guess it will be me that has to change.

@Petah Petah unassigned caitp Nov 12, 2016
@gkalpak
Copy link
Member

gkalpak commented Nov 14, 2016

FTW, from 1.6 onwards (with the sandbox removed), there will be no issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants