Illegal Invocation / Infinite Digest on deep $watch if a property is a DOM or jQuery object.

[micah-williamson]

Example JSBin:

http://jsbin.com/favorinava/6/edit

Steps to reproduce:

• Create a scope.
• Set a property on the scope to a DOM element.
• Create a $watch on the scope and set the objectEquality property to true.

angular.module('app', [])
  .controller('SomeCtrl', function($scope) {
    $scope.data = {
      test: document.body
    };

    // No problem
    $scope.$watch('data', function(newVal) {}, false);

    // Problem
    $scope.$watch('data', function(newVal) {}, true);
  })

[lgalfaso]

I do not know what you are trying to do, but watching DOM elements/jQuery elements is not something planned to be supported any time soon

[pkozlowski-opensource]

Not to mention horrible performance implications of doing so.... It would be interesting to understand what is going on, though. But yeh, we should actively discourage people from watching DOM objects and their properties as part of the digest loop.

[micah-williamson]

I'm not trying to watch a DOM object and I agree with both of your comments. I noticed this bug with angular-ui-sortable. This module does a deep watch on its sortOptions which can contain a DOM/jQuery object. The intention isn't to watch the DOM object itself but it gets scooped into the deep watch just the same, causing the infinite loop and illegal invocation errors.

Here is my bug on that project: angular-ui/ui-sortable#330

I dug deeper into the problem and found I could replicate it with vanilla angular.js so the bug wasn't in ui-sortable but in angular. The problem is caused by the deep watch doing an angular.copy on the scope object, which attempts to do a deep copy of the DOM object. I don't think this is intended functionality and angular.copy should not attempt to copy DOM objects.

Here is my pull request: #11006

[lgalfaso]

Hi, Sorry, I just do not agree with this issue nor with the solution provided at #11006. In fact, the only real solution would be that if any DOM element goes into angular.copy then we should throw (regardless if this element is the one being copied of within the structure being copied).

This will be a breaking change, so can not happen in a point release.

@petebacondarwin WDYT?

[micah-williamson]

My PR does throw if any DOM element goes into copy.

The other side of this is if you're doing a deep watch on any object if it finds a DOM/jQuery object in that object it will not attempt to copy it.

I don't think this is a breaking change. If anyone has done a copy or deep watch on an object containing a DOM object their application is already broken.

[micah-williamson]

I think we're in agreement, maybe I'm not explaining correctly.

• I don't want to copy DOM objects. And don't think you should be able to.
• I don't want to watch DOM objects. And don't think you should be able to.
• Current angular watch tries to watch and copy DOM objects. Which is impossible because of illegal invocation and infinite digest loop. My PR stops copy on a DOM object if you're doing a deep watch. My PR throws an error if you try to copy a DOM object.

[lgalfaso]

Hi, the PR allows this to be valid and not throw

it("should not throw an exception if a copied object contains a DOM and/or Element.", function() {
  var src = {a: document.body, b: angular.element(document.body)};
  var dst = {};
  expect(function() { copy(src, dst); }).not.toThrow();
});

and it should throw. Anything that is part of the DOM should never go into a $watch

[micah-williamson]

It copies a and b to dest but does not attempt to copy document.body recursively.

There is valid cases for having a DOM object on an object being watched though.
https://github.com/angular-ui/ui-sortable/blob/master/src/sortable.js [297 - 332]

[lgalfaso]

@iamchairs this looks simply wrong, deep watching on any structure that ends up in a DOM should be illegal. In this specific case, the right solution is to use $watchCollection and not a deep watch.

[micah-williamson]

Then there needs to be an update to throw when this happens. Otherwise, illegal invocation, infinite digest, browser crashes.

[lgalfaso]

and I would be ok with a change to angular.copy to throw when some elements are being asked to be copied. Eg window, document or a DOM element. This implies that when deep copying a structure that contains any of these elements should throw too.

[micah-williamson]

You can close this out. No need to update my PR because this doesn't solve my problem. Back to angular-ui-sortable!