From 191333a139f2728e788948767205d7dcc912143b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Neto=C4=8Dn=C3=BD?= Date: Fri, 16 Jun 2017 13:50:13 +0200 Subject: [PATCH 1/5] Fixed memory leaking in UniqueSelectionDispatcher --- src/lib/button-toggle/button-toggle.ts | 15 +++++++++++++-- .../coordination/unique-selection-dispatcher.ts | 12 ++++++++++-- src/lib/expansion/accordion-item.ts | 6 +++++- src/lib/radio/radio.ts | 6 +++++- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/lib/button-toggle/button-toggle.ts b/src/lib/button-toggle/button-toggle.ts index 4475dec407dc..fe6dd406056d 100644 --- a/src/lib/button-toggle/button-toggle.ts +++ b/src/lib/button-toggle/button-toggle.ts @@ -16,6 +16,7 @@ import { HostBinding, Input, OnInit, + OnDestroy, Optional, Output, QueryList, @@ -271,7 +272,7 @@ export class MdButtonToggleGroupMultiple extends _MdButtonToggleGroupMixinBase 'class': 'mat-button-toggle' } }) -export class MdButtonToggle implements OnInit { +export class MdButtonToggle implements OnInit, OnDestroy { /** Whether or not this button toggle is checked. */ private _checked: boolean = false; @@ -287,6 +288,9 @@ export class MdButtonToggle implements OnInit { /** Whether or not the button toggle is a single selection. */ private _isSingleSelector: boolean = null; + /** Unregister function for _buttonToggleDispatcherListener **/ + private _buttonToggleDispatcherListener: Function; + @ViewChild('input') _inputElement: ElementRef; /** The parent button toggle group (exclusive selection). Optional. */ @@ -372,7 +376,7 @@ export class MdButtonToggle implements OnInit { this.buttonToggleGroupMultiple = toggleGroupMultiple; if (this.buttonToggleGroup) { - _buttonToggleDispatcher.listen((id: string, name: string) => { + this._buttonToggleDispatcherListener = _buttonToggleDispatcher.listen((id: string, name: string) => { if (id != this.id && name == this.name) { this.checked = false; } @@ -446,4 +450,11 @@ export class MdButtonToggle implements OnInit { event.value = this._value; this.change.emit(event); } + + // Unregister buttonToggleDispatcherListener on destroy + ngOnDestroy(): void { + if(this._buttonToggleDispatcherListener) { + this._buttonToggleDispatcherListener(); + } + } } diff --git a/src/lib/core/coordination/unique-selection-dispatcher.ts b/src/lib/core/coordination/unique-selection-dispatcher.ts index 46eb5d0b8189..bfe2eee37d5b 100644 --- a/src/lib/core/coordination/unique-selection-dispatcher.ts +++ b/src/lib/core/coordination/unique-selection-dispatcher.ts @@ -36,9 +36,17 @@ export class UniqueSelectionDispatcher { } } - /** Listen for future changes to item selection. */ - listen(listener: UniqueSelectionDispatcherListener) { + /** + * Listen for future changes to item selection. + * @return Function used to unregister listener + **/ + listen(listener: UniqueSelectionDispatcherListener): Function { this._listeners.push(listener); + return () => { + this._listeners = this._listeners.filter((registered: UniqueSelectionDispatcherListener) => { + return listener !== registered; + }); + } } } diff --git a/src/lib/expansion/accordion-item.ts b/src/lib/expansion/accordion-item.ts index 08cbfb4467da..a84d64ac3f3e 100644 --- a/src/lib/expansion/accordion-item.ts +++ b/src/lib/expansion/accordion-item.ts @@ -48,9 +48,12 @@ export class AccordionItem implements OnDestroy { } private _expanded: boolean; + /** Unregister function for _expansionDispatcherListener **/ + private _expansionDispatcherListener: Function; + constructor(@Optional() public accordion: CdkAccordion, protected _expansionDispatcher: UniqueSelectionDispatcher) { - _expansionDispatcher.listen((id: string, accordionId: string) => { + this._expansionDispatcherListener = _expansionDispatcher.listen((id: string, accordionId: string) => { if (this.accordion && !this.accordion.multi && this.accordion.id === accordionId && this.id !== id) { this.expanded = false; @@ -61,6 +64,7 @@ export class AccordionItem implements OnDestroy { /** Emits an event for the accordion item being destroyed. */ ngOnDestroy() { this.destroyed.emit(); + this._expansionDispatcherListener(); } /** Toggles the expanded state of the accordion item. */ diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts index 2d141f3fd7c4..ddb90c8804fa 100644 --- a/src/lib/radio/radio.ts +++ b/src/lib/radio/radio.ts @@ -458,6 +458,9 @@ export class MdRadioButton extends _MdRadioButtonMixinBase /** Reference to the current focus ripple. */ private _focusRipple: RippleRef; + /** Unregister function for _radioDispatcher **/ + private _radioDispatcherListener: Function; + /** The native `` element */ @ViewChild('input') _inputElement: ElementRef; @@ -473,7 +476,7 @@ export class MdRadioButton extends _MdRadioButtonMixinBase // TODO(jelbourn): Assert that there's no name binding AND a parent radio group. this.radioGroup = radioGroup; - _radioDispatcher.listen((id: string, name: string) => { + this._radioDispatcherListener = _radioDispatcher.listen((id: string, name: string) => { if (id != this.id && name == this.name) { this.checked = false; } @@ -513,6 +516,7 @@ export class MdRadioButton extends _MdRadioButtonMixinBase ngOnDestroy() { this._focusOriginMonitor.stopMonitoring(this._inputElement.nativeElement); + this._radioDispatcherListener(); } /** Dispatch change event with current value. */ From b1add8160c42e6c577a05fdbb353b06d97f427ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Neto=C4=8Dn=C3=BD?= Date: Fri, 16 Jun 2017 14:05:35 +0200 Subject: [PATCH 2/5] Fixed lint errors --- src/lib/button-toggle/button-toggle.ts | 11 ++++++----- .../coordination/unique-selection-dispatcher.ts | 2 +- src/lib/expansion/accordion-item.ts | 13 +++++++------ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/lib/button-toggle/button-toggle.ts b/src/lib/button-toggle/button-toggle.ts index fe6dd406056d..188ec6e6c784 100644 --- a/src/lib/button-toggle/button-toggle.ts +++ b/src/lib/button-toggle/button-toggle.ts @@ -376,11 +376,12 @@ export class MdButtonToggle implements OnInit, OnDestroy { this.buttonToggleGroupMultiple = toggleGroupMultiple; if (this.buttonToggleGroup) { - this._buttonToggleDispatcherListener = _buttonToggleDispatcher.listen((id: string, name: string) => { - if (id != this.id && name == this.name) { - this.checked = false; - } - }); + this._buttonToggleDispatcherListener = _buttonToggleDispatcher.listen( + (id: string, name: string) => { + if (id != this.id && name == this.name) { + this.checked = false; + } + }); this._type = 'radio'; this.name = this.buttonToggleGroup.name; diff --git a/src/lib/core/coordination/unique-selection-dispatcher.ts b/src/lib/core/coordination/unique-selection-dispatcher.ts index bfe2eee37d5b..76d0c228cf64 100644 --- a/src/lib/core/coordination/unique-selection-dispatcher.ts +++ b/src/lib/core/coordination/unique-selection-dispatcher.ts @@ -46,7 +46,7 @@ export class UniqueSelectionDispatcher { this._listeners = this._listeners.filter((registered: UniqueSelectionDispatcherListener) => { return listener !== registered; }); - } + }; } } diff --git a/src/lib/expansion/accordion-item.ts b/src/lib/expansion/accordion-item.ts index a84d64ac3f3e..0fedefa59473 100644 --- a/src/lib/expansion/accordion-item.ts +++ b/src/lib/expansion/accordion-item.ts @@ -53,12 +53,13 @@ export class AccordionItem implements OnDestroy { constructor(@Optional() public accordion: CdkAccordion, protected _expansionDispatcher: UniqueSelectionDispatcher) { - this._expansionDispatcherListener = _expansionDispatcher.listen((id: string, accordionId: string) => { - if (this.accordion && !this.accordion.multi && - this.accordion.id === accordionId && this.id !== id) { - this.expanded = false; - } - }); + this._expansionDispatcherListener = _expansionDispatcher.listen( + (id: string, accordionId: string) => { + if (this.accordion && !this.accordion.multi && + this.accordion.id === accordionId && this.id !== id) { + this.expanded = false; + } + }); } /** Emits an event for the accordion item being destroyed. */ From b5f7d9f399e09b363858bf86b2d0687e9edea7d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Neto=C4=8Dn=C3=BD?= Date: Fri, 16 Jun 2017 14:12:09 +0200 Subject: [PATCH 3/5] Fixed lint - missing whitespace --- src/lib/button-toggle/button-toggle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/button-toggle/button-toggle.ts b/src/lib/button-toggle/button-toggle.ts index 188ec6e6c784..f49e55d5dcc8 100644 --- a/src/lib/button-toggle/button-toggle.ts +++ b/src/lib/button-toggle/button-toggle.ts @@ -454,7 +454,7 @@ export class MdButtonToggle implements OnInit, OnDestroy { // Unregister buttonToggleDispatcherListener on destroy ngOnDestroy(): void { - if(this._buttonToggleDispatcherListener) { + if (this._buttonToggleDispatcherListener) { this._buttonToggleDispatcherListener(); } } From cfd98ac2a60aab9e7f224b3ef4d438eb7b4fe423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Neto=C4=8Dn=C3=BD?= Date: Tue, 20 Jun 2017 09:32:32 +0200 Subject: [PATCH 4/5] Changed return type from Function to () => void --- src/lib/button-toggle/button-toggle.ts | 12 +++++------- .../coordination/unique-selection-dispatcher.ts | 2 +- src/lib/expansion/accordion-item.ts | 10 +++++----- src/lib/radio/radio.ts | 15 ++++++++------- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/lib/button-toggle/button-toggle.ts b/src/lib/button-toggle/button-toggle.ts index f49e55d5dcc8..5c4ae67fedc3 100644 --- a/src/lib/button-toggle/button-toggle.ts +++ b/src/lib/button-toggle/button-toggle.ts @@ -288,8 +288,8 @@ export class MdButtonToggle implements OnInit, OnDestroy { /** Whether or not the button toggle is a single selection. */ private _isSingleSelector: boolean = null; - /** Unregister function for _buttonToggleDispatcherListener **/ - private _buttonToggleDispatcherListener: Function; + /** Unregister function for _buttonToggleDispatcher **/ + private _removeUniqueSelectionListener: () => void = () => {}; @ViewChild('input') _inputElement: ElementRef; @@ -376,8 +376,8 @@ export class MdButtonToggle implements OnInit, OnDestroy { this.buttonToggleGroupMultiple = toggleGroupMultiple; if (this.buttonToggleGroup) { - this._buttonToggleDispatcherListener = _buttonToggleDispatcher.listen( - (id: string, name: string) => { + this._removeUniqueSelectionListener = + _buttonToggleDispatcher.listen((id: string, name: string) => { if (id != this.id && name == this.name) { this.checked = false; } @@ -454,8 +454,6 @@ export class MdButtonToggle implements OnInit, OnDestroy { // Unregister buttonToggleDispatcherListener on destroy ngOnDestroy(): void { - if (this._buttonToggleDispatcherListener) { - this._buttonToggleDispatcherListener(); - } + this._removeUniqueSelectionListener(); } } diff --git a/src/lib/core/coordination/unique-selection-dispatcher.ts b/src/lib/core/coordination/unique-selection-dispatcher.ts index 76d0c228cf64..6f96f5713f74 100644 --- a/src/lib/core/coordination/unique-selection-dispatcher.ts +++ b/src/lib/core/coordination/unique-selection-dispatcher.ts @@ -40,7 +40,7 @@ export class UniqueSelectionDispatcher { * Listen for future changes to item selection. * @return Function used to unregister listener **/ - listen(listener: UniqueSelectionDispatcherListener): Function { + listen(listener: UniqueSelectionDispatcherListener): () => void { this._listeners.push(listener); return () => { this._listeners = this._listeners.filter((registered: UniqueSelectionDispatcherListener) => { diff --git a/src/lib/expansion/accordion-item.ts b/src/lib/expansion/accordion-item.ts index 0fedefa59473..52ec10681bc8 100644 --- a/src/lib/expansion/accordion-item.ts +++ b/src/lib/expansion/accordion-item.ts @@ -48,13 +48,13 @@ export class AccordionItem implements OnDestroy { } private _expanded: boolean; - /** Unregister function for _expansionDispatcherListener **/ - private _expansionDispatcherListener: Function; + /** Unregister function for _expansionDispatcher **/ + private _removeUniqueSelectionListener: () => void = () => {}; constructor(@Optional() public accordion: CdkAccordion, protected _expansionDispatcher: UniqueSelectionDispatcher) { - this._expansionDispatcherListener = _expansionDispatcher.listen( - (id: string, accordionId: string) => { + this._removeUniqueSelectionListener = + _expansionDispatcher.listen((id: string, accordionId: string) => { if (this.accordion && !this.accordion.multi && this.accordion.id === accordionId && this.id !== id) { this.expanded = false; @@ -65,7 +65,7 @@ export class AccordionItem implements OnDestroy { /** Emits an event for the accordion item being destroyed. */ ngOnDestroy() { this.destroyed.emit(); - this._expansionDispatcherListener(); + this._removeUniqueSelectionListener(); } /** Toggles the expanded state of the accordion item. */ diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts index ddb90c8804fa..1cea973ed216 100644 --- a/src/lib/radio/radio.ts +++ b/src/lib/radio/radio.ts @@ -459,7 +459,7 @@ export class MdRadioButton extends _MdRadioButtonMixinBase private _focusRipple: RippleRef; /** Unregister function for _radioDispatcher **/ - private _radioDispatcherListener: Function; + private _removeUniqueSelectionListener: () => void = () => {}; /** The native `` element */ @ViewChild('input') _inputElement: ElementRef; @@ -476,11 +476,12 @@ export class MdRadioButton extends _MdRadioButtonMixinBase // TODO(jelbourn): Assert that there's no name binding AND a parent radio group. this.radioGroup = radioGroup; - this._radioDispatcherListener = _radioDispatcher.listen((id: string, name: string) => { - if (id != this.id && name == this.name) { - this.checked = false; - } - }); + this._removeUniqueSelectionListener = + _radioDispatcher.listen((id: string, name: string) => { + if (id != this.id && name == this.name) { + this.checked = false; + } + }); } /** Focuses the radio button. */ @@ -516,7 +517,7 @@ export class MdRadioButton extends _MdRadioButtonMixinBase ngOnDestroy() { this._focusOriginMonitor.stopMonitoring(this._inputElement.nativeElement); - this._radioDispatcherListener(); + this._removeUniqueSelectionListener(); } /** Dispatch change event with current value. */ From 7daeb3f0807b0029a72a3d9b9ad0ab669221d00d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Neto=C4=8Dn=C3=BD?= Date: Tue, 20 Jun 2017 09:54:30 +0200 Subject: [PATCH 5/5] Test that the listeners are correctly unregistered. --- .../unique-selection-dispatcher.spec.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 src/lib/core/coordination/unique-selection-dispatcher.spec.ts diff --git a/src/lib/core/coordination/unique-selection-dispatcher.spec.ts b/src/lib/core/coordination/unique-selection-dispatcher.spec.ts new file mode 100644 index 000000000000..32b8e81fa930 --- /dev/null +++ b/src/lib/core/coordination/unique-selection-dispatcher.spec.ts @@ -0,0 +1,32 @@ +import {UniqueSelectionDispatcher} from './unique-selection-dispatcher'; + + +describe('Unique selection dispatcher', () => { + + describe('register', () => { + it('once unregistered the listener must not be called on notify', (done) => { + let dispatcher: UniqueSelectionDispatcher = new UniqueSelectionDispatcher(); + let called = false; + + // Register first listener + dispatcher.listen(() => { + called = true; + }); + + // Register a listener + let deregisterFn = dispatcher.listen(() => { + done.fail('Should not be called'); + }); + + // Unregister + deregisterFn(); + + // Call registered listeners + dispatcher.notify('testId', 'testName'); + + expect(called).toBeTruthy('Registered listener must be called.'); + + done(); + }); + }); +});