From 20f89245f4273c9ed8d3ac0b20e33aac1f42b3de Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 7 Nov 2018 18:10:35 +0000 Subject: [PATCH] fix(scrolling): leaking subscription if same element is registered multiple times (#13974) If the `ScrollDispatcher.register` method is called twice in a row, the consumer will end up with a subscription that can't be cleaned up. These changes add an extra check to prevent the same scrollable from being re-registered. --- src/cdk/scrolling/scroll-dispatcher.spec.ts | 17 +++++++++++++++++ src/cdk/scrolling/scroll-dispatcher.ts | 8 ++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/cdk/scrolling/scroll-dispatcher.spec.ts b/src/cdk/scrolling/scroll-dispatcher.spec.ts index f786c686c12..5ba121a294d 100644 --- a/src/cdk/scrolling/scroll-dispatcher.spec.ts +++ b/src/cdk/scrolling/scroll-dispatcher.spec.ts @@ -112,6 +112,23 @@ describe('ScrollDispatcher', () => { subscription.unsubscribe(); }); + it('should not register the same scrollable twice', () => { + const scrollable = fixture.componentInstance.scrollable; + const scrollSpy = jasmine.createSpy('scroll spy'); + const scrollSubscription = scroll.scrolled(0).subscribe(scrollSpy); + + expect(scroll.scrollContainers.has(scrollable)).toBe(true); + + scroll.register(scrollable); + scroll.deregister(scrollable); + + dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll'); + fixture.detectChanges(); + + expect(scrollSpy).not.toHaveBeenCalled(); + scrollSubscription.unsubscribe(); + }); + }); describe('Nested scrollables', () => { diff --git a/src/cdk/scrolling/scroll-dispatcher.ts b/src/cdk/scrolling/scroll-dispatcher.ts index bf96307d2dd..76cf5545b41 100644 --- a/src/cdk/scrolling/scroll-dispatcher.ts +++ b/src/cdk/scrolling/scroll-dispatcher.ts @@ -52,10 +52,10 @@ export class ScrollDispatcher implements OnDestroy { * @param scrollable Scrollable instance to be registered. */ register(scrollable: CdkScrollable): void { - const scrollSubscription = scrollable.elementScrolled() - .subscribe(() => this._scrolled.next(scrollable)); - - this.scrollContainers.set(scrollable, scrollSubscription); + if (!this.scrollContainers.has(scrollable)) { + this.scrollContainers.set(scrollable, scrollable.elementScrolled() + .subscribe(() => this._scrolled.next(scrollable))); + } } /**