components icon indicating copy to clipboard operation
components copied to clipboard

bug(ListKeyManager): ListKeyManager contain rxjs subscription memory leaks

Open sashaqred opened this issue 3 years ago • 0 comments
trafficstars

Is this a regression?

  • [ ] Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

No response

Description

There are a couple of observable subscriptions, but the mechanism of unsubscribing (using takeUntil or .unsubscribe) isn't implemented when we don't need that class anymore. This behavior could cause memory leaks in an app.

For example in withTypeAhead method, there is a call of this._typeaheadSubscription.unsubscribe();, but it works only for the previous subscription when call this method couple of times:

  • some component calls for the first time withTypeAhead(), a subscription is created
  • when it calls it for the second time, the subscription from the previous step is unsubscribed, and subscribed one more time
  • after the component is destroyed, the last subscription will still be in the memory

Also, the same subscription issue is in the constructor https://github.com/angular/components/blob/8504c7761f4fb334753e8ec4d8624ad85f071444/src/cdk/a11y/key-manager/list-key-manager.ts#L68

Reproduction

Steps to reproduce:

  1. Open src/cdk/a11y/key-manager/list-key-manager.ts
  2. In the method withTypeAhead after this._typeaheadSubscription.unsubscribe(); add some logging (console.log('LIST KEY MANAGER subscribed'))
  3. Pass the third argument to subscribe function like () => console.log('LISTKEYMANAGER unsubscribed')
  4. Run any test which uses ListKeyManager, for example arn test src/cdk/a11y --debug or yarn test src/cdk/menu --debug

After adding logs withTypeAhead could look like this:

withTypeAhead(debounceInterval: number = 200): this {
  if (
    (typeof ngDevMode === 'undefined' || ngDevMode) &&
    this._items.length &&
    this._items.some(item => typeof item.getLabel !== 'function')
  ) {
    throw Error('ListKeyManager items in typeahead mode must implement the `getLabel` method.');
  }

  this._typeaheadSubscription.unsubscribe();
  console.log('LIST KEY MANAGER subscribed');

  // Debounce the presses of non-navigational keys, collect the ones that correspond to letters
  // and convert those letters back into a string. Afterwards find the first item that starts
  // with that string and select it.
  this._typeaheadSubscription = this._letterKeyStream
    .pipe(
      tap(letter => this._pressedLetters.push(letter)),
      debounceTime(debounceInterval),
      filter(() => this._pressedLetters.length > 0),
      map(() => this._pressedLetters.join('')),
    )
    .subscribe(
      inputString => {
        const items = this._getItemsArray();

        // Start at 1 because we want to start searching at the item immediately
        // following the current active item.
        for (let i = 1; i < items.length + 1; i++) {
          const index = (this._activeItemIndex + i) % items.length;
          const item = items[index];

          if (
            !this._skipPredicateFn(item) &&
            item.getLabel!().toUpperCase().trim().indexOf(inputString) === 0
          ) {
            this.setActiveItem(index);
            break;
          }
        }

        this._pressedLetters = [];
      },
      undefined,
      () => console.log('LIST KEY MANAGER unsubscribed'),
    );

  return this;
}

Expected Behavior

'LIST KEY MANAGER unsubscribed' string should be in the console exactly the same times as 'LIST KEY MANAGER subscribed' string.

Actual Behavior

Only 'LIST KEY MANAGER subscribed' string is presented in console.

Environment

  • Angular:
  • CDK/Material: https://github.com/angular/components/commit/e63390c712b8e66b81b6f026dcfd313c0fb3a682
  • Browser(s): —
  • Operating System (e.g. Windows, macOS, Ubuntu): MacOS 12.5

sashaqred avatar Aug 26 '22 11:08 sashaqred