-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix($browser): prevent infinite $digest from no trailing slash in IE9 #11675
Conversation
fix($browser): prevent infinite $digest from no trailing slash in IE9 This fix prevents IE9 from throwing an infinite $digest error when the user accesses the base URL of the site without a trailing slash. Suppose you owned http://www.mysite.com/app and had an Angular app hosted in a subdirectory "app". If an IE9 user accessed http://www.mysite.com/app infinite $digest errors would be thrown on the console, but the app itself would eventually resolve properly and work fine. Now the infinite $digest errors will not be thrown. Closes angular#11439
Is there anything I need to do to aid in the lifecycle of this issue, @Narretz? It'd really be preferable if this was included in a 1.3.x release (you'll notice the changes are actually to the v1.3.x branch) due to the overhead of retesting our entire app to move to 1.4, if the powers that be are even okay with moving to something that's not yet marked as the stable branch. :) |
Ok, so it would be good if this applies to 1.4.x, too. Do you think you could apply the tests to 1.4.x and see if they pass? |
I'm moderately certain that the codepath hasn't changed significantly, but On Tue, Apr 28, 2015 at 3:56 PM, Martin Staffa notifications@github.com
|
With just the new tests in place, 1.4.x fails the 6 tests I would expect it to fail, and it succeeds in the 6 tests I would expect it to succeed. So this issue definitely persists into 1.4.x. |
And with merged identical changes made in 1.4.x, the new tests all pass. |
At the risk of potentially being annoying here, is there a possible ETA for this fix to land (or be reviewed, fixed up, and then land)? I'm trying to figure out if I should be forking from 1.3.15 locally for our app since we have a public release coming up in about 5 weeks. Ah, corporate life... :) |
I am working on it @hamfastgamgee - sorry about the delay. Give me a few days. |
No worries at all. Let me know if there's anything I can help with. (I did do the local fork for our app, just in case. :) ) |
Anything more I can help with here? Looking at some other checkins, I'm wondering if the |
Agh, I got pulled into ng2 doc generation stuff and haven't had a chance to look at this yet. It is the first item after we get the next RC release out. |
Now that rc.2 is out, anything I can do to assist here? (Let me know if I need to just give this a while. Like I said, I have my local workaround, but I know my management would prefer us not to be forked if possible. :) ) |
Thursday morning (GMT). It's in the diary |
Looking at this now. |
I don't disagree in principle, but I have been entirely unable to find a way to trigger it without ngRoute coming into play. The only thing I can think of is maybe wiring up a manual $locationChangeSuccess watcher that basically emulates ngRoute. Thoughts? |
OK, I really want to get a reproduction (in unit test) of this without ngRoute. I guess it will involve setting up a |
While I'm working on that: Do you want the new |
I initially tried just setting that caching variable more often, and a bunch of other tests started failing. This was the only way I could find to "solve" the issue that always succeeded. I suppose potentially we could check before/after values inside the $evalAsync in $locationWatch and fire off the reset function if we detect that a watcher of $locationChangeSuccess has changed the value. |
@hamfastgamgee - thanks for all this work. I am going to take another look today. |
And thank you for taking a look at it. And of course, "correct" always On Fri, May 22, 2015 at 6:25 AM, Pete Bacon Darwin <notifications@github.com
|
I'm currently trying to see if I can update angular-mocks to have its mock browser act more like the actual $browser, and while it's certainly possible that I am messing something up, the volume of tests that seem to be throwing infinite $digest errors from just making the mock more like the real one is a bit concerning. Makes me think there are a lot more edge cases currently in the code that suffer from infinite $digest problems than just the ones I was targeting. I'm trying to see if I can clear all of that up, because if I can, I think the fix is likely to make this substantially more robust than it was. (Which doesn't mean what I have currently submitted here is "wrong".) |
@hamfastgamgee - I have provided a unit test in here #11935. Please take a look |
also fix downstream impacts and tests
Okay, in this latest version, angular-mocks is updated to have the mock browser's url setter actually act like the real $browser.url(). This includes use of the caching variable and setting it only when the sameBase check is false, which is the actual underlying root cause of this issue. That caused a bunch of other tests to break with the original "targeted only at HTML5 non-history mode" check, so I have made the code run in more circumstances. All of the tests succeed now, running through a mock browser that actually mocks $browser. :) I also included the unit test from #11935 at the bottom, porting it to use the existing initBrowser() function that is already in locationSpec. (Curiously, if I didn't do that and just pulled over the mockBrowser() function, it threw an error trying to set the length of Browser.pollFns[]. Don't know why.) I'm totally cool with removing my tests, or maybe expanding your test to cover more cases just to make sure we have coverage of all the cases (HTML5 vs not, and history browser vs not). But all of them pass at the moment, which is the important thing. @petebacondarwin - Can you take another look? |
Or it'll bomb on IE9 for some reason. Ugh... |
Well, I have a version that satisfies all the tests, but I'm not entirely sure if I'm happy with the mock browser, so I absolutely welcome feedback. In particular, passing self.$$url into the listener() callback in onUrlChange is not what the actual |
@hamfastgamgee - thanks again for the work you are doing on this. Unfortunately, by mocking out the browser like this we are back to the situation where we are not actually testing the invalid behaviour. If you comment out the "fix" in $location, the last test in locationSpec.js, i.e. the one that I wrote passes, which it should not, given that in real life this would fail. So somewhere in the mock browser we are hiding the real functionality that needs to be tested. |
But, if we include the version from my PR as-is then this does indeed fail with infinite-digest error. |
// The first $browser.url() set doesn't reset $browser.url()'s caching variable appropriately, | ||
// so after the second call to it, logic kicks in to force an update of the caching variable. | ||
// Hence, two calls to $browser.url()'s setter is exactly correct here. | ||
expect($browserUrl.calls.length).toBe(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me sad. Either we need to fix the logic or we have to give up on only calling $browser.url() once per digest...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, on the plus side, it only happens for hashbang URLs, but I know what you mean. It's really the exact same bug as I originally reported.
In $browser.url(), the "sameBase" variable ends up as false (almost?) every time when using URLs without the hashbang, so even if you somehow skip the state change logic, the caching variable still gets set. But in hashbang mode, sameBase often ends up as true, so we don't reset the caching variable in those calls and end up in infinite $digest loops in $locationWatch.
My first attempt at fixing this problem was to just remove the sameBase logic, but that made a bunch of tests about changing the URL externally fail and drove me away from that solution quickly.
Works for me. I really only care about my original case anyway. —Reply to this email directly or view it on GitHub. |
I spoke with @IgorMinar last night about this. We feel that we really ought to be able to fix this at the root rather than just catching the start of an infinite digest. I am going to look into it again this week. Sorry @hamfastgamgee. Also we should have an E2E test that checks the fix against real browsers on top of any unit tests. |
I'm totally okay with a real root cause fix. (Like I said at one point, I tried that originally and couldn't riddle it out without breaking a bunch of other tests, which is why I arrived at this "good and working is better than 'perfect' and potentially broken" solution.) We already had to work around the issue by using my patch in our release last weekend, so the immediate urgency for me is lessened. |
I do wonder, though, from what I found when trying to update the mock browser to reflect what the real $browser was actually doing using the reloadLocation variable, whether there are a bunch of other potential infinite digest scenarios there. I still think having a mock browser that's more reality-based would be a good thing. :) |
@hamfastgamgee - I am sorry that this is taking so long and that you had to patch your release. We will get to the bottom of this. |
No worries. :) Let me know if there's anything I can do to help. On Wed, Jun 10, 2015 at 6:40 AM, Pete Bacon Darwin <notifications@github.com
|
I think I have a better solution... The problem appears to occur when we have triggered a reload (via So to be more explicit:
This is why @hamfastgamgee's fix appears to work. When we see that we are in an infinite loop and there is a 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 |
Thanks to @hamfastgamgee for getting this fix in place. Closes #11439 Closes #11675 Closes #11935 Closes #12083
Thanks to @hamfastgamgee for getting this fix in place. Closes angular#11439 Closes angular#11675 Closes angular#11935 Closes angular#12083
fix($browser): prevent infinite $digest from no trailing slash in IE9
This fix prevents IE9 from throwing an infinite $digest error when the user accesses the base
URL of the site without a trailing slash. Suppose you owned http://www.mysite.com/app
and had an Angular app hosted in a subdirectory "app". If an IE9 user accessed
http://www.mysite.com/app infinite $digest errors would be thrown on the console, but the app
itself would eventually resolve properly and work fine. Now the infinite $digest errors will
not be thrown.
Closes #11439