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

fix($location): do not get caught in infinite digest in IE9 #12083

Closed
wants to merge 4 commits into from

Conversation

petebacondarwin
Copy link
Contributor

The problem appears to occur when we have triggered a reload (via replace()) that updates the reloadLocation but then before the reload happens we trigger another change to the URL that would not have triggered a reload if we had already reloaded. In this case, the $browser is ignoring the reloadLocation because it thinks that we will no longer reload.

So to be more explicit:

Navigate to http://my.server.com/Home
Because we are on IE9 this get rewritten (with replace()) to http://my.server.com/#/Home, which would cause a reload on IE9
The SuccessChangeHandler kicks in and then changes the $location.path to /, i.e. the URL gets changed to http://my.server.com/#/`.
Since http://my.server.com/#/Home -> http://my.server.com/#/ would not normally trigger a reload we don't update the reloadLocation, which gets stuck at http://my.server.com/#/Home
We get stuck in an infinite digest as the $location keeps trying to update the URL to http://my.server.com/#/ but failing because the reloadLocation is stuck at http://my.server.com/#/Home
This is why @hamfastgamgee's fix appears to work. When we see that we are in an infinite loop and there is a reloadLocation then we force it to change to the proper URL.

A better solution is simply to realize that we are updating a URL in $browser that is expected to cause a reload by changing the line at https://github.com/angular/angular.js/blob/v1.4.0/src/ng/browser.js#L150:

  if ($sniffer.history && (!sameBase || !sameState)) {
    history[replace ? 'replaceState' : 'pushState'](state, '', url);
    cacheState();
    // Do the assignment again so that those two variables are referentially identical.
    lastHistoryState = cachedState;
  } else {
    if (!sameBase) {
      reloadLocation = url;
    }
    if (replace) {
      location.replace(url);
    } else if (!sameBase) {
      location.href = url;
    } else {
      location.hash = getHash(url);
    }
  }

to

  if ($sniffer.history && (!sameBase || !sameState)) {
    history[replace ? 'replaceState' : 'pushState'](state, '', url);
    cacheState();
    // Do the assignment again so that those two variables are referentially identical.
    lastHistoryState = cachedState;
  } else {
    if (!sameBase || reloadLocation) {
      reloadLocation = url;
    }
    if (replace) {
      location.replace(url);
    } else if (!sameBase) {
      location.href = url;
    } else {
      location.hash = getHash(url);
    }
  }

In other words we check not only whether the base has changed but also whether we are in the middle of a reload any way.

Closes #11439
Closes #11935
Closes #11675

@petebacondarwin
Copy link
Contributor Author

@hamfastgamgee can you take a look at this alternative?

$location.path('/').replace();
}
});
$rootScope.$digest();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check that the change handler actually gets called and that the path is updated correctly

@hamfastgamgee
Copy link

Well, leaving aside the failed tests, I can verify that it does in fact fix my case. I had tried a similar fix previously (never exactly this; it was mostly removing the sameBase check) and had at least three failed tests, but it appears that your fix at least satisfies most browsers on travis. :)

@petebacondarwin
Copy link
Contributor Author

OK, I'll fix up these tests and then if you can give it a once-over, I'll be able to merge it tomorrow

@hamfastgamgee
Copy link

hamfastgamgee commented Jun 11, 2015 via email

@petebacondarwin
Copy link
Contributor Author

I think this should be able to be back-ported

@petebacondarwin
Copy link
Contributor Author

Updated.

@hamfastgamgee
Copy link

LGTM. Still works in my app. :)

@petebacondarwin
Copy link
Contributor Author

If Travis is green by the morning I will merge.

@IgorMinar
Copy link
Contributor

lgtm especially with all the tests!

@petebacondarwin
Copy link
Contributor Author

And it's in! Backporting to 1.3...
Then to check whether all those other infinite digest issues are fixed too...

petebacondarwin added a commit that referenced this pull request Jun 12, 2015
Thanks to @hamfastgamgee for getting this fix in place.

Closes #11439
Closes #11675
Closes #11935
Closes #12083
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@petebacondarwin petebacondarwin deleted the inf-digest-2 branch November 24, 2016 09:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IE9 infinite $digest bug if loading a base URL without a trailing slash
3 participants