fix(common): cleanup location change listeners when the root view is removed (#40867)

In the new behavior Angular cleanups `popstate` and `hashchange` event listeners
when the root view gets destroyed, thus event handlers are not added twice
when the application is bootstrapped again.

BREAKING CHANGE:

Methods of the `PlatformLocation` class, namely `onPopState` and `onHashChange`,
used to return `void`. Now those methods return functions that can be called
to remove event handlers.

PR Close #31546

PR Close #40867

goldens/public-api/common/common.d.ts

@@ -96,11 +96,12 @@ export declare function getLocaleWeekEndRange(locale: string): [WeekDay, WeekDay
 
 export declare function getNumberOfCurrencyDigits(code: string): number;
 
-export declare class HashLocationStrategy extends LocationStrategy {
+export declare class HashLocationStrategy extends LocationStrategy implements OnDestroy {
     constructor(_platformLocation: PlatformLocation, _baseHref?: string);
     back(): void;
     forward(): void;
     getBaseHref(): string;
+    ngOnDestroy(): void;
     onPopState(fn: LocationChangeListener): void;
     path(includeHash?: boolean): string;
     prepareExternalUrl(internal: string): string;
@@ -324,11 +325,12 @@ export declare enum NumberSymbol {
     CurrencyGroup = 13
 }
 
-export declare class PathLocationStrategy extends LocationStrategy {
+export declare class PathLocationStrategy extends LocationStrategy implements OnDestroy {
     constructor(_platformLocation: PlatformLocation, href?: string);
     back(): void;
     forward(): void;
     getBaseHref(): string;
+    ngOnDestroy(): void;
     onPopState(fn: LocationChangeListener): void;
     path(includeHash?: boolean): string;
     prepareExternalUrl(internal: string): string;
@@ -355,8 +357,8 @@ export declare abstract class PlatformLocation {
     abstract forward(): void;
     abstract getBaseHrefFromDOM(): string;
     abstract getState(): unknown;
-    abstract onHashChange(fn: LocationChangeListener): void;
-    abstract onPopState(fn: LocationChangeListener): void;
+    abstract onHashChange(fn: LocationChangeListener): VoidFunction;
+    abstract onPopState(fn: LocationChangeListener): VoidFunction;
     abstract pushState(state: any, title: string, url: string): void;
     abstract replaceState(state: any, title: string, url: string): void;
 }

goldens/public-api/common/testing/testing.d.ts

@@ -33,8 +33,8 @@ export declare class MockPlatformLocation implements PlatformLocation {
     forward(): void;
     getBaseHrefFromDOM(): string;
     getState(): unknown;
-    onHashChange(fn: LocationChangeListener): void;
-    onPopState(fn: LocationChangeListener): void;
+    onHashChange(fn: LocationChangeListener): VoidFunction;
+    onPopState(fn: LocationChangeListener): VoidFunction;
     pushState(state: any, title: string, newUrl: string): void;
     replaceState(state: any, title: string, newUrl: string): void;
 }

goldens/size-tracking/aio-payloads.json

@@ -26,4 +26,4 @@
       }
     }
   }
-}
+}

packages/common/src/location/hash_location_strategy.ts

@@ -6,7 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Inject, Injectable, Optional} from '@angular/core';
+import {Inject, Injectable, OnDestroy, Optional} from '@angular/core';
+
 import {APP_BASE_HREF, LocationStrategy} from './location_strategy';
 import {LocationChangeListener, PlatformLocation} from './platform_location';
 import {joinWithSlash, normalizeQueryParams} from './util';
@@ -32,8 +33,10 @@ import {joinWithSlash, normalizeQueryParams} from './util';
  * @publicApi
  */
 @Injectable()
-export class HashLocationStrategy extends LocationStrategy {
+export class HashLocationStrategy extends LocationStrategy implements OnDestroy {
   private _baseHref: string = '';
+  private _removeListenerFns: (() => void)[] = [];
+
   constructor(
       private _platformLocation: PlatformLocation,
       @Optional() @Inject(APP_BASE_HREF) _baseHref?: string) {
@@ -43,9 +46,15 @@ export class HashLocationStrategy extends LocationStrategy {
     }
   }
 
+  ngOnDestroy(): void {
+    while (this._removeListenerFns.length) {
+      this._removeListenerFns.pop()!();
+    }
+  }
+
   onPopState(fn: LocationChangeListener): void {
-    this._platformLocation.onPopState(fn);
-    this._platformLocation.onHashChange(fn);
+    this._removeListenerFns.push(
+        this._platformLocation.onPopState(fn), this._platformLocation.onHashChange(fn));
   }
 
   getBaseHref(): string {

packages/common/src/location/location_strategy.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Inject, Injectable, InjectionToken, Optional, ɵɵinject} from '@angular/core';
+import {Inject, Injectable, InjectionToken, OnDestroy, Optional, ɵɵinject} from '@angular/core';
 import {DOCUMENT} from '../dom_tokens';
 import {LocationChangeListener, PlatformLocation} from './platform_location';
 import {joinWithSlash, normalizeQueryParams} from './util';
@@ -105,8 +105,9 @@ export const APP_BASE_HREF = new InjectionToken<string>('appBaseHref');
  * @publicApi
  */
 @Injectable()
-export class PathLocationStrategy extends LocationStrategy {
+export class PathLocationStrategy extends LocationStrategy implements OnDestroy {
   private _baseHref: string;
+  private _removeListenerFns: (() => void)[] = [];
 
   constructor(
       private _platformLocation: PlatformLocation,
@@ -125,9 +126,15 @@ export class PathLocationStrategy extends LocationStrategy {
     this._baseHref = href;
   }
 
+  ngOnDestroy(): void {
+    while (this._removeListenerFns.length) {
+      this._removeListenerFns.pop()!();
+    }
+  }
+
   onPopState(fn: LocationChangeListener): void {
-    this._platformLocation.onPopState(fn);
-    this._platformLocation.onHashChange(fn);
+    this._removeListenerFns.push(
+        this._platformLocation.onPopState(fn), this._platformLocation.onHashChange(fn));
   }
 
   getBaseHref(): string {

packages/common/src/location/platform_location.ts

@@ -40,8 +40,14 @@ import {DOCUMENT} from '../dom_tokens';
 export abstract class PlatformLocation {
   abstract getBaseHrefFromDOM(): string;
   abstract getState(): unknown;
-  abstract onPopState(fn: LocationChangeListener): void;
-  abstract onHashChange(fn: LocationChangeListener): void;
+  /**
+   * Returns a function that, when executed, removes the `popstate` event handler.
+   */
+  abstract onPopState(fn: LocationChangeListener): VoidFunction;
+  /**
+   * Returns a function that, when executed, removes the `hashchange` event handler.
+   */
+  abstract onHashChange(fn: LocationChangeListener): VoidFunction;
 
   abstract get href(): string;
   abstract get protocol(): string;
@@ -122,12 +128,16 @@ export class BrowserPlatformLocation extends PlatformLocation {
     return getDOM().getBaseHref(this._doc)!;
   }
 
-  onPopState(fn: LocationChangeListener): void {
-    getDOM().getGlobalEventTarget(this._doc, 'window').addEventListener('popstate', fn, false);
+  onPopState(fn: LocationChangeListener): VoidFunction {
+    const window = getDOM().getGlobalEventTarget(this._doc, 'window');
+    window.addEventListener('popstate', fn, false);
+    return () => window.removeEventListener('popstate', fn);
   }
 
-  onHashChange(fn: LocationChangeListener): void {
-    getDOM().getGlobalEventTarget(this._doc, 'window').addEventListener('hashchange', fn, false);
+  onHashChange(fn: LocationChangeListener): VoidFunction {
+    const window = getDOM().getGlobalEventTarget(this._doc, 'window');
+    window.addEventListener('hashchange', fn, false);
+    return () => window.removeEventListener('hashchange', fn);
   }
 
   get href(): string {

packages/common/testing/src/mock_platform_location.ts

@@ -153,13 +153,15 @@ export class MockPlatformLocation implements PlatformLocation {
     return this.baseHref;
   }
 
-  onPopState(fn: LocationChangeListener): void {
+  onPopState(fn: LocationChangeListener): VoidFunction {
     // No-op: a state stack is not implemented, so
     // no events will ever come.
+    return () => {};
   }
 
-  onHashChange(fn: LocationChangeListener): void {
-    this.hashUpdate.subscribe(fn);
+  onHashChange(fn: LocationChangeListener): VoidFunction {
+    const subscription = this.hashUpdate.subscribe(fn);
+    return () => subscription.unsubscribe();
   }
 
   get href(): string {

packages/platform-server/src/location.ts

@@ -70,13 +70,15 @@ export class ServerPlatformLocation implements PlatformLocation {
     return getDOM().getBaseHref(this._doc)!;
   }
 
-  onPopState(fn: LocationChangeListener): void {
+  onPopState(fn: LocationChangeListener): VoidFunction {
     // No-op: a state stack is not implemented, so
     // no events will ever come.
+    return () => {};
   }
 
-  onHashChange(fn: LocationChangeListener): void {
-    this._hashUpdate.subscribe(fn);
+  onHashChange(fn: LocationChangeListener): VoidFunction {
+    const subscription = this._hashUpdate.subscribe(fn);
+    return () => subscription.unsubscribe();
   }
 
   get url(): string {

packages/router/test/bootstrap.spec.ts

@@ -6,13 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {APP_BASE_HREF, DOCUMENT, Location, ɵgetDOM as getDOM} from '@angular/common';
+import {APP_BASE_HREF, DOCUMENT, ɵgetDOM as getDOM} from '@angular/common';
 import {ApplicationRef, Component, CUSTOM_ELEMENTS_SCHEMA, destroyPlatform, NgModule} from '@angular/core';
 import {inject} from '@angular/core/testing';
 import {BrowserModule} from '@angular/platform-browser';
 import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
 import {NavigationEnd, Resolve, Router, RouterModule} from '@angular/router';
-import {filter, first} from 'rxjs/operators';
 
 describe('bootstrap', () => {
   if (isNode) return;
@@ -369,7 +368,30 @@ describe('bootstrap', () => {
     done();
   });
 
-  function waitForNavigationToComplete(router: Router): Promise<any> {
-    return router.events.pipe(filter((e: any) => e instanceof NavigationEnd), first()).toPromise();
-  }
+  it('should cleanup "popstate" and "hashchange" listeners', async () => {
+    @NgModule({
+      imports: [BrowserModule, RouterModule.forRoot([])],
+      declarations: [RootCmp],
+      bootstrap: [RootCmp],
+      providers: testProviders,
+    })
+    class TestModule {
+    }
+
+    spyOn(window, 'addEventListener').and.callThrough();
+    spyOn(window, 'removeEventListener').and.callThrough();
+
+    const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
+    ngModuleRef.destroy();
+
+    expect(window.addEventListener).toHaveBeenCalledTimes(2);
+
+    expect(window.addEventListener)
+        .toHaveBeenCalledWith('popstate', jasmine.any(Function), jasmine.any(Boolean));
+    expect(window.addEventListener)
+        .toHaveBeenCalledWith('hashchange', jasmine.any(Function), jasmine.any(Boolean));
+
+    expect(window.removeEventListener).toHaveBeenCalledWith('popstate', jasmine.any(Function));
+    expect(window.removeEventListener).toHaveBeenCalledWith('hashchange', jasmine.any(Function));
+  });
 });