fix($browser): prevent infinite $digest from no trailing slash in IE9

[hamfastgamgee]

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

[hamfastgamgee]

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. :)

[Narretz]

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?

[hamfastgamgee]

I'm moderately certain that the codepath hasn't changed significantly, but
I can certainly give it a try.

On Tue, Apr 28, 2015 at 3:56 PM, Martin Staffa notifications@github.com
wrote:

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?

—
Reply to this email directly or view it on GitHub
#11675 (comment).

[hamfastgamgee]

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.

[hamfastgamgee]

And with merged identical changes made in 1.4.x, the new tests all pass.

[hamfastgamgee]

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... :)

[petebacondarwin]

I am working on it @hamfastgamgee - sorry about the delay. Give me a few days.

[hamfastgamgee]

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. :) )

[hamfastgamgee]

Anything more I can help with here? Looking at some other checkins, I'm wondering if the $browser helper function I added should be $$ prefixed and get a no-op entry in angular-mocks, but I don't really want to do that until I get feedback about the overall approach (which is basically creating an end condition to break out of an infinite loop).

[petebacondarwin]

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.

[hamfastgamgee]

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. :) )

[petebacondarwin]

Thursday morning (GMT). It's in the diary

[petebacondarwin]

Looking at this now.
My first comment is that this bug is not really related to ngRoute, so the tests should be inside $locationSpec.js and not require ngRoute to display the problem.

[hamfastgamgee]

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?

[petebacondarwin]

OK, I really want to get a reproduction (in unit test) of this without ngRoute. I guess it will involve setting up a $locationChangedSucess event that changes the path right?

[hamfastgamgee]

While I'm working on that: Do you want the new $browser method to use the $$ prefix?

[hamfastgamgee]

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.

[petebacondarwin]

@hamfastgamgee - thanks for all this work. I am going to take another look today.

[hamfastgamgee]

And thank you for taking a look at it. And of course, "correct" always
beats "fast".

On Fri, May 22, 2015 at 6:25 AM, Pete Bacon Darwin <notifications@github.com

wrote:

@hamfastgamgee https://github.com/hamfastgamgee - thanks for all this
work. I am going to take another look today.

—
Reply to this email directly or view it on GitHub
#11675 (comment).

[hamfastgamgee]

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".)

[petebacondarwin]

@hamfastgamgee - I have provided a unit test in here #11935. Please take a look

[hamfastgamgee]

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?

[hamfastgamgee]

Or it'll bomb on IE9 for some reason. Ugh...

[hamfastgamgee]

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 $browser does (it would pass what I call "currUrl"), but if I don't pass self.$$url there, 3-5 tests bomb that I'm pretty sure would not actually bomb in real world scenarios. In any case, I think it's ready for re-review (and possibly more fixes from @petebacondarwin). :)

[petebacondarwin]

@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.

[petebacondarwin]

But, if we include the version from my PR as-is then this does indeed fail with infinite-digest error.
I think for the time-being I would be happy with this fix and the tests from your PR and mine to cover ourselves. I would like to dig further into a cleaner way for $browser and $location to ensure that they don't get trapped like this.

[petebacondarwin]

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...

[hamfastgamgee]

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.

[hamfastgamgee]

Works for me. I really only care about my original case anyway.
Sent using Boxer
On May 24, 2015 3:10 PM, Pete Bacon Darwin notifications@github.com wrote:But, if we include the version from my PR as-is then this does indeed fail with infinite-digest error.
I think for the time-being I would be happy with this fix and the tests from your PR and mine to cover ourselves. I would like to dig further into a cleaner way for $browser and $location to ensure that they don't get trapped like this.

—Reply to this email directly or view it on GitHub.

[petebacondarwin]

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.

[hamfastgamgee]

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.

[hamfastgamgee]

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. :)

[petebacondarwin]

@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.

[hamfastgamgee]

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

wrote:

@hamfastgamgee https://github.com/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.

—
Reply to this email directly or view it on GitHub
#11675 (comment).

[petebacondarwin]

I think I have a better solution...

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 cancelling its 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.