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.

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

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

Closed
wants to merge 2 commits into from

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented Nov 29, 2020

PR Checklist

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

@google-cla google-cla bot added the cla: yes label Nov 29, 2020
@pullapprove pullapprove bot requested a review from mhevery Nov 29, 2020
@mhevery mhevery added action: merge PR author is ready for this to merge target: patch This PR is targeted for the next patch release labels Nov 30, 2020
@mhevery mhevery self-assigned this Nov 30, 2020
@mhevery
Copy link
Contributor

@mhevery mhevery commented Nov 30, 2020

presubmit

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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

packages/core/src/application_ref.ts Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 3033,
"main-es2015": 447975,
"main-es2015": 448535,
Copy link
Contributor

@AndrewKushnir AndrewKushnir Nov 30, 2020

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/core/src/application_ref.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from IgorMinar Nov 30, 2020
@AndrewKushnir AndrewKushnir added action: cleanup action: presubmit A standard presubmit is running / required labels Nov 30, 2020
@jessicajaniuk jessicajaniuk added the comp: core Runtime issues label Dec 1, 2020
@ngbot ngbot bot added this to the needsTriage milestone Dec 1, 2020
@mhevery
Copy link
Contributor

@mhevery mhevery commented Dec 1, 2020

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

@mhevery
Copy link
Contributor

@mhevery mhevery commented Dec 1, 2020

presubmit

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 1, 2020

@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 #39365 that you've referred to. So these changes should address #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.

@AndrewKushnir AndrewKushnir removed the action: merge PR author is ready for this to merge label Dec 1, 2020
@arturovt
Copy link
Contributor Author

@arturovt arturovt commented Dec 1, 2020

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

arturovt and others added 2 commits Dec 1, 2020
…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
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 1, 2020

Presubmit.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Reviewed-for: size-tracking

mhevery
mhevery approved these changes Dec 2, 2020
Copy link
Contributor

@mhevery mhevery left a comment

reviewed-for: global-approvers

@mhevery mhevery removed the request for review from IgorMinar Dec 2, 2020
@mhevery mhevery removed request for atscott, jelbourn and alxhub Dec 2, 2020
@mhevery mhevery added the action: merge PR author is ready for this to merge label Dec 2, 2020
@AndrewKushnir AndrewKushnir removed the action: presubmit A standard presubmit is running / required label Dec 2, 2020
@mhevery 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 mhevery closed this in df27027 Dec 2, 2020
mhevery pushed a commit that referenced this issue Dec 2, 2020
…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
@mhevery 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
mhevery pushed a commit that referenced this issue Dec 2, 2020
…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
@arturovt arturovt deleted the fix/22106 branch Dec 2, 2020
mhevery pushed a commit that referenced this issue Dec 3, 2020
…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
mhevery added a commit to mhevery/angular that referenced this issue Dec 14, 2020
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
mhevery added a commit to mhevery/angular that referenced this issue Dec 14, 2020
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
mhevery added a commit to mhevery/angular that referenced this issue Dec 14, 2020
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
mhevery added a commit to mhevery/angular that referenced this issue Dec 14, 2020
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
mhevery added a commit to mhevery/angular that referenced this issue Dec 15, 2020
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
josephperrott pushed a commit that referenced this issue Dec 22, 2020
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
josephperrott pushed a commit that referenced this issue Dec 22, 2020
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
Copy link

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

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 Jan 2, 2021
@pullapprove pullapprove bot removed the comp: core Runtime issues label Jan 2, 2021
@ngbot ngbot bot removed this from the needsTriage milestone Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants