fix($location): prevent infinite digest with IDN urls in Edge #15235

Narretz · 2016-10-09T20:20:04Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix

What is the current behavior? (You can also link to an open issue here)
Internationalized Domain Name Urls (IDN), e.g. with an Umlaut, in Edge lead to an infinite digest on page load.

What is the new behavior (if this is a feature change)?
No Infdig

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)

Fixes #15217

Note:
We don't test on Edge yet, but I've tested this locally with the https://github.com/nickmccurdy/karma-edge-launcher

jbedard · 2016-10-09T20:25:06Z

See my comment on the bug. I'm not sure if that really matters, depends where this method is used.

jbedard · 2016-10-09T20:25:32Z

src/ng/location.js

@@ -49,7 +49,7 @@ function parseAppUrl(relativeUrl, locationObj) {
 }

 function startsWith(haystack, needle) {
-  return haystack.lastIndexOf(needle, 0) === 0;
+  return haystack.indexOf(needle, 0) === 0;


Can drop the 0 param.

Internationalized Domain Urls, for example Urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version. indexOf does, though. Fixes angular#15217

petebacondarwin · 2016-10-10T05:06:05Z

core-js has an interesting patch for startsWith:

str.slice(index, index + search.length) === search;

what do you think?

gkalpak

If were concerned about perfromance, we could "sniff" if the implementation of lastIndexOf is broken and fall back to indexOf or slice (I am not sure which on is faster).

But this function isn't on a performance-critical path, so it might not be worth "super-optimizing".

gkalpak · 2016-10-10T07:51:27Z

src/ng/location.js

@@ -49,7 +49,7 @@ function parseAppUrl(relativeUrl, locationObj) {
 }

 function startsWith(haystack, needle) {
-  return haystack.lastIndexOf(needle, 0) === 0;
+  return haystack.indexOf(needle) === 0;


Maybe it is worth adding a comment to prevent someone from "optimizing" it back in the future.

petebacondarwin · 2016-10-10T13:35:48Z

I know that micro-performance is not a good way to measure but....

Testing the following strings against abcdefghijklmnopqrstuvwxyz yields:

RegExp#test: abc x 2,746,460 ops/sec ±1.39% (81 runs sampled)
String#indexOf abc x 11,012,610 ops/sec ±2.09% (76 runs sampled)
String#startsWith abc x 16,389,041 ops/sec ±1.64% (78 runs sampled)
String#slice abc x 19,636,781 ops/sec ±2.81% (74 runs sampled)
String#lastIndexOf abc x 14,675,551 ops/sec ±2.43% (78 runs sampled)
RegExp#test: xyz x 3,138,299 ops/sec ±1.62% (80 runs sampled)
String#indexOf xyz x 11,157,966 ops/sec ±1.55% (80 runs sampled)
String#startsWith xyz x 16,344,142 ops/sec ±2.90% (79 runs sampled)
String#slice xyz x 22,107,036 ops/sec ±1.58% (80 runs sampled)
String#lastIndexOf xyz x 14,774,806 ops/sec ±1.85% (79 runs sampled)
RegExp#test: 123 x 2,990,055 ops/sec ±2.17% (78 runs sampled)
String#indexOf 123 x 12,009,833 ops/sec ±1.71% (78 runs sampled)
String#startsWith 123 x 16,548,223 ops/sec ±2.40% (81 runs sampled)
String#slice 123 x 22,226,588 ops/sec ±1.66% (78 runs sampled)
String#lastIndexOf 123 x 14,810,254 ops/sec ±1.81% (78 runs sampled)

with string slice winning out easily in all cases (even against lastIndexOf and startsWith :-) )

gkalpak · 2016-10-10T13:51:45Z

I know that micro-performance is not a good way to measure

(No buts 😛)

Narretz · 2016-10-10T19:29:34Z

I can put in the slice version as startsWith, without considering any existing startsWith fn. Btw, the bug is reported here: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/ I'll add a comment to the line

Edge Bug url: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/

petebacondarwin · 2016-10-15T15:00:33Z

LGTM

Internationalized Domain Urls, for example urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version when the search string is the same as the haystack string. The patch uses an implementation based on core.js: https://github.com/zloirock/core-js/blob/v2.4.1/modules/es6.string.starts-with.js#L16 Edge Bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/ Fixes #15217 PR #15235

Internationalized Domain Urls, for example urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version when the search string is the same as the haystack string. The patch uses an implementation based on core.js: https://github.com/zloirock/core-js/blob/v2.4.1/modules/es6.string.starts-with.js#L16 Edge Bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/ Fixes angular#15217 PR angular#15235

Internationalized Domain Urls, for example urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version when the search string is the same as the haystack string. The patch uses an implementation based on core.js: https://github.com/zloirock/core-js/blob/v2.4.1/modules/es6.string.starts-with.js#L16 Edge Bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/ Fixes #15217 PR #15235

Internationalized Domain Urls, for example urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version when the search string is the same as the haystack string. The patch uses an implementation based on core.js: https://github.com/zloirock/core-js/blob/v2.4.1/modules/es6.string.starts-with.js#L16 Edge Bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/ Fixes angular#15217 PR angular#15235

googlebot added the cla: yes label Oct 9, 2016

jbedard reviewed Oct 9, 2016

View reviewed changes

fix($location): prevent infinite digest with IDN urls in Edge

3b0ac77

Internationalized Domain Urls, for example Urls with Umlaut (Ä, Ö, Ü) cause infinite digest in Edge 38.14393.0.0 because lastIndexOf doesn't work correctly in this version. indexOf does, though. Fixes angular#15217

Narretz force-pushed the fix-location-edge branch from 1a3aa28 to 3b0ac77 Compare October 9, 2016 20:36

Narretz added type: bug needs: review browser: Edge frequency: low labels Oct 9, 2016

petebacondarwin added this to the 1.6.x (post 1.6.0) milestone Oct 10, 2016

gkalpak reviewed Oct 10, 2016

View reviewed changes

fixup: faster implementation

2a3f6e6

Edge Bug url: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9271625/

Narretz merged commit 705afcd into angular:master Oct 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($location): prevent infinite digest with IDN urls in Edge #15235

fix($location): prevent infinite digest with IDN urls in Edge #15235

Narretz commented Oct 9, 2016 •

edited

Loading

jbedard commented Oct 9, 2016

jbedard Oct 9, 2016

petebacondarwin commented Oct 10, 2016

gkalpak left a comment

gkalpak Oct 10, 2016 •

edited

Loading

petebacondarwin commented Oct 10, 2016 •

edited

Loading

gkalpak commented Oct 10, 2016

Narretz commented Oct 10, 2016

petebacondarwin commented Oct 15, 2016

fix($location): prevent infinite digest with IDN urls in Edge #15235

fix($location): prevent infinite digest with IDN urls in Edge #15235

Conversation

Narretz commented Oct 9, 2016 • edited Loading

jbedard commented Oct 9, 2016

jbedard Oct 9, 2016

Choose a reason for hiding this comment

petebacondarwin commented Oct 10, 2016

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

petebacondarwin commented Oct 10, 2016 • edited Loading

gkalpak commented Oct 10, 2016

Narretz commented Oct 10, 2016

petebacondarwin commented Oct 15, 2016

Narretz commented Oct 9, 2016 •

edited

Loading

gkalpak Oct 10, 2016 •

edited

Loading

petebacondarwin commented Oct 10, 2016 •

edited

Loading