Watch File on Safari causes infinite $digest loop

[hugomallet]

Hi,

On Safari with angular 1.3, watching changes on a JavaScript File object throws:

[Error] Deprecated attempt to access property 'name' on a non-File object.
    equals (angular.js, line 934)
    $digest (angular.js, line 14036)
    $apply (angular.js, line 14304)
    setFile (runner, line 33)
    handleFiles (runner, line 39)
    eventHandler (angular.js, line 2979)

and causes a infinite $digest loop.

Reproducable here:
http://jsbin.com/vaketipeyo/1/edit?js,console,output
Choosing a file will produce the error

Angular version: > 1.3

My config:
Browser: safari 8.0.2
OS: OS X 10.10.1

This issue might be related to: #10370 #10210

[lgalfaso]

this boils down to two parts:

• when a copy is performed, the same prototype is used, but the properties on the prototype are not copied, this is here https://github.com/angular/angular.js/blob/master/src/Angular.js#L765
• when comparing the previous and next value, all keys are compared, regardless if they come from the object or the prototype https://github.com/angular/angular.js/blob/master/src/Angular.js#L859

I think that copy is doing the right thing, as we should expect objects to behave (BTW, the very root issue is that Safari is putting properties on the prototype and not on the instance, in this case, the property name)
The only other option left is that equals does not compare the properties from the prototype. This is a breaking change :(

@petebacondarwin how do you feel about making the change to equals for 1.4?

[caitp]

Some background on why this happens:

File (and many others) are what used to be called host objects, parts of the platform mainly implemented in C++ --- they have JS bindings automatically generated from IDL files, like this https://github.com/WebKit/webkit/blob/master/Source/WebCore/fileapi/File.idl

Those properties are defined as accessors, and they'll try to invoke the accessors implemented in C++ --- the problem is that Object.create(File.prototype) isn't going to construct the C++ backend for this class, thus we don't really have these accessors, thus JSC decides to throw here (see https://github.com/WebKit/webkit/blob/master/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm#L2206-L2211 --- if it can't cast to the host object, it's gonna throw.

So we aren't going to construct that host object with Object.create(File.prototype), because VMs are a bit silly (if we do, we'll have an object with the right prototype, but no C++ backing for it). We can circumvent that particular error by ignoring properties on the prototype when iterating, but I'm not sure that's a change worth making.

A good idea is to just not deep-watch host objects if you want to make sure Safari doesn't explode :(

[petebacondarwin]

How does this fare with $watchCollection instead of deepWatch?

[lgalfaso]

@petebacondarwin everybody is happy is $watchCollection is used

[caitp]

$watchCollection doesn't really make sense here

[lgalfaso]

@caitp why do you think it does not make sense to use $watchCollection? Using it would check for changes in the properties values (and we would never clone File). I am not saying this is a perfect solution, just that we do not run into the $digest error and that the listener is triggered when the file changes

[caitp]

because it's very different, semantically

[lgalfaso]

I think that everybody knows that $watchCollection and $watch with deepWatch=true are different things. Also, everybody is aware that using a different function that works is a different way does not solve the original issue, that is that Safari is reflecting properties on the prototype and not on the instance.

Is this what you mean when you say that they are semantically different?

Now, the question is if we should solve this or not. If we should solve this then how, and if not then what are the possible work-arounds. The one solution I can think of (without adding special rules for host objects) is #10524 (comment). This is not perfect and for sure it is not for free. If we are not going to solve this then we have to be clear that Safari does not behave well when you are deep watching a host object, and have solution for the community to use. In this case, using $watchCollection is a possible work-around

[caitp]

semantically different means exactly what it sounds like, the semantics of the two functions are different, and are not interchangeable. This isn't a real workaround, because it will only work under certain conditions (when array or object valued properties aren't considered, and when the prototype chain of the object being watched doesn't matter). Beyond that, you're telling people to write text which does not represent their intentions, or the actions they're actually performing, which is always a bad recipe.

Unfortunately, there isn't much we can do about it. Fixing one use-case harms other use cases. There are other solutions, which are a bit more complicated, but work better.

You can watch the specific properties you care about. If you need to watch multiple properties, you can use $watchGroup and supply a set of expressions to watch. That's probably the best case scenario for deep-watching host objects.

[petebacondarwin]

It seems that this issue is a corner case that has a couple of workarounds: i.e. to use $watchCollection or to watch individual properties of the file object (via $watch or $watchGroup) so we are not going to prioritize a fix for this. @hugomallet - if you have time to look into it and come up with a decent solution then do please send in a pull request.

[wesleycho]

A suitable workaround for something like this is to do

$scope.$watch(() => scope.file && scope.file.name, (fileName) => ...)

IMO this can probably be closed.

[Narretz]

Yes, I agree this is a very uncommon scenario with a number of workarounds, so no need to change the core for this.