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
petebacondarwin wants to merge 4 commits into
angular:masterfrom
petebacondarwin:inf-digest-2
Closed

petebacondarwin wants to merge 4 commits into
angular:masterfrom
petebacondarwin:inf-digest-2

Conversation

@petebacondarwin

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@hamfastgamgee can you take a look at this alternative?

Comment thread test/ng/locationSpec.js Outdated

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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

hamfastgamgee commented Jun 11, 2015 via email

Copy link
Copy Markdown

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

I think this should be able to be back-ported

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Updated.

@hamfastgamgee

Copy link
Copy Markdown

LGTM. Still works in my app. :)

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

If Travis is green by the morning I will merge.

@IgorMinar

Copy link
Copy Markdown
Contributor

lgtm especially with all the tests!

@petebacondarwin

Copy link
Copy Markdown
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
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