Prevent infinite digest error in IE7

[angelcabo]

Refactored the replacedUrl variable to store the new URL on both
location.replace and setting location.href directly to handle delays in
actual location value change in IE.

Closes #2802

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[IgorMinar]

Thanks for the PR. I like that we are setting the variable just once now but I don't expect that that fixes ie7. Does setting the variable before updating location.href make a difference?

[IgorMinar]

Also I can't find a CLA from you or your company. Can you please sign it?

[angelcabo]

CLA signed.

I'm using angular on a current project that requires IE7 support. In testing I can verify there seems to be a race condition that happens when setting location.href directly (similar to what is being seen with the replace method) and then returning the location.href value on the next call to the getter. The digest cycle gets caught in an infinite loop at that point because the url setter keeps hold of the lastBrowserUrl variable that should have been set in the last call to set url and returns without attempting to set location.href again while the $locationWatcher function keeps firing because the location change has finally occurred.

Doesn't seem like there is any benefit in setting it before or after the call to location.href. As long as after the set the $browser object attempts to return the local variable first on the subsequent getter call. The change seems to fix the issue for me in IE 7.

[IgorMinar]

so in that case the code you fixed should already work for you in IE7. ever since we merged dca2317 no? your change cleans up the code, but even the version before your change should fix ie7

[angelcabo]

The problem still existed for me because of line 168 in the commit you reference. The variable replacedUrl was being nulled out unless the location change was flagged as a "replace". In this case, on the next getter call replacedUrl would be null and the get would delegate down to location.href which would not be set yet. Moving the replacedUrl variable up a level should capture all location changes in non-modern browsers instead of only capturing the location change when location is being replaced.

[angelcabo]

The underlying issue is that any change to the window.location object seems to be on a delay in IE. Whether you're setting it through the replace function or setting the href attribute directly. I'm not sure what the underlying root cause for this delay is (either due to native implementation or some other issue related to the digest cycle) but nevertheless the dca2317 commit only solves the race condition when the location change is replacing history.

[IgorMinar]

I see. thanks for the explanation

[IgorMinar]

ok, I looked at the code again and I realized that this would break window.location change observation.

the url() method is called from fireUrlChange which is called on popstate/hashchange events or by polling method for legacy browsers. in any case by the time fireUrlChange fires, the location.href should be up to date so we should get real value. if we don't (and that's what we do today before and after your change) we'll miss updates to the url that happen outside of the $browser or $location (typically by user clicking on a link or editing the url by hand).

[IgorMinar]

I wonder if it would be sufficient to just set newLocation to null as the first thing in fireUrlChange function

[angelcabo]

Good catch. I wrote a test to isolate the bug you mention and it seems resetting newLocation to null as the first thing in the fireUrlChange function works. I amended the PR with a test and fix. Let me know what you think.

[btford]

Sorry for the delay in resolving this.

I went over it with @IgorMinar today and it looks good. I made a few changes to clarify the test, and landed this as d707114.

Thanks!