fix(router): ensure duplicate popstate/hashchange events are handled …

…correctly

The current method of handling duplicate navigations caused by 'hashchange' and 'popstate' events for the same url change does not correctly handle cancelled navigations. Because `scheduleNavigation` is called in a `setTimeout` in the location change subscription, the duplicate navigations are not flushed at the same time. This means that if the initial navigation hits a guard that schedules a new navigation, the navigation for the duplicate event will not compare to the correct transition (because we inserted another navigation between the duplicates). See angular#16710 (comment)

Fixes angular#16710

goldens/size-tracking/integration-payloads.json

@@ -39,7 +39,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 2289,
-        "main-es2015": 245488,
+        "main-es2015": 246044,
         "polyfills-es2015": 36938,
         "5-es2015": 751
       }
@@ -49,7 +49,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 2289,
-        "main-es2015": 221268,
+        "main-es2015": 221897,
         "polyfills-es2015": 36938,
         "5-es2015": 779
       }

packages/router/src/router.ts

@@ -6,9 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Location} from '@angular/common';
+import {Location, PopStateEvent} from '@angular/common';
 import {Compiler, Injectable, Injector, isDevMode, NgModuleFactoryLoader, NgModuleRef, NgZone, Type, ɵConsole as Console} from '@angular/core';
-import {BehaviorSubject, EMPTY, Observable, of, Subject, Subscription} from 'rxjs';
+import {BehaviorSubject, EMPTY, Observable, of, Subject, SubscriptionLike} from 'rxjs';
 import {catchError, filter, finalize, map, switchMap, tap} from 'rxjs/operators';
 
 import {QueryParamsHandling, Route, Routes, standardizeConfig, validateConfig} from './config';
