Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(panel): memory leak on destroy #11145

Merged
merged 1 commit into from
Mar 12, 2018
Merged

Conversation

tiberiuzuld
Copy link
Contributor

@tiberiuzuld tiberiuzuld commented Mar 1, 2018

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When destroying md-tooltip the callback onDomAdded remained attached and was causing a memory leak.

Issue Number:
#11133

What is the new behavior?

Added 4 lines with setting all 4 callbacks to null in MdPanelRef.prototype.destroy.

From my tests this will clear the memory leak.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 1, 2018
@Splaktar Splaktar self-assigned this Mar 3, 2018
@Splaktar Splaktar added the needs: more info The issue does not contain enough information for the team to determine if it is a real bug label Mar 3, 2018
@Splaktar
Copy link
Member

Splaktar commented Mar 3, 2018

Do you have a CodePen or repro steps for the docs or something that can help to reproduce this issue?

Is there a related issue open already? Is it related to #11133?

@Splaktar Splaktar changed the title fix(panel) memory leak on destroy fix(panel): memory leak on destroy Mar 3, 2018
@Splaktar Splaktar added type: bug severity: memory leak Issues where a memory leak is triggered. labels Mar 3, 2018
@tiberiuzuld
Copy link
Contributor Author

tiberiuzuld commented Mar 3, 2018

Yes is the same issue as described in #11133
Didn't see the issue since I searched for panel.
This PR fixes #11133

@Splaktar Splaktar added needs: review This PR is waiting on review from the team needs: manual testing This issue or PR needs to have some manual testing and verification done and removed needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Mar 3, 2018
@Splaktar Splaktar added this to the 1.1.8 milestone Mar 3, 2018
@Splaktar Splaktar added the P1: urgent Urgent issues that should be addressed in the next minor or patch release. label Mar 3, 2018
@Splaktar
Copy link
Member

Splaktar commented Mar 3, 2018

Excellent, thank you! I will do some manual testing to verify the fix.

Can you please update the commit message to add Fixes #11133 to the footer? That will help with the changelog.

@tiberiuzuld
Copy link
Contributor Author

Yes will add.
Thanks

When destroying md-tooltip the callback `onDomAdded` remained attached and was causing a memory leak.

Added 4 lines with setting all 4 callbacks to null in `MdPanelRef.prototype.destroy`.

Fixes angular#11133
@tiberiuzuld
Copy link
Contributor Author

Done

@Splaktar Splaktar removed the needs: manual testing This issue or PR needs to have some manual testing and verification done label Mar 6, 2018
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer pr: presubmit-failures and removed needs: review This PR is waiting on review from the team labels Mar 6, 2018
@Splaktar
Copy link
Member

This is currently triggering some presubmit failures that appear to be flakes (i.e. from flaky tests, not actual real failures). This is being looked at internally by Google. No work is needed on your side at this time.

@Splaktar Splaktar added the severity: regression This issue is related to a regression label Mar 10, 2018
@jelbourn
Copy link
Member

Passes presubmit

@jelbourn jelbourn merged commit 2ef87f4 into angular:master Mar 12, 2018
chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018
When destroying md-tooltip the callback `onDomAdded` remained attached and was causing a memory leak.

Added 4 lines with setting all 4 callbacks to null in `MdPanelRef.prototype.destroy`.

Fixes angular#11133
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
When destroying md-tooltip the callback `onDomAdded` remained attached and was causing a memory leak.

Added 4 lines with setting all 4 callbacks to null in `MdPanelRef.prototype.destroy`.

Fixes #11133
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review severity: memory leak Issues where a memory leak is triggered. severity: regression This issue is related to a regression type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants