Fix infinite digest loop triggered by changing $location.path()

[isabo]

Web apps that use the $route module will typically not work on Android 2.x without this fix.

On Android 2.x, using $location.path() to change the url occasionally triggers an infinite digest iterations exception for no good reason.

The problem is caused by a faulty assumption in $browser.onUrlChange(). The assumption is that the hashchange event will fire only as a result of non-Angular actions, e.g. a human editing the browser's url. Unfortunately in Android 2.2 and 2.3, this event occasionally fires when Angular's own $browser.url() changes the url.

The knock-on effect is a race condition between $location's two monitoring loops:

• One uses $browser.onUrlChange() to get notified about non-Angular url changes and synchs $location's inner data.
• The other, $locationWatch(), watches for changes in $location.absUrl() and synchs them to $browser.

They end up reversing each other's changes in a vicious cycle until Angular cuts them off. Meanwhile the browser gets stuck and goes nowhere.

Closes #3814

[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!

[isabo]

Just signed CLA.
name = Itzchak Sabo

[bcharp]

Had the same problem on Kindle Fire on Android 2.3. This solve it. 👍

[doup]

Fixed on Nexus One 2.3.6 😂
Thank you!

[bcharp]

Strangely it works well on 2.3, but I still have routing problem on android 4.0.4, no idea why yet.

[isabo]

@bcharp : Does it appear to be the same problem?

My fix works only with Android versions that don't properly support the history API, namely version < 4. For it to take effect on everything, and it should not do any harm, add the ignoreHashChange = true/false around lines 157-162:

ignoreHashChange = true;
if (replace) history.replaceState(null, '', url);
else {
  history.pushState(null, '', url);
  // Crazy Opera Bug: http://my.opera.com/community/forums/topic.dml?id=1185462
  baseElement.attr('href', baseElement.attr('href'));
}
ignoreHashChange = false;

Does that fix it?

[jolealdoneto]

Thanks a lot, works fine for me!

[isabo]

@xrg I see that you increased the scope of the fix. I thought of doing this, but didn't because:

1. It is is unnecessary, because the problem does not exist for Android 4.x (at least, I haven't been able to reproduce it). Fixes for quirks like this should IMHO generally be restricted to the minimal circumstances in which they happen. Otherwise we get bogged down in too many layers of defensive coding and cannot progress easily.
2. In addition, wrapping this section in a try ... finally block introduces a potential problem -- you are indeed ensuring that our new flag gets reset in all cases, but in the case of an exception, the .history object still gets left in an unpredictable state, as well as replacedUrl, so the error handler has not really solved anything -- in such a case we anyway have much bigger problems.
   If error handling is actuallly necessary here (does the browser ever trigger an error when 'history' or 'location' methods are called?) it would need to be quite a bit more comprehensive, and our fix does not affect the necessity for this.

[xrg]

Indeed, I have written "1" because I was trying to eliminate the possibility of url change in a wider sense. Turned out that it was not the real problem, at the end of the day.

About "2", it is a subjective fix, since (as you say) the browser will have even bigger trouble than a stale flag. But still, cleaning the stale flag wouldn't do any harm, would it? The exception would propagate anyway.

[isabo]

I agree that your expanded fix is fine -- it works and does not cause any problems. I think our opinions differ on what is considered optimal for a fast-moving framework such as Angular, and that's OK.

[xrg]

Agreed. Let's move on to the rest of unresolved bugs!

[olgasmola]

Many thanks, also works for me!

[machard]

Is this problem solved with 1.2.1 ?

[peey]

Why has this not been merged yet?

[caitp]

@peey It's bitrotten, there are no tests, and our CI is not running on any mobile devices currently anyways (although hopefully this will change soon)

[pkozlowski-opensource]

So this was not complete (missing tests) for a while and got bit-rotten over 6 months ago so I don't think we can consider landing this one.

@isabo there were recently tons of fixes about #-handling and infinite loops in master so please try the latest version to see if it fixes problems for you. If not please open a new issue with a reproduce scenario (or even better - a pull request with tests!). Thnx!