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

$location.hash('') causes infinite digest loop #9635

Closed
jdkanani opened this issue Oct 15, 2014 · 43 comments
Closed

$location.hash('') causes infinite digest loop #9635

jdkanani opened this issue Oct 15, 2014 · 43 comments

Comments

@jdkanani
Copy link

Angular version: v1.3.0
$location.hash('some_param') works fine but $location.hash('') gives infinite digest loop.

Plunker link: http://embed.plnkr.co/mwtW1ZxDPDgEdj4PE7wj/preview
Related bug: #9629

Problem is any page with Angular v1.3.0 + empty hash, runs into this problem. Try this: https://docs.angularjs.org/api/ng/service/$location#

@jdkanani jdkanani changed the title Bug: $location.hash('') causes infinite digest loop $location.hash('') causes infinite digest loop Oct 15, 2014
@caitp
Copy link
Contributor

caitp commented Oct 15, 2014

Possibly same bug as #9629

@caitp
Copy link
Contributor

caitp commented Oct 15, 2014

Note this is not related to html5Mode, (which can be seen in the repro)

@jdkanani
Copy link
Author

@caitp Yes. of course. My mistake.

@jdkanani
Copy link
Author

I looked into code and I think bug is the effect of commit 0656484

$location.hash('') executes location.href="http://server/" at $browser#url and causes page refresh as we are assigning same URL( without hash or search parameter) to location.href.

Meanwhile at $locationWatch, oldUrl !== $location.absUrl() always yields true and makes digest loop dirty.

Result: It throws an error and causes page refresh as we experienced.

I guess it will also fix bug #9629

@caitp
Copy link
Contributor

caitp commented Oct 18, 2014

the "bug" in the repro is pretty simple, it's an infinite loop. not angular's fault there --- but maybe its weird to chang location when search/hash changes

@jdkanani
Copy link
Author

I agree. Infinite loop is not angular's fault. But location.href = "http://server/" causes page refresh while setting hash and search to '' and null respectively. Because of that oldUrl and $location.absUrl() will not be same in $locationWatch and evalAsync never drains.

Solution is to change location.href=url (here) to history[replace ? 'replaceState' : 'pushState'](state, '', url) (with related state changes of course).

@caitp @tbosch @IgorMinar Any suggestions?

@caitp
Copy link
Contributor

caitp commented Oct 18, 2014

