fix($location): fix infinite recursion/digest on URLs with special characters

[jbedard]

Some characters are treated differently by $location compared to $browser and the native browser. When comparing URLs across these two services this must be taken into account.

Fixes #16592

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)
$browser and $location persist URLs differently, so comparing them sometimes treats them as not-equal when they should be considered equal

What is the new behavior (if this is a feature change)?
$location compares a normalized version of the URLs

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Fix/Feature: Docs have been added/updated
• Fix/Feature: Tests have been added; existing tests pass

Other information:
All the added tests previously failed, some due to infdig and others due to RangeError: Maximum call stack size exceeded.

The tests all use the single-quote to trigger the error, but I assume there are similar cases where $location and window.location.href have different escaping/encoding.

#16606 partially fixed this (which I still don't quite understand), but some of the tests in this PR still failed.

[Narretz]

Since this seems at least related to ~#16611~~ #16606 , I've run the tests in this here PR with the fix from #16611. And only https://github.com/angular/angular.js/pull/16611/files#diff-96e97950408af743f7eb96283d0aa11aR1010 still failed. So maybe we can remove some of these comparisons? Since urlResolve involves DOM manipulation, I was already wondering if they might be too expensive when the url changes often.

[jbedard]

You mean related to #16606 (I think you typed the wrong #)? I know the fix from #16606 seemed to solve this issue, but I still think there are 2 separate problems here (and maybe we need more tests to show that).

#16611: $location compares $location.absUrl() vs $browser.url() and these have the potential to be different because $location adds some URL beautification (less encoding) where $browser uses the raw location.href.

#16606: in $browser.url(newVal) the newVal was directly assigned to lastBrowserUrl without any normalization. Where in other spots lastBrowserUrl = $browser.url() (a normalized version) and in some places it checks lastBrowserUrl === $browser.url() which could then be wrong/inconsistent based on the inconsistent normalizing of lastBrowserUrl.

One of those "normalizations" is standardizing ' vs %27, which I think just happens to make quotes the same in $browser.url() vs $location.absUrl(), which will fix the infinite recursion for quotes but maybe not other things (especially the manual stuff 0, 1)

[Narretz]

Yes, I meant #16606 . The point still stands though. When you combine both PRs, you actually only need to compare the normalized urls here + and include the changes from #16606 : https://github.com/angular/angular.js/pull/16611/files#diff-06a39a7c214933c36a5c4b080ef4d56dR1006. I just don't think it's necessary to include more code than we need.

[jbedard]

You mean only invoke urlsEqual in that one location? I think that's ok, or might even be more correct?

[Narretz]

That's right only the one Invocation I linked to

[jbedard]

Since that's the only one done in a watcher I think that's fine 👍

Can you think of any tests to add that would enforce that?

[Narretz]

What exactly should the tests enforce? That we only need the single urlsEquals?

[jbedard]

If we decide that we should vs should not use urlsEquals it would be nice if we had tests showing why. I guess it's not as necessary if we don't make any changes though. I'll revert the unnecessary urlsEqual changes...

[jbedard]

3 tests were failing in IE9-11, Edge and Mobile Safari 10 (but not Mobile Safari 11!). See https://travis-ci.org/angular/angular.js/jobs/401137341

These browsers seem to not escape quotes in location.href which $browser.url() returns. This bug+fix was in $location so I don't think those expect()s are necessary so I just removed them (7aac363). The modified tests all fail without the fix still (with the infinite recursion/max stack size error).

Look ok before sqush+merge?

[petebacondarwin]

LGTM

[sod]

As someone that has a shop based on angularjs, this bug crashes the website as soon as someone searches for something like o'reilly. When do you plan on releasing this?

[petebacondarwin]

We should have a new release out later this week @sod. Good to see you are keeping up with the latest versions!