fix($browser): account for IE deserializing history.state on each read

IE 10-11+ deserialize history.state on every read, causing simple comparisons
against history.state always return false. Account for that caching
`history.state` on every hashchange or popstate event.

Also:
1. Prevent firing onUrlChange callbacks twice if both popstate and hashchange
event were fired.
2. Fix the issue of routes sometimes not firing the URL change in all browsers.

Closes #9587
Fixes #9545

src/ng/browser.js

@@ -123,11 +123,14 @@ function Browser(window, document, $log, $sniffer) {
   // URL API
   //////////////////////////////////////////////////////////////
 
-  var lastBrowserUrl = location.href,
-      lastHistoryState = history.state,
+  var cachedState, lastHistoryState,
+      lastBrowserUrl = location.href,
       baseElement = document.find('base'),
       reloadLocation = null;
 
+  cacheState();
+  lastHistoryState = cachedState;
+
   /**
    * @name $browser#url
    *
@@ -165,18 +168,20 @@ function Browser(window, document, $log, $sniffer) {
       // Don't change anything if previous and current URLs and states match. This also prevents
       // IE<10 from getting into redirect loop when in LocationHashbangInHtml5Url mode.
       // See https://github.com/angular/angular.js/commit/ffb2701
-      if (lastBrowserUrl === url && (!$sniffer.history || history.state === state)) {
+      if (lastBrowserUrl === url && (!$sniffer.history || cachedState === state)) {
         return;
       }
       var sameBase = lastBrowserUrl && stripHash(lastBrowserUrl) === stripHash(url);
       lastBrowserUrl = url;
+      lastHistoryState = state;
       // Don't use history API if only the hash changed
       // due to a bug in IE10/IE11 which leads
       // to not firing a `hashchange` nor `popstate` event
       // in some cases (see #9143).
-      if ($sniffer.history && (!sameBase || history.state !== state)) {
+      if ($sniffer.history && (!sameBase || cachedState !== state)) {
         history[replace ? 'replaceState' : 'pushState'](state, '', url);
-        lastHistoryState = history.state;
+        cacheState();
+        lastHistoryState = cachedState;
       } else {
         if (!sameBase) {
           reloadLocation = url;
@@ -208,20 +213,40 @@ function Browser(window, document, $log, $sniffer) {
    * @returns {object} state
    */
   self.state = function() {
-    return isUndefined(history.state) ? null : history.state;
+    return cachedState;
   };
 
   var urlChangeListeners = [],
       urlChangeInit = false;
 
+  function cacheStateAndFireUrlChange() {
+    cacheState();
+    fireUrlChange();
+  }
+
+  // This variable should be used *only* inside the cacheState function.
+  var lastCachedState = null;
+  function cacheState() {
+    // This should be the only place in $browser where `history.state` is read.
+    cachedState = window.history.state;
+    cachedState = isUndefined(cachedState) ? null : cachedState;
+
+    // Prevent callbacks fo fire twice if both hashchange & popstate were fired.
+    if (equals(cachedState, lastCachedState)) {
+      cachedState = lastCachedState;
+    }
+    lastCachedState = cachedState;
+  }
+
   function fireUrlChange() {
-    if (lastBrowserUrl === self.url() && lastHistoryState === history.state) {
+    if (lastBrowserUrl === self.url() && lastHistoryState === cachedState) {
       return;
     }
 
     lastBrowserUrl = self.url();
+    lastHistoryState = cachedState;
     forEach(urlChangeListeners, function(listener) {
-      listener(self.url(), history.state);
+      listener(self.url(), cachedState);
     });
   }
 
@@ -254,9 +279,9 @@ function Browser(window, document, $log, $sniffer) {
       // changed by push/replaceState
 
       // html5 history api - popstate event
-      if ($sniffer.history) jqLite(window).on('popstate', fireUrlChange);
+      if ($sniffer.history) jqLite(window).on('popstate', cacheStateAndFireUrlChange);
       // hashchange event
-      jqLite(window).on('hashchange', fireUrlChange);
+      jqLite(window).on('hashchange', cacheStateAndFireUrlChange);
 
       urlChangeInit = true;
     }

src/ng/location.js

@@ -850,9 +850,10 @@ function $LocationProvider(){
       var oldUrl = $browser.url();
       var oldState = $browser.state();
       var currentReplace = $location.$$replace;
+      var urlOrStateChanged = oldUrl !== $location.absUrl() ||
+        ($location.$$html5 && $sniffer.history && oldState !== $location.$$state);
 
-      if (initializing || oldUrl !== $location.absUrl() ||
-          ($location.$$html5 && $sniffer.history && oldState !== $location.$$state)) {
+      if (initializing || urlOrStateChanged) {
         initializing = false;
 
         $rootScope.$evalAsync(function() {
@@ -861,8 +862,10 @@ function $LocationProvider(){
             $location.$$parse(oldUrl);
             $location.$$state = oldState;
           } else {
-            setBrowserUrlWithFallback($location.absUrl(), currentReplace,
-                                      oldState === $location.$$state ? null : $location.$$state);
+            if (urlOrStateChanged) {
+              setBrowserUrlWithFallback($location.absUrl(), currentReplace,
+                                        oldState === $location.$$state ? null : $location.$$state);
+            }
             afterLocationChange(oldUrl, oldState);
           }
         });

test/ng/browserSpecs.js

@@ -3,11 +3,16 @@
 var historyEntriesLength;
 var sniffer = {};
 
-function MockWindow() {
+function MockWindow(options) {
+  if (typeof options !== 'object') {
+    options = {};
+  }
   var events = {};
   var timeouts = this.timeouts = [];
   var locationHref = 'http://server/';
   var mockWindow = this;
+  var msie = options.msie;
+  var ieState;
 
   historyEntriesLength = 1;
 
@@ -53,16 +58,32 @@ function MockWindow() {
   };
 
   this.history = {
-    state: null,
-    pushState: function() {
+    pushState: function () {
       this.replaceState.apply(this, arguments);
       historyEntriesLength++;
     },
-    replaceState: function(state, title, url) {
+    replaceState: function (state, title, url) {
       locationHref = url;
       mockWindow.history.state = copy(state);
     }
   };
+  // IE 10-11 deserialize history.state on each read making subsequent reads
+  // different object.
+  if (!msie) {
+    this.history.state = null;
+  } else {
+    ieState = null;
+    Object.defineProperty(this.history, 'state', {
+      get: function() {
+        return copy(ieState);
+      },
+      set: function(value) {
+        ieState = value;
+      },
+      configurable: true,
+      enumerable: true,
+    });
+  }
 }
 
 function MockDocument() {
@@ -90,7 +111,7 @@ function MockDocument() {
 
 describe('browser', function() {
   /* global Browser: false */
-  var browser, fakeWindow, fakeDocument, logs, scripts, removedScripts;
+  var browser, fakeWindow, fakeDocument, fakeLog, logs, scripts, removedScripts;
 
   beforeEach(function() {
     scripts = [];
@@ -109,30 +130,55 @@ describe('browser', function() {
     browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);
   });
 
-  describe('MockBrowser historyEntriesLength', function() {
-    it('should increment historyEntriesLength when setting location.href', function() {
-      expect(historyEntriesLength).toBe(1);
-      fakeWindow.location.href = '/foo';
-      expect(historyEntriesLength).toBe(2);
+  describe('MockBrowser', function() {
+    describe('historyEntriesLength', function() {
+      it('should increment historyEntriesLength when setting location.href', function() {
+        expect(historyEntriesLength).toBe(1);
+        fakeWindow.location.href = '/foo';
+        expect(historyEntriesLength).toBe(2);
+      });
+
+      it('should not increment historyEntriesLength when using location.replace', function() {
+        expect(historyEntriesLength).toBe(1);
+        fakeWindow.location.replace('/foo');
+        expect(historyEntriesLength).toBe(1);
+      });
+
+      it('should increment historyEntriesLength when using history.pushState', function() {
+        expect(historyEntriesLength).toBe(1);
+        fakeWindow.history.pushState({a: 2}, 'foo', '/bar');
+        expect(historyEntriesLength).toBe(2);
+      });
+
+      it('should not increment historyEntriesLength when using history.replaceState', function() {
+        expect(historyEntriesLength).toBe(1);
+        fakeWindow.history.replaceState({a: 2}, 'foo', '/bar');
+        expect(historyEntriesLength).toBe(1);
+      });
     });
 
-    it('should not increment historyEntriesLength when using location.replace', function() {
-      expect(historyEntriesLength).toBe(1);
-      fakeWindow.location.replace('/foo');
-      expect(historyEntriesLength).toBe(1);
-    });
+    describe('in IE', runTests({msie: true}));
+    describe('not in IE', runTests({msie: false}));
 
-    it('should increment historyEntriesLength when using history.pushState', function() {
-      expect(historyEntriesLength).toBe(1);
-      fakeWindow.history.pushState({a: 2}, 'foo', '/bar');
-      expect(historyEntriesLength).toBe(2);
-    });
+    function runTests(options) {
+      return function () {
+        it('should return the same state object on every read', function () {
+          var msie = options.msie;
 
-    it('should not increment historyEntriesLength when using history.replaceState', function() {
-      expect(historyEntriesLength).toBe(1);
-      fakeWindow.history.replaceState({a: 2}, 'foo', '/bar');
-      expect(historyEntriesLength).toBe(1);
-    });
+          fakeWindow = new MockWindow({msie: msie});
+          fakeWindow.location.state = {prop: 'val'};
+          browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);
+
+          browser.url(fakeWindow.location.href, false, {prop: 'val'});
+          if (msie) {
+            expect(fakeWindow.history.state).not.toBe(fakeWindow.history.state);
+            expect(fakeWindow.history.state).toEqual(fakeWindow.history.state);
+          } else {
+            expect(fakeWindow.history.state).toBe(fakeWindow.history.state);
+          }
+        });
+      };
+    }
   });
 
   it('should contain cookie cruncher', function() {
@@ -602,58 +648,118 @@ describe('browser', function() {
       currentHref = fakeWindow.location.href;
     });
 
-    it('should change state', function() {
-      browser.url(currentHref + '/something', false, {prop: 'val1'});
-      expect(fakeWindow.history.state).toEqual({prop: 'val1'});
+    describe('in IE', runTests({msie: true}));
+    describe('not in IE', runTests({msie: false}));
+
+    function runTests(options) {
+      return function() {
+        beforeEach(function() {
+          fakeWindow = new MockWindow({msie: options.msie});
+          browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);
+          browser.onUrlChange(function() {});
+        });
+
+        it('should change state', function() {
+          browser.url(currentHref, false, {prop: 'val1'});
+          expect(fakeWindow.history.state).toEqual({prop: 'val1'});
+          browser.url(currentHref + '/something', false, {prop: 'val2'});
+          expect(fakeWindow.history.state).toEqual({prop: 'val2'});
+        });
+
+        it('should allow to set falsy states (except `undefined`)', function() {
+          fakeWindow.history.state = {prop: 'val1'};
+          fakeWindow.fire('popstate');
+
+          browser.url(currentHref, false, null);
+          expect(fakeWindow.history.state).toBe(null);
+
+          browser.url(currentHref, false, false);
+          expect(fakeWindow.history.state).toBe(false);
+
+          browser.url(currentHref, false, '');
+          expect(fakeWindow.history.state).toBe('');
+
+          browser.url(currentHref, false, 0);
+          expect(fakeWindow.history.state).toBe(0);
+        });
+
+        it('should treat `undefined` state as `null`', function() {
+          fakeWindow.history.state = {prop: 'val1'};
+          fakeWindow.fire('popstate');
+
+          browser.url(currentHref, false, undefined);
+          expect(fakeWindow.history.state).toBe(null);
+        });
+
+        it('should do pushState with the same URL and a different state', function() {
+          browser.url(currentHref, false, {prop: 'val1'});
+          expect(fakeWindow.history.state).toEqual({prop: 'val1'});
+
+          browser.url(currentHref, false, null);
+          expect(fakeWindow.history.state).toBe(null);
+
+          browser.url(currentHref, false, {prop: 'val2'});
+          browser.url(currentHref, false, {prop: 'val3'});
+          expect(fakeWindow.history.state).toEqual({prop: 'val3'});
+        });
+
+        it('should do pushState with the same URL and null state', function() {
+          fakeWindow.history.state = {prop: 'val1'};
+          fakeWindow.fire('popstate');
+
+          browser.url(currentHref, false, null);
+          expect(fakeWindow.history.state).toEqual(null);
+        });
+
+        it('should do pushState with the same URL and the same non-null state', function() {
+          fakeWindow.history.state = null;
+          fakeWindow.fire('popstate');
+
+          browser.url(currentHref, false, {prop: 'val2'});
+          expect(fakeWindow.history.state).toEqual({prop: 'val2'});
+        });
+      };
+    }
+  });
+
+  describe('state', function() {
+    var currentHref;
+
+    beforeEach(function() {
+      sniffer = {history: true};
+      currentHref = fakeWindow.location.href;
     });
 
-    it('should allow to set falsy states (except `undefined`)', function() {
-      fakeWindow.history.state = {prop: 'val1'};
+    describe('in IE', runTests({msie: true}));
+    describe('not in IE', runTests({msie: false}));
 
-      browser.url(currentHref, false, null);
-      expect(fakeWindow.history.state).toBe(null);
+    function runTests(options) {
+      return function() {
+        beforeEach(function() {
+          fakeWindow = new MockWindow({msie: options.msie});
+          browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);
+        });
 
-      browser.url(currentHref, false, false);
-      expect(fakeWindow.history.state).toBe(false);
+        it('should return history.state', function() {
+          browser.url(currentHref, false, {prop: 'val'});
+          expect(browser.state()).toEqual({prop: 'val'});
+          browser.url(currentHref, false, 2);
+          expect(browser.state()).toEqual(2);
+          browser.url(currentHref, false, null);
+          expect(browser.state()).toEqual(null);
+        });
 
-      browser.url(currentHref, false, '');
-      expect(fakeWindow.history.state).toBe('');
+        it('should return null if history.state is undefined', function() {
+          browser.url(currentHref, false, undefined);
+          expect(browser.state()).toBe(null);
+        });
 
-      browser.url(currentHref, false, 0);
-      expect(fakeWindow.history.state).toBe(0);
-    });
-
-    it('should treat `undefined` state as `null`', function() {
-      fakeWindow.history.state = {prop: 'val1'};
-
-      browser.url(currentHref, false, undefined);
-      expect(fakeWindow.history.state).toBe(null);
-    });
-
-    it('should do pushState with the same URL and a different state', function() {
-      browser.url(currentHref, false, {prop: 'val1'});
-      expect(fakeWindow.history.state).toEqual({prop: 'val1'});
-
-      browser.url(currentHref, false, null);
-      expect(fakeWindow.history.state).toBe(null);
-
-      browser.url(currentHref, false, {prop: 'val2'});
-      browser.url(currentHref, false, {prop: 'val3'});
-      expect(fakeWindow.history.state).toEqual({prop: 'val3'});
-    });
-
-    it('should do pushState with the same URL and null state', function() {
-      fakeWindow.history.state = {prop: 'val1'};
-      browser.url(currentHref, false, null);
-      expect(fakeWindow.history.state).toEqual(null);
-    });
-
-    it('should do pushState with the same URL and the same non-null state', function() {
-      browser.url(currentHref, false, {prop: 'val2'});
-      fakeWindow.history.state = {prop: 'val3'};
-      browser.url(currentHref, false, {prop: 'val2'});
-      expect(fakeWindow.history.state).toEqual({prop: 'val2'});
-    });
+        it('should return the same state object in subsequent invocations in IE', function() {
+          browser.url(currentHref, false, {prop: 'val'});
+          expect(browser.state()).toBe(browser.state());
+        });
+      };
+    }
   });
 
   describe('urlChange', function() {
@@ -718,6 +824,36 @@ describe('browser', function() {
       fakeWindow.fire('hashchange');
       expect(callback).not.toHaveBeenCalled();
     });
+
+    describe('state handling', function() {
+      var currentHref;
+
+      beforeEach(function() {
+        sniffer = {history: true};
+        currentHref = fakeWindow.location.href;
+      });
+
+      describe('in IE', runTests({msie: true}));
+      describe('not in IE', runTests({msie: false}));
+
+      function runTests(options) {
+        return function() {
+          beforeEach(function() {
+            fakeWindow = new MockWindow({msie: options.msie});
+            browser = new Browser(fakeWindow, fakeDocument, fakeLog, sniffer);
+          });
+
+          it('should fire onUrlChange listeners only once if both popstate and hashchange triggered', function() {
+            fakeWindow.history.state = {prop: 'val'};
+            browser.onUrlChange(callback);
+
+            fakeWindow.fire('hashchange');
+            fakeWindow.fire('popstate');
+            expect(callback).toHaveBeenCalledOnce();
+          });
+        };
+      }
+    });
   });
 
 
@@ -876,9 +1012,7 @@ describe('browser', function() {
 
         // from $location for rewriting the initial url into a hash url
         expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', true);
-        // from the initial call to the watch in $location for watching $location
-        expect(browser.url).toHaveBeenCalledWith('http://server/#/some/deep/path', false, null);
-        expect(changeUrlCount).toBe(2);
+        expect(changeUrlCount).toBe(1);
       });
 
     });