Skip to content

Commit 20f8924

Browse files
crisbetovivian-hu-zz
authored andcommitted
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.
1 parent 6fe84f3 commit 20f8924

File tree

2 files changed

+21
-4
lines changed

2 files changed

+21
-4
lines changed

src/cdk/scrolling/scroll-dispatcher.spec.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,23 @@ describe('ScrollDispatcher', () => {
112112
subscription.unsubscribe();
113113
});
114114

115+
it('should not register the same scrollable twice', () => {
116+
const scrollable = fixture.componentInstance.scrollable;
117+
const scrollSpy = jasmine.createSpy('scroll spy');
118+
const scrollSubscription = scroll.scrolled(0).subscribe(scrollSpy);
119+
120+
expect(scroll.scrollContainers.has(scrollable)).toBe(true);
121+
122+
scroll.register(scrollable);
123+
scroll.deregister(scrollable);
124+
125+
dispatchFakeEvent(fixture.componentInstance.scrollingElement.nativeElement, 'scroll');
126+
fixture.detectChanges();
127+
128+
expect(scrollSpy).not.toHaveBeenCalled();
129+
scrollSubscription.unsubscribe();
130+
});
131+
115132
});
116133

117134
describe('Nested scrollables', () => {

src/cdk/scrolling/scroll-dispatcher.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,10 @@ export class ScrollDispatcher implements OnDestroy {
5252
* @param scrollable Scrollable instance to be registered.
5353
*/
5454
register(scrollable: CdkScrollable): void {
55-
const scrollSubscription = scrollable.elementScrolled()
56-
.subscribe(() => this._scrolled.next(scrollable));
57-
58-
this.scrollContainers.set(scrollable, scrollSubscription);
55+
if (!this.scrollContainers.has(scrollable)) {
56+
this.scrollContainers.set(scrollable, scrollable.elementScrolled()
57+
.subscribe(() => this._scrolled.next(scrollable)));
58+
}
5959
}
6060

6161
/**

0 commit comments

Comments
 (0)