Angular.js: angular 1.5 + es5-shim + currency filter crashes safari 8.x

[stringbeans]

When using the built-in currency filter in Angular 1.5.x , with es5-shim 4.5.8 and browsing with Safari 8.x the browser will crash...

http://plnkr.co/edit/zshIf4lDR0YgUXmHl8ou?p=preview

(also logged with es5-shim... https://github.com/es-shims/es5-shim/issues/398)

[stringbeans]

after debugging ive found that it occurs in angular 1.4.9 but NOT 1.4.8... so 1.4.9 introduced the breaking change

[ljharb]

https://github.com/angular/angular.js/compare/v1.4.8...v1.4.9 is pretty big. It would be awesome to get a bisect and figure out which commit broke it :-)

[ljharb]

Lots of potential in https://github.com/angular/angular.js/commit/9c49eb131a6100d58c965d01fb08bcd319032229 for infinite loops and relying on non-standard slice/indexOf/replace behavior (no specific clues tho)

[gkalpak]

Does it happen on all 1.5.x versions ? (I.e. from 1.5.0-beta.1 onwards ?)

[stringbeans]

@gkalpak I've found that it occurs on all versions starting from 1.4.9 to 1.5.5. I have not tried the beta versions of 1.5

[gkalpak]

So, what does _"all versions"_ mean ? All stable versions ?

[stringbeans]

@gkalpak sorry, let me clarify.

I have verified that this issue occurs in Angular 1.4.9, 1.4.10, 1.5.0, 1.5.1, 1.5.2, 1.5.3, 1.5.4, and 1.5.5. All stable versions (no release candidates or beta releases).

In Angular 1.4.8 the behaviour is as expected, so the transition from 1.4.8 to 1.4.9 is what caused the breaking change.

[joshbDev]

+1, this is happening for me too

1.5.3 of Angular

[Narretz]

@ljharb can you clarify why do you think infinite loops might be possible in this commit?

[joshbDev]

once I removed the currency filter the site worked again as intended, if that helps

[ljharb]

@Narretz any time you're using a while loop, it's a possibility - loops should be avoided. specifically, your while loops rely on how charAt and splice works - which may not be accounting for shim-corrected standard behavior.

[Narretz]

@ljharb if ES5 is shimmed on all browsers, then why does it only fail on Safari 8?

[ljharb]

I don't know yet :-) but different things are shimmed on different browsers, and sometimes browsers have weird bugs when built in methods are replaced.

[gkalpak]

It is probably because Safari 8 fails some test (spliceWorksWithLargeSparseArrays seems like a good candidate) and get the Array.prototype.splice method patched, while other browsers pass the test and retain their original (non-es5shim) implementations.

[gkalpak]

The issue is indeed an infinite loop caused by "unexpected" behavior of the (shimmed) Array.prototype.splice() method in this line.

Angular (and MDN) expects that if .splice() is called without a 2nd argument (deleteCount), then _"deleteCount will be equal to (arr.length - start)"_.

The es5-shimmed version, treats an omitted (undefined) deleteCount argument as 0, which leaves the array unchanged and leads to the infinite loop.

I'm not sure what the spec says about it. Needs further investigation (but it would be very surprising if es5-shim is "right" here).

[gkalpak]

So, to my great surprise ( :stuck_out_tongue: ), in ES5 omitting the 2nd argument shouldn't do anything (so es5-shim is doing the right™ thing). One could argue that the spec isn't 100% clear on what should happen when .splice() is called with less than 2 arguments, but there is definitely no justification that the ES6 behavior is to be assumed.

In ES6 the behavior is clearly specified and it is as described on MDN.

:unamused:

So, it seems like we need to fix that :smiley:

[ljharb]

The es6-shim fwiw does correct the es5-shim's splice implementation.

However yes, relying on behavior that changed between ES5 and ES6 should be avoided :-)

[gkalpak]

The fix is in. It will be included in the next 1.4.x/1.5.x releases.
Thx everyone :+1: