fix(core): remove application from the testability registry when the root view is removed #39876

arturovt · 2020-11-29T00:07:07Z

PR Checklist

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #22106

What is the new behavior?

In the new behavior Angular removes applications from the testability registry when the root view gets destroyed. This eliminates a memory leak, because before that the TestabilityRegistry holds references to HTML elements, thus they cannot be GCed.

Does this PR introduce a breaking change?

Yes
No

mhevery · 2020-11-30T18:59:39Z

presubmit

AndrewKushnir

@arturovt, thanks for the PR, I've just left a couple comments. Thank you.

packages/core/src/application_ref.ts

AndrewKushnir · 2020-11-30T19:50:05Z

goldens/size-tracking/aio-payloads.json

@@ -12,7 +12,7 @@
    "master": {
      "uncompressed": {
        "runtime-es2015": 3033,
-        "main-es2015": 447975,
+        "main-es2015": 448535,


FYI, I've checked the current size and it's 448229, so the delta introduced by this PR is 306 bytes (which I think is the case for other affected payload sizes as well). I don't see any symbols that become non-tree-shakable due to these changes, so I guess the size is coming from the names of new private fields (note: I'm not suggesting renaming them, just sharing observations with other reviewers).

packages/core/src/application_ref.ts

mhevery · 2020-12-01T00:56:58Z

I fixed up the code to make it consistent between Ivy and VE. @AndrewKushnir please have a look.

mhevery · 2020-12-01T01:04:08Z

presubmit

AndrewKushnir · 2020-12-01T03:42:17Z

@mhevery thanks, I will have a look and add extra tests as we discussed. I just noticed that the commit message is a bit strange: it refers to a different commit from master branch - could you please have a look when you get a chance?

@arturovt just wanted to give a quick update: we've came across some inconsistencies in existing code while reviewing the changes in this PR and we went ahead with additional fixes:

to make sure that the cleanup is consistent in case ApplicationRef is destroyed (as a result of an Injector destroy) or a ComponentRef.destroy is called.
the callbacks registered with componentRef.onDestroy(cb) are invoked when corresponding view is removed. This was the problem in ComponentRef<T> onDestroyed() callback does not get called #39365 that you've referred to. So these changes should address ComponentRef<T> onDestroyed() callback does not get called #39365 as well.

I plan to add a couple more tests to verify the above-mentioned scenarios (and the one from #39365) and will push an extra commit. Please let us know if you're ok with us contributing to your PR (if you're not ok, we can revert our changes and create a separate PR).

@arturovt it'd be helpful if you could also have a look at the changes in this PR (as original author of the changes) and let us know if you have any questions/feedback.

Thank you.

arturovt · 2020-12-01T07:38:39Z

@AndrewKushnir I don't have any questions, looks great. Seems also great that it closes another issue simultaneously.

…root view is removed In the new behavior Angular removes applications from the testability registry when the root view gets destroyed. This eliminates a memory leak, because before that the TestabilityRegistry holds references to HTML elements, thus they cannot be GCed. PR Close #22106

…f is destroyed This commit adds a few tests to verify that the `onDestroy` callbacks are invoked when `ComponentRef` instance is destroyed and the logic is consistent between ViewEngine and Ivy.

AndrewKushnir · 2020-12-01T23:29:55Z

Presubmit.

AndrewKushnir

Reviewed-for: size-tracking

mhevery

reviewed-for: global-approvers

…f is destroyed (#39876) This commit adds a few tests to verify that the `onDestroy` callbacks are invoked when `ComponentRef` instance is destroyed and the logic is consistent between ViewEngine and Ivy. PR Close #39876

…root view is removed (#39876) In the new behavior Angular removes applications from the testability registry when the root view gets destroyed. This eliminates a memory leak, because before that the TestabilityRegistry holds references to HTML elements, thus they cannot be GCed. PR Close #22106 PR Close #39876

PR angular#39876 introduced an error where the `onDestroy` of `ComponentRef` would only get called if `ngDevMode` was set to true. This was because in dev mode we would freeze `TCleanup` to verify that no more static cleanup would get added to `TCleanup` array. This ensured that `TCleanup` was always present in dev mode. In production the `TCleanup` would get created only when needed. The resulting cleanup code was incorrectly indented and would only run if `TCleanup` was present causing this issue. Fix angular#40105

PR #39876 introduced an error where the `onDestroy` of `ComponentRef` would only get called if `ngDevMode` was set to true. This was because in dev mode we would freeze `TCleanup` to verify that no more static cleanup would get added to `TCleanup` array. This ensured that `TCleanup` was always present in dev mode. In production the `TCleanup` would get created only when needed. The resulting cleanup code was incorrectly indented and would only run if `TCleanup` was present causing this issue. Fix #40105 PR Close #40120

angular-automatic-lock-bot · 2021-01-02T16:47:50Z

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.}

google-cla bot added the cla: yes label Nov 29, 2020

pullapprove bot requested a review from mhevery November 29, 2020 00:07

mhevery approved these changes Nov 30, 2020

View reviewed changes

pullapprove bot requested review from alxhub, AndrewKushnir, atscott and jelbourn November 30, 2020 18:57

mhevery added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Nov 30, 2020

mhevery self-assigned this Nov 30, 2020

AndrewKushnir reviewed Nov 30, 2020

View reviewed changes

pullapprove bot requested a review from IgorMinar November 30, 2020 20:02

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels Nov 30, 2020

jessicajaniuk added the area: core Issues related to the framework runtime label Dec 1, 2020

ngbot bot added this to the needsTriage milestone Dec 1, 2020

AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Dec 1, 2020

arturovt and others added 2 commits December 1, 2020 11:50

test(core): verify onDestroy callbacks are invoked when ComponentRe…

66e8d75

…f is destroyed This commit adds a few tests to verify that the `onDestroy` callbacks are invoked when `ComponentRef` instance is destroyed and the logic is consistent between ViewEngine and Ivy.

AndrewKushnir 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 Dec 1, 2020

AndrewKushnir mentioned this pull request Dec 1, 2020

ComponentRef<T> onDestroyed() callback does not get called #39365

Closed

AndrewKushnir approved these changes Dec 2, 2020

View reviewed changes

mhevery approved these changes Dec 2, 2020

View reviewed changes

mhevery removed the request for review from IgorMinar December 2, 2020 18:17

mhevery removed request for atscott, jelbourn and alxhub December 2, 2020 18:17

mhevery added the action: merge The PR is ready for merge by the caretaker label Dec 2, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Dec 2, 2020

mhevery added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Dec 2, 2020

mhevery closed this in df27027 Dec 2, 2020

mhevery added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Dec 2, 2020

arturovt deleted the fix/22106 branch December 2, 2020 21:12

mhevery mentioned this pull request Dec 4, 2020

fix(core): fix memory leak on application reference destroy #22238

Closed

magnusbakken mentioned this pull request Dec 14, 2020

onDestroy on ComponentRef isn't called in production mode #40105

Closed

mhevery mentioned this pull request Dec 14, 2020

fix(core): Call onDestroy in production mode as well #40120

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 2, 2021

pullapprove bot removed the area: core Issues related to the framework runtime label Jan 2, 2021

ngbot bot removed this from the needsTriage milestone Jan 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): remove application from the testability registry when the root view is removed #39876

fix(core): remove application from the testability registry when the root view is removed #39876

arturovt commented Nov 29, 2020

mhevery commented Nov 30, 2020

AndrewKushnir left a comment

AndrewKushnir Nov 30, 2020

mhevery commented Dec 1, 2020

mhevery commented Dec 1, 2020

AndrewKushnir commented Dec 1, 2020

arturovt commented Dec 1, 2020

AndrewKushnir commented Dec 1, 2020

AndrewKushnir left a comment

mhevery left a comment

angular-automatic-lock-bot bot commented Jan 2, 2021

fix(core): remove application from the testability registry when the root view is removed #39876

fix(core): remove application from the testability registry when the root view is removed #39876

Conversation

arturovt commented Nov 29, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

mhevery commented Nov 30, 2020

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Nov 30, 2020

Choose a reason for hiding this comment

mhevery commented Dec 1, 2020

mhevery commented Dec 1, 2020

AndrewKushnir commented Dec 1, 2020

arturovt commented Dec 1, 2020

AndrewKushnir commented Dec 1, 2020

AndrewKushnir left a comment

Choose a reason for hiding this comment

mhevery left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Jan 2, 2021