well it's legacy mode, can't really rely on the history api existing until we forget IE9 exists =(

@jdkanani
Copy link
Author

😞 I understand. Anyway, thank you @caitp for your help.

@caitp
Copy link
Contributor

caitp commented Oct 18, 2014

I think we can probably try to prevent $locationChangeXXX if it's just a hash or search query that changed, though.

@jdkanani
Copy link
Author

I don't know how to do that stuff as It took me two days to figure out this problem. :)

Just a thought -- can't we use history API if hash/search is not available and location.href = url if it's available.

Something like this(pseudo code):

if (!empty(url.hash) && !empty(url.search())) {
  history[replace ? 'replaceState' : 'pushState'](state, '', url);
} else {
  location.href = url;
} 

Changing $locationChangeXXX event behavior can be risky and can introduce more bugs.

@caitp
Copy link
Contributor

caitp commented Oct 18, 2014

Just a thought -- can't we use history API if hash/search is not available and location.href = url if it's available.

I don't think this would be good, because they'll test their code on Chrome/Safari/FF or whatever, even though they need to support IE2.5 for whatever reason, and they'll think their code works, but in reality it crashes on IE9.

It sucks, but I don't think we can really do that :(

@jdkanani
Copy link
Author

:( For now, I have changed my code to $location.hash(randomString) to make it work. Let's hope it will get fixed in future.

@caitp
Copy link
Contributor

caitp commented Oct 19, 2014

that seems like a weird way to fix it --- the real fix should basically be, make sure you don't start an infinite loop.

@heavi5ide
Copy link

I'm running into this issue too. I haven't looked into this in any great depth, but from what I can tell this is a problem in Angular and not a user-created infinite loop. I created a self-contained example (based on @jdkanani's plnkr) that shows that simply calling $location.hash('') will cause an infinite loop. @caitp sorry if I'm misunderstanding -- are you saying that this should never be done?

Here's the example in a gist; I can't see hashes in plnkr and find that that adds to the confusion:

https://gist.github.com/heavi5ide/6d4ccfd6c6834362dfd6

@jdkanani
Copy link
Author

My scenario: I want to open comment side modal on article when hash tag is - #comment, scroll to particular comment when hash tag is #comment_commentID, and want to remove hash when user closes comment modal. For now, I have used $location.hash('article') to close comment modal to work around.

I cannot avoid infinite loop without setting non-empty hash. I would say this is very important issue with latest version of angular as it affects every angular application (with v1.3.0).

Any suggestions?

@jdkanani
Copy link
Author

Here is simple repro steps:

  1. open https://docs.angularjs.org/api/ng/service/$location in chrome
  2. open developer tool console and set preserve log to true
  3. Execute location.hash='comment'
    It will add hash #comment in url
  4. Execute location.hash=''
    It will produce infinite digest loop error and redirect to https://docs.angularjs.org/api/ng/service/$location with page refresh

@iyogeshjoshi
Copy link

@jdkanani +1

@young
Copy link

young commented Oct 30, 2014

Running into this issue as well

@petebacondarwin petebacondarwin modified the milestones: 1.3.2, 1.3.x Nov 3, 2014
@petebacondarwin petebacondarwin self-assigned this Nov 3, 2014
@petebacondarwin
Copy link
Member

This is a valid bug (indeed regression) in Angular 1.3.x.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 1, 2014
By retaining a trailing `#` character in the URL we ensure that the browser
does not attempt a reload, which in turn allows us to read the correct
`location.href` value.

Closes angular#9635
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 1, 2014
The url is the same whether or not there is an empty `#` marker at the end.
This prevents unwanted calls to update the browser, since the browser is
automatically applying an empty hash if necessary to prevent page reloads.

Closes angular#9635
@pkozlowski-opensource pkozlowski-opensource modified the milestones: 1.3.5, 1.3.6 Dec 1, 2014
@petebacondarwin
Copy link
Member

That commit does not fix this issue. Sorry

@jdkanani
Copy link
Author

jdkanani commented Dec 2, 2014

@petebacondarwin I think we should use pushState as I mentioned in this comment

@shahata
Copy link
Contributor

shahata commented Dec 2, 2014

BTW, this issue also exists in 1.2.27

@petebacondarwin
Copy link
Member

We can't use pushState on browsers that don't support it.

This bug was caused by trying to fix a bug with IE - see 0656484

I would happily revert that commit if we could find an alternate fix for #9143.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 3, 2014
By using `location.hash` to update the current browser location when only
the hash has changed, we prevent the browser from attempting to reload.

Closes angular#9629
Closes angular#9635
Closes angular#10228
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 3, 2014
…ation urls

Previously if there was a hash fragment but no hashPrefix we would throw an error.
Now we assume that the hash-bang path is empty and that the hash is a valid fragment.

This prevents unnecessary exceptions where we clear the hashBang path, say by
navigating back to the base url, where the $browser leaves an empty hash symbol
on the URL to ensure there is no browser reload.

BREAKING CHANGE:

We no longer throw an `ihshprfx` error if the URL after the base path
contains only a hash fragment.  Previously, if the base URL was `http://abc.com/base/`
and the hashPrefix is `!` then trying to parse `http://abc.com/base/#some-fragment`
would have thrown an error. Now we simply assume it is a normal fragment and
that the path is empty, resulting `$location.absUrl() === "http://abc.com/base/#!/#some-fragment"`.

This should not break any applications, but you can no longer rely on receiving the
`ihshprfx` error for paths that have the syntax above. It is actually more similar
to what currently happens for invalid extra paths anyway:  If the base URL
and hashPrfix are set up as above, then `http://abc.com/base/other/path` does not
throw an error but just ignores the extra path: `http://abc.com/base`.

Closes angular#9629
Closes angular#9635
Closes angular#10228
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 3, 2014
…ation urls

Previously if there was a hash fragment but no hashPrefix we would throw an error.
Now we assume that the hash-bang path is empty and that the hash is a valid fragment.

This prevents unnecessary exceptions where we clear the hashBang path, say by
navigating back to the base url, where the $browser leaves an empty hash symbol
on the URL to ensure there is no browser reload.

BREAKING CHANGE:

We no longer throw an `ihshprfx` error if the URL after the base path
contains only a hash fragment.  Previously, if the base URL was `http://abc.com/base/`
and the hashPrefix is `!` then trying to parse `http://abc.com/base/#some-fragment`
would have thrown an error. Now we simply assume it is a normal fragment and
that the path is empty, resulting `$location.absUrl() === "http://abc.com/base/#!/#some-fragment"`.

This should not break any applications, but you can no longer rely on receiving the
`ihshprfx` error for paths that have the syntax above. It is actually more similar
to what currently happens for invalid extra paths anyway:  If the base URL
and hashPrfix are set up as above, then `http://abc.com/base/other/path` does not
throw an error but just ignores the extra path: `http://abc.com/base`.

Closes angular#9629
Closes angular#9635
Closes angular#10228
@petebacondarwin
Copy link
Member

Take a look at #10308 - and see if this works for you.

See http://plnkr.co/edit/IMZFjT5Mr9NhiMM4fSo0?p=preview

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Dec 3, 2014
…ation urls

Previously if there was a hash fragment but no hashPrefix we would throw an error.
Now we assume that the hash-bang path is empty and that the hash is a valid fragment.

This prevents unnecessary exceptions where we clear the hashBang path, say by
navigating back to the base url, where the $browser leaves an empty hash symbol
on the URL to ensure there is no browser reload.

BREAKING CHANGE:

We no longer throw an `ihshprfx` error if the URL after the base path
contains only a hash fragment.  Previously, if the base URL was `http://abc.com/base/`
and the hashPrefix is `!` then trying to parse `http://abc.com/base/#some-fragment`
would have thrown an error. Now we simply assume it is a normal fragment and
that the path is empty, resulting `$location.absUrl() === "http://abc.com/base/#!/#some-fragment"`.

This should not break any applications, but you can no longer rely on receiving the
`ihshprfx` error for paths that have the syntax above. It is actually more similar
to what currently happens for invalid extra paths anyway:  If the base URL
and hashPrfix are set up as above, then `http://abc.com/base/other/path` does not
throw an error but just ignores the extra path: `http://abc.com/base`.

Closes angular#9629
Closes angular#9635
Closes angular#10228
petebacondarwin added a commit that referenced this issue Dec 4, 2014
…ation urls

Previously if there was a hash fragment but no hashPrefix we would throw an error.
Now we assume that the hash-bang path is empty and that the hash is a valid fragment.

This prevents unnecessary exceptions where we clear the hashBang path, say by
navigating back to the base url, where the $browser leaves an empty hash symbol
on the URL to ensure there is no browser reload.

BREAKING CHANGE:

We no longer throw an `ihshprfx` error if the URL after the base path
contains only a hash fragment.  Previously, if the base URL was `http://abc.com/base/`
and the hashPrefix is `!` then trying to parse `http://abc.com/base/#some-fragment`
would have thrown an error. Now we simply assume it is a normal fragment and
that the path is empty, resulting `$location.absUrl() === "http://abc.com/base/#!/#some-fragment"`.

This should not break any applications, but you can no longer rely on receiving the
`ihshprfx` error for paths that have the syntax above. It is actually more similar
to what currently happens for invalid extra paths anyway:  If the base URL
and hashPrfix are set up as above, then `http://abc.com/base/other/path` does not
throw an error but just ignores the extra path: `http://abc.com/base`.

Closes #9629
Closes #9635
Closes #10228
Closes #10308
@dtritus
Copy link
Contributor

dtritus commented Dec 9, 2014

Still causes page reloading in HTML5 mode, if we loading page with empty hash at the end of URL.

Looks like issue happens here: https://github.com/angular/angular.js/blob/master/src/ng/location.js#L882

For example I load http://localhost/#. I such case $location.absUrl() returns http://localhost/ and $browser.url($location.absUrl(), true); called, which causes page reloading.

@pkozlowski-opensource
Copy link
Member

@dtritus oh, I see. Could you please open a new issue for this one with a short description? Otherwise we might miss this one...

jeffbcross pushed a commit to jeffbcross/angular.js that referenced this issue Dec 17, 2014
By using `location.hash` to update the current browser location when only
the hash has changed, we prevent the browser from attempting to reload.

Closes angular#9629
Closes angular#9635
Closes angular#10228
Closes angular#10308
jeffbcross pushed a commit to jeffbcross/angular.js that referenced this issue Dec 17, 2014
By using `location.hash` to update the current browser location when only
the hash has changed, we prevent the browser from attempting to reload.

Closes angular#9629
Closes angular#9635
Closes angular#10228
Closes angular#10308
tleruitte pushed a commit to tleruitte/angular.js that referenced this issue Jan 14, 2015
Backported from e93710f

The url is the same whether or not there is an empty `#` marker at the end.
This prevents unwanted calls to update the browser, since the browser is
automatically applying an empty hash if necessary to prevent page reloads.

Closes angular#9635
tleruitte pushed a commit to tleruitte/angular.js that referenced this issue Jan 14, 2015
Backported from e93710f

The url is the same whether or not there is an empty `#` marker at the end.
This prevents unwanted calls to update the browser, since the browser is
automatically applying an empty hash if necessary to prevent page reloads.

Closes angular#9635
petebacondarwin pushed a commit that referenced this issue Feb 4, 2015
Backported from e93710f

The url is the same whether or not there is an empty `#` marker at the end.
This prevents unwanted calls to update the browser, since the browser is
automatically applying an empty hash if necessary to prevent page reloads.

Closes #9635
Closes #10748
ggershoni pushed a commit to ggershoni/angular.js that referenced this issue Sep 29, 2015
Backported from 45f5dad

The url is the same whether or not there is an empty `#` marker at the end.
This prevents unwanted calls to update the browser, since the browser is
automatically applying an empty hash if necessary to prevent page reloads.

Closes angular#9635
Closes angular#10748
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.