@@ -276,6 +276,16 @@ function defaultRouterHook(snapshot: RouterStateSnapshot, runExtras: {
   return of(null) as any;
 }
 
+/**
+ * Information related to a location change, necessary for scheduling follow-up Router navigations.
+ */
+type LocationChangeInfo = {
+  source: 'popstate'|'hashchange',
+  urlTree: UrlTree,
+  state: RestoredState|null,
+  transitionId: number
+};
+
 /**
  * @description
  *
@@ -298,8 +308,12 @@ export class Router {
   private lastSuccessfulNavigation: Navigation|null = null;
   private currentNavigation: Navigation|null = null;
 
-  // TODO(issue/24571): remove '!'.
-  private locationSubscription!: Subscription;
+  private locationSubscription?: SubscriptionLike;
+  /**
+   * Tracks the previously seen location change from the location subscription so we can compare
+   * the two latest to see if they are duplicates. See setUpLocationChangeListener.
+   */
+  private lastLocationChangeInfo: LocationChangeInfo|null = null;
   private navigationId: number = 0;
   private configLoader: RouterConfigLoader;
   private ngModule: NgModuleRef<any>;
@@ -851,26 +865,66 @@ export class Router {
   }
 
   /**
-   * Sets up the location change listener.
+   * Sets up the location change listener. This listener detects navigations triggered from outside
+   * the Router (the browser back/forward buttons, for example) and schedules a corresponding Router
+   * navigation so that the correct events, guards, etc. are triggered.
    */
   setUpLocationChangeListener(): void {
     // Don't need to use Zone.wrap any more, because zone.js
     // already patch onPopState, so location change callback will
     // run into ngZone
     if (!this.locationSubscription) {
-      this.locationSubscription = <any>this.location.subscribe((change: any) => {
-        let rawUrlTree = this.parseUrl(change['url']);
-        const source: NavigationTrigger = change['type'] === 'popstate' ? 'popstate' : 'hashchange';
-        // Navigations coming from Angular router have a navigationId state property. When this
-        // exists, restore the state.
-        const state = change.state && change.state.navigationId ? change.state : null;
-        setTimeout(() => {
-          this.scheduleNavigation(rawUrlTree, source, state, {replaceUrl: true});
-        }, 0);
+      this.locationSubscription = this.location.subscribe(event => {
+        const currentChange = this.extractLocationChangeInfoFromEvent(event);
+        if (this.shouldScheduleNavigation(this.lastLocationChangeInfo, currentChange)) {
+          // The `setTimeout` was added in #12160 and is likely to support Angular/AngularJS
+          // hybrid apps.
+          setTimeout(() => {
+            const {source, state, urlTree} = currentChange;
+            this.scheduleNavigation(urlTree, source, state, {replaceUrl: true});
+          }, 0);
+        }
+        this.lastLocationChangeInfo = currentChange;
       });
     }
   }
 
+  /** Extracts router-related information from a `PopStateEvent`. */
+  private extractLocationChangeInfoFromEvent(change: PopStateEvent): LocationChangeInfo {
+    return {
+      source: change['type'] === 'popstate' ? 'popstate' : 'hashchange',
+      urlTree: this.parseUrl(change['url']!),
+      // Navigations coming from Angular router have a navigationId state
+      // property. When this exists, restore the state.
+      state: change.state?.navigationId ? change.state : null,
+      transitionId: this.getTransition().id
+    } as const;
+  }
+
+  /**
+   * Determines whether two events triggered by the Location subscription are due to the same
+   * navigation. The location subscription can fire two events (popstate and hashchange) for a
+   * single navigation. The second one should be ignored, that is, we should not schedule another
+   * navigation in the Router.
+   */
+  private shouldScheduleNavigation(previous: LocationChangeInfo|null, current: LocationChangeInfo):
+      boolean {
+    if (!previous) return true;
+
+    const sameDestination = current.urlTree.toString() === previous.urlTree.toString();
+    const eventsOccurredAtSameTime = current.transitionId === previous.transitionId;
+    if (!eventsOccurredAtSameTime || !sameDestination) {
+      return true;
+    }
+
+    if ((current.source === 'hashchange' && previous.source === 'popstate') ||
+        (current.source === 'popstate' && previous.source === 'hashchange')) {
+      return false;
+    }
+
+    return true;
+  }
+
   /** The current URL. */
   get url(): string {
     return this.serializeUrl(this.currentUrlTree);
@@ -918,7 +972,7 @@ export class Router {
   dispose(): void {
     if (this.locationSubscription) {
       this.locationSubscription.unsubscribe();
-      this.locationSubscription = null!;
+      this.locationSubscription = undefined;
     }
   }
 
@@ -1135,21 +1189,6 @@ export class Router {
       return Promise.resolve(true);  // return value is not used
     }
 
-    // Because of a bug in IE and Edge, the location class fires two events (popstate and
-    // hashchange) every single time. The second one should be ignored. Otherwise, the URL will
-    // flicker. Handles the case when a popstate was emitted first.
-    if (lastNavigation && source == 'hashchange' && lastNavigation.source === 'popstate' &&
-        lastNavigation.rawUrl.toString() === rawUrl.toString()) {
-      return Promise.resolve(true);  // return value is not used
-    }
-    // Because of a bug in IE and Edge, the location class fires two events (popstate and
-    // hashchange) every single time. The second one should be ignored. Otherwise, the URL will
-    // flicker. Handles the case when a hashchange was emitted first.
-    if (lastNavigation && source == 'popstate' && lastNavigation.source === 'hashchange' &&
-        lastNavigation.rawUrl.toString() === rawUrl.toString()) {
-      return Promise.resolve(true);  // return value is not used
-    }
-
     let resolve: any;
     let reject: any;
     let promise: Promise<boolean>;

packages/router/test/integration.spec.ts

@@ -910,17 +910,29 @@ describe('Integration', () => {
      })));
 
   describe('should reset location if a navigation by location is successful', () => {
+    @Injectable()
+    class RedirectingGuard {
+      constructor(private router: Router) {}
+      canActivate() {
+        this.router.navigate(['/simple']);
+        return false;
+      }
+    }
+
     beforeEach(() => {
       TestBed.configureTestingModule({
-        providers: [{
-          provide: 'in1Second',
-          useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
-            let res: any = null;
-            const p = new Promise(_ => res = _);
-            setTimeout(() => res(true), 1000);
-            return p;
-          }
-        }]
+        providers: [
+          {
+            provide: 'in1Second',
+            useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
+              let res: any = null;
+              const p = new Promise(_ => res = _);
+              setTimeout(() => res(true), 1000);
+              return p;
+            }
+          },
+          RedirectingGuard
+        ]
       });
     });
 
@@ -944,6 +956,28 @@ describe('Integration', () => {
 
          expect(location.path()).toEqual('/simple');
        })));
+
+    it('should skip duplicate location events', fakeAsync(() => {
+         const router = TestBed.inject(Router);
+         const location = TestBed.inject(Location) as unknown as SpyLocation;
+         const fixture = createRoot(router, RootCmp);
+
+         router.resetConfig([
+           {path: 'blocked', component: SimpleCmp, canActivate: [RedirectingGuard]},
+           {path: 'simple', component: SimpleCmp}
+         ]);
+         router.navigateByUrl('/simple');
+         advance(fixture);
+
+         const recordedEvents = [] as Event[];
+         router.events.forEach(e => onlyNavigationStartAndEnd(e) && recordedEvents.push(e));
+
+         location.simulateUrlPop('/blocked');
+         location.simulateHashChange('/blocked');
+
+         advance(fixture);
+         expectEvents(recordedEvents, [[NavigationStart, '/blocked']]);
+       }));
   });
 
   it('should support secondary routes', fakeAsync(inject([Router], (router: Router) => {