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

angular 1.5 + es5-shim + currency filter crashes safari 8.x #14467

Closed
stringbeans opened this issue Apr 19, 2016 · 18 comments
Closed

angular 1.5 + es5-shim + currency filter crashes safari 8.x #14467

stringbeans opened this issue Apr 19, 2016 · 18 comments
Labels
browser: Safari component: filters frequency: low needs: investigation severity: broken expected use severity: regression type: bug
Milestone
1.5.6

Comments

@stringbeans
Copy link

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... es-shims/es5-shim#398)
@stringbeans
Copy link
Author

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
Copy link

ljharb commented Apr 19, 2016

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
Copy link

ljharb commented Apr 19, 2016 •
edited
Loading

Lots of potential in 9c49eb1 for infinite loops and relying on non-standard slice/indexOf/replace behavior (no specific clues tho)

@gkalpak gkalpak added this to the Backlog milestone Apr 20, 2016
@gkalpak
Copy link
Member

gkalpak commented Apr 20, 2016

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

@stringbeans
Copy link
Author

@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
Copy link
Member

gkalpak commented Apr 20, 2016

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

@stringbeans
Copy link
Author

@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
Copy link

joshbDev commented Apr 20, 2016 •
edited
Loading

+1, this is happening for me too

1.5.3 of Angular

@Narretz
Copy link
Contributor

Narretz commented Apr 21, 2016

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

@Narretz Narretz modified the milestones: 1.5.6, Backlog Apr 21, 2016
@joshbDev
Copy link

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

@ljharb
Copy link

ljharb commented Apr 21, 2016

@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
Copy link
Contributor

Narretz commented Apr 21, 2016

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

@ljharb
Copy link

ljharb commented Apr 21, 2016

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
Copy link
Member

gkalpak commented Apr 21, 2016 •
edited
Loading

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
Copy link
Member

gkalpak commented Apr 21, 2016 •
edited
Loading

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
Copy link
Member

gkalpak commented Apr 21, 2016 •
edited
Loading

So, to my great surprise ( 😛 ), 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.

😒

So, it seems like we need to fix that 😃

@ljharb
Copy link

ljharb commented Apr 22, 2016 •
edited
Loading

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 added a commit to gkalpak/angular.js that referenced this issue Apr 22, 2016
@gkalpak
fix(filters): always call splice() with 2 arguments or more
bf1d486 
When calling `.splice()` without a 2nd argument (`deleteCount`), most browsers will splice to the
end of the array. As it turns out, this is the behavior specified in [ES2015][es6]. In [ES5][es5],
the spec seems to imply that nothing should happen.

To avoid inconsistent behavior among browsers implementing different versions of the EcmaScript
standart or when ES5 shims are included (e.g. https://github.com/es-shims/es5-shim/), this commit
ensures that all calls to `.splice()` provide at least two arguments.

[es5]: http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.12
[es6]: http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.splice

Fixes angular#14467
@gkalpak gkalpak mentioned this issue Apr 22, 2016
Closed
3 tasks
gkalpak added a commit that referenced this issue Apr 22, 2016
@gkalpak
fix(filters): always call splice() with 2 arguments or more
6f1fae8 
When calling `.splice()` without a 2nd argument (`deleteCount`), most browsers will splice to the
end of the array. As it turns out, this is the behavior specified in [ES2015][es6]. In [ES5][es5],
the spec seems to imply that nothing should happen.

To avoid inconsistent behavior among browsers implementing different versions of the EcmaScript
standart or when ES5 shims are included (e.g. https://github.com/es-shims/es5-shim/), this commit
ensures that all calls to `.splice()` provide at least two arguments.

[es5]: http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.12
[es6]: http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.splice

Fixes #14467

Closes #14489
gkalpak added a commit that referenced this issue Apr 22, 2016
@gkalpak
fix(filters): always call splice() with 2 arguments or more
4e735e5 
When calling `.splice()` without a 2nd argument (`deleteCount`), most browsers will splice to the
end of the array. As it turns out, this is the behavior specified in [ES2015][es6]. In [ES5][es5],
the spec seems to imply that nothing should happen.

To avoid inconsistent behavior among browsers implementing different versions of the EcmaScript
standart or when ES5 shims are included (e.g. https://github.com/es-shims/es5-shim/), this commit
ensures that all calls to `.splice()` provide at least two arguments.

[es5]: http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.12
[es6]: http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.splice

Fixes #14467

Closes #14489
@gkalpak
Copy link
Member

gkalpak commented Apr 22, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
browser: Safari component: filters frequency: low needs: investigation severity: broken expected use severity: regression type: bug
Projects
None yet
Milestone
1.5.6
Development

Successfully merging a pull request may close this issue.

fix(filters): always call splice() with 2 arguments or more gkalpak/angular.js
5 participants
@ljharb @Narretz @stringbeans @joshbDev @gkalpak