Skip to content

Commit

Permalink
fix(google-maps): avoid event listener leaks if inputs change (#17664)
Browse files Browse the repository at this point in the history
Currently we initialize the map-related event listeners inside a subscription in the various map components, however the only time we remove them is on destroy. This can lead to leaks if the subcription fires multiple times. Note that at the moment we have a guard in place in the form of `take(1)`, but we could easily end up with event leaks if it were to be moved/removed if we decided to make it to react to changes post initialization.
  • Loading branch information
crisbeto authored and jelbourn committed Nov 20, 2019
1 parent 022acf8 commit b2ea4c8
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 9 deletions.
18 changes: 14 additions & 4 deletions src/google-maps/google-map/google-map.ts
Expand Up @@ -196,7 +196,7 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {

private _googleMapChanges!: Observable<google.maps.Map>;

private readonly _listeners: google.maps.MapsEventListener[] = [];
private _listeners: google.maps.MapsEventListener[] = [];

private readonly _options = new BehaviorSubject<google.maps.MapOptions>(DEFAULT_OPTIONS);
private readonly _center =
Expand Down Expand Up @@ -246,9 +246,7 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
ngOnDestroy() {
this._destroy.next();
this._destroy.complete();
for (let listener of this._listeners) {
listener.remove();
}
this._clearListeners();
}

/**
Expand Down Expand Up @@ -449,6 +447,9 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
}

private _initializeEventHandlers() {
// Ensure that we don't leak if called multiple times.
this._clearListeners();

const eventHandlers = new Map<string, EventEmitter<void>>([
['bounds_changed', this.boundsChanged],
['center_changed', this.centerChanged],
Expand Down Expand Up @@ -493,4 +494,13 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
}));
}
}

/** Clears all currently-registered event listeners. */
private _clearListeners() {
for (let listener of this._listeners) {
listener.remove();
}

this._listeners = [];
}
}
14 changes: 13 additions & 1 deletion src/google-maps/map-info-window/map-info-window.ts
Expand Up @@ -80,7 +80,7 @@ export class MapInfoWindow implements OnInit, OnDestroy {
private readonly _position =
new BehaviorSubject<google.maps.LatLngLiteral|google.maps.LatLng|undefined>(undefined);

private readonly _listeners: google.maps.MapsEventListener[] = [];
private _listeners: google.maps.MapsEventListener[] = [];

private readonly _destroy = new Subject<void>();

Expand Down Expand Up @@ -167,6 +167,9 @@ export class MapInfoWindow implements OnInit, OnDestroy {
}

private _initializeEventHandlers() {
// Ensure that we don't leak if called multiple times.
this._clearListeners();

const eventHandlers = new Map<string, EventEmitter<void>>([
['closeclick', this.closeclick],
['content_changed', this.contentChanged],
Expand All @@ -182,4 +185,13 @@ export class MapInfoWindow implements OnInit, OnDestroy {
}
});
}

/** Clears all currently-registered event listeners. */
private _clearListeners() {
for (let listener of this._listeners) {
listener.remove();
}

this._listeners = [];
}
}
18 changes: 14 additions & 4 deletions src/google-maps/map-marker/map-marker.ts
Expand Up @@ -205,7 +205,7 @@ export class MapMarker implements OnInit, OnDestroy {

private readonly _destroy = new Subject<void>();

private readonly _listeners: google.maps.MapsEventListener[] = [];
private _listeners: google.maps.MapsEventListener[] = [];

_marker?: google.maps.Marker;

Expand All @@ -230,9 +230,7 @@ export class MapMarker implements OnInit, OnDestroy {
ngOnDestroy() {
this._destroy.next();
this._destroy.complete();
for (let listener of this._listeners) {
listener.remove();
}
this._clearListeners();
if (this._marker) {
this._marker.setMap(null);
}
Expand Down Expand Up @@ -390,6 +388,9 @@ export class MapMarker implements OnInit, OnDestroy {
}

private _initializeEventHandlers() {
// Ensure that we don't leak if called multiple times.
this._clearListeners();

const eventHandlers = new Map<string, EventEmitter<void>>([
['animation_changed', this.animationChanged],
['clickable_changed', this.clickableChanged],
Expand Down Expand Up @@ -433,4 +434,13 @@ export class MapMarker implements OnInit, OnDestroy {
}
});
}

/** Clears all currently-registered event listeners. */
private _clearListeners() {
for (let listener of this._listeners) {
listener.remove();
}

this._listeners = [];
}
}

0 comments on commit b2ea4c8

Please sign in to comment.