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

packages/router/src/router.ts

@@ -6,10 +6,10 @@
  * 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 {catchError, filter, finalize, map, switchMap, tap} from 'rxjs/operators';
+import {catchError, filter, finalize, map, pairwise, startWith, switchMap, tap} from 'rxjs/operators';
 
 import {QueryParamsHandling, Route, Routes, standardizeConfig, validateConfig} from './config';
 import {createRouterState} from './create_router_state';
@@ -298,8 +298,8 @@ export class Router {
   private lastSuccessfulNavigation: Navigation|null = null;
   private currentNavigation: Navigation|null = null;
 
-  // TODO(issue/24571): remove '!'.
-  private locationSubscription!: Subscription;
+  private readonly _locationSubject = new Subject<PopStateEvent>();
+  private locationSubscription?: Subscription;
   private navigationId: number = 0;
   private configLoader: RouterConfigLoader;
   private ngModule: NgModuleRef<any>;
@@ -858,16 +858,51 @@ export class Router {
     // 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 = new Subscription();
+      this.locationSubscription.add(this.location.subscribe(e => this._locationSubject.next(e)));
+      this.locationSubscription.add(
+          this._locationSubject
+              .pipe(
+                  map(change => ({
+                        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 && change.state.navigationId ? change.state : null,
+                        transitionId: this.getTransition().id
+                      } as const)),
+                  startWith(null),
+                  pairwise(),
+                  )
+              .subscribe(([previous, current]) => {
+                const {source, state, urlTree} = current!;
+
+                function shouldScheduleNavigation(
+                    previous: {source: string, urlTree: UrlTree, transitionId: number}|null,
+                    current: {source: string, urlTree: UrlTree, transitionId: number}): boolean {
+                  const sameDestination =
+                      current.urlTree.toString() !== previous?.urlTree?.toString();
+                  const eventsOccurredAtSameTime = current.transitionId === previous?.transitionId;
+                  if (!previous || !eventsOccurredAtSameTime || !sameDestination) {
+                    return true;
+                  }
+
+                  // The location subscription can fire two events (popstate and hashchange) for a
+                  // single navigation. The second one should be ignored.
+                  if ((current.source === 'hashchange' && previous.source === 'popstate') ||
+                      (current.source === 'popstate' && previous.source === 'hashchange')) {
+                    return false;
+                  }
+
+                  return true;
+                }
+
+                if (shouldScheduleNavigation(previous, current!)) {
+                  setTimeout(() => {
+                    this.scheduleNavigation(urlTree, source, state, {replaceUrl: true});
+                  }, 0);
+                }
+              }));
     }
   }
 
@@ -918,7 +953,7 @@ export class Router {
   dispose(): void {
     if (this.locationSubscription) {
       this.locationSubscription.unsubscribe();
-      this.locationSubscription = null!;
+      this.locationSubscription = undefined;
     }
   }
 
@@ -1135,21 +1170,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) => {