Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): DebugElement.listeners not cleared on destroy #31820

Closed
wants to merge 1 commit into from
Closed

fix(core): DebugElement.listeners not cleared on destroy #31820

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

Currently the DebugElement.listeners array are retained after the node is destroyed. This means that they'll continue to fire through triggerEventHandler and can cause memory leaks. This has already been fixed in Ivy, but these changes fix it in ViewEngine for consistency.

@crisbeto crisbeto force-pushed the FW-1464/debug-node-destroy-listeners branch from 24e1346 to 88d83a0 Compare July 24, 2019 14:48
@crisbeto
fix(core): DebugElement.listeners not cleared on destroy
56c503b 
Currently the `DebugElement.listeners` array are retained after the node is destroyed. This means that they'll continue to fire through `triggerEventHandler` and can cause memory leaks. This has already been fixed in Ivy, but these changes fix it in ViewEngine for consistency.
@crisbeto crisbeto force-pushed the FW-1464/debug-node-destroy-listeners branch from 88d83a0 to 56c503b Compare July 24, 2019 14:58
@crisbeto crisbeto added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jul 24, 2019
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Jul 24, 2019
@crisbeto crisbeto marked this pull request as ready for review July 24, 2019 15:44
@crisbeto crisbeto requested a review from a team as a code owner July 24, 2019 15:44
@crisbeto
Copy link
Member Author

Merge assistance: We should run a presubmit to see how break-ey this would be in G3, before we decide whether to merge it.

@AndrewKushnir AndrewKushnir added state: blocked and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jul 24, 2019
@AndrewKushnir
Copy link
Contributor

@crisbeto thanks for the fix! I will run global presubmit tonight and report back all the findings. For now, I marked this PR as "blocked", so we don't merge it.

@AndrewKushnir
Copy link
Contributor

VE Blueprint Presubmit (will run global presubmit tonight).

@AndrewKushnir
Copy link
Contributor

FYI, VE Blueprint Presubmit indicated no problems caused by these changes (all targets passed). Will let you know the result after running global TAP tonight.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jul 24, 2019
kara
kara approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, one nit

packages/core/src/view/services.ts Show resolved Hide resolved
@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 24, 2019
@kara kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 24, 2019
@kara kara added the area: core Issues related to the framework runtime label Jul 24, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 24, 2019
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 25, 2019 •
edited

VE Global TAP Presubmit

@AndrewKushnir
Copy link
Contributor

Quick update: Global TAP identified 1 target failing due to this change, however there might be more, since a portion of targets were in failed state already. I will try to make the broken target forward-compatible with this change and after that run another global TAP. Thank you.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 26, 2019 •
edited

VE Global TAP Presubmit #2

@AndrewKushnir
Copy link
Contributor

"Global TAP Presubmit 2" looks good, my plan is:

  • run final Global TAP this Sunday (to make sure there are no extra failed targets)
  • Global TAP is green, I'll merge this change on Monday and sync to g3

Thank you.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 29, 2019 •
edited

VE Global TAP Presubmit #3

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Jul 29, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 29, 2019
@crisbeto @AndrewKushnir
fix(core): DebugElement.listeners not cleared on destroy (#31820)
46b160e 
Currently the `DebugElement.listeners` array are retained after the node is destroyed. This means that they'll continue to fire through `triggerEventHandler` and can cause memory leaks. This has already been fixed in Ivy, but these changes fix it in ViewEngine for consistency.

PR Close #31820
@AndrewKushnir
Copy link
Contributor

FYI, the most recent Global TAP Presubmit indicated no failing targets related to this change, so I've merged this PR and will do a sync to google3. Thank you.

sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@crisbeto @sabeersulaiman
fix(core): DebugElement.listeners not cleared on destroy (angular#31820)
bac18b7 
Currently the `DebugElement.listeners` array are retained after the node is destroyed. This means that they'll continue to fire through `triggerEventHandler` and can cause memory leaks. This has already been fixed in Ivy, but these changes fix it in ViewEngine for consistency.

PR Close angular#31820
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes risk: medium target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@crisbeto @AndrewKushnir @kara @googlebot