fix: Clear the "view" to avoid left over referenced once the View is detached and should be collected by GC #6353

VladimirAmiorkov · 2018-10-04T13:54:32Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - Due to having to profile the JS heap I am not sure how we can efficiently make a unit test for that

What is the current behavior?

When using the Angular createEmbeddeedView from the ViewContainerRef, after calling clear() the created NativeScript Views are left in the memory

What is the new behavior?

When using the Angular createEmbeddeedView from the ViewContainerRef, after calling clear() the created NativeScript Views are collected by the GC and a no longer in the memory

Fixes/Implements/Closes #[Issue Number].
ProgressNS/nativescript-ui-feedback#825

facetious · 2018-10-04T14:19:22Z

tns-core-modules/ui/styling/style-scope.ts

    public onUnloaded(): void {
        this.unsubscribeFromDynamicUpdates();
+        this.view = undefined;
+        this._matchInvalid = false;


This variable is poorly named. "Match invalid = false" implies that the match is valid, but you're about to set the match to undefined, which strikes me as ultimately invalid. It is usually best to name boolean variables as the "positive"; i.e. change the naming of the variable to _matchValid so that you're not doing gymnastics to understand double-negatives.

SvetoslavTsenov · 2018-10-04T15:36:50Z

@VladimirAmiorkov, could you please rebase your branch on master, since there are a lot of important fixes.

…detached and should be collected by GC

VladimirAmiorkov · 2018-10-05T06:23:54Z

@SvetoslavTsenov Yes of course. Done.

vakrilov · 2018-10-05T13:02:42Z

Hey @VladimirAmiorkov

Can you elaborate a bit on what is holding a reference to the CssState in this scenario?
Any ideas on how this fix can be tested so that we ensure that there are no regressions in the future.
The view field is set in the constructor. It is possible that the view is loaded/unloaded multiple times. If you clear the reference in the unloaded -> that means that next time the view is loaded again it the CSSState associated to it will not work as expected. You should either set the view again in loaded or clear the view field when you are sure the view will no longer be loaded again.

VladimirAmiorkov · 2018-10-05T14:25:51Z

@vakrilov

I noticed a couple of other things that could be a source of this memory leak. I will investigate them now and get back at you.

VladimirAmiorkov · 2018-10-08T10:39:36Z

Hi @vakrilov ,

After researching this issue today I am unable to observe it even without any fixes in the tns-core-modules. Currently the only fixes that are required for this issue are this PR in nativescript-angular and an fix in the nativescript-ui-listview. It is very odd that last week while working on this I was able to observe the CssState in the retainers chain but today I am no longer able to observe the same. The CssState was in the retainers chain but its start was somewhere inside the core.js of Angular itself so it is possible something from Angular was the cause that has been resolved, I do hope that the Chrome Dev tools Snapshot/profiling is not misguiding us on this.

I suggest we close this PR for now since I can no longer observe an memory leak when working with the latest version (^4.2.1) of tns-core-modules and/or with the next version.

VladimirAmiorkov · 2018-10-08T11:34:03Z

On a side note:

It may be nothing but I noticed that in the CssState class we are subscribing to one event here and unsubscribing from a different one here, meaning we subscribe to (attribute)Change but unsubscribe from onPropertyChanged:(attribute). This looks strange but I was not able to test it to see if it could lead to some sort of leak. If you know how I could test this part of the code let me know.

lock · 2019-10-11T07:52:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

VladimirAmiorkov self-assigned this Oct 4, 2018

ghost added the in progress label Oct 4, 2018

ns-bot added the cla: yes label Oct 4, 2018

VladimirAmiorkov mentioned this pull request Oct 4, 2018

RLV Memory leak in Angular application ProgressNS/nativescript-ui-feedback#825

Closed

facetious reviewed Oct 4, 2018

View reviewed changes

fix: Clear the "view" to avoid left over referenced once the View is …

3212837

…detached and should be collected by GC

VladimirAmiorkov force-pushed the amiorkov/embedded-views-memory-leak-fix branch from e639a66 to 3212837 Compare October 5, 2018 06:23

VladimirAmiorkov closed this Oct 11, 2018

ghost removed the in progress label Oct 11, 2018

VladimirAmiorkov deleted the amiorkov/embedded-views-memory-leak-fix branch October 11, 2018 07:16

VladimirAmiorkov mentioned this pull request Nov 1, 2018

Resolve incorrect name of listener when unsubscribing causing Views to be leaked in memory #6487

Merged

5 tasks

lock bot locked and limited conversation to collaborators Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: Clear the "view" to avoid left over referenced once the View is detached and should be collected by GC #6353

fix: Clear the "view" to avoid left over referenced once the View is detached and should be collected by GC #6353

Uh oh!

VladimirAmiorkov commented Oct 4, 2018

Uh oh!

facetious Oct 4, 2018 •

edited

Loading

Uh oh!

This comment was marked as abuse.

Uh oh!

SvetoslavTsenov commented Oct 4, 2018

Uh oh!

VladimirAmiorkov commented Oct 5, 2018

Uh oh!

vakrilov commented Oct 5, 2018

Uh oh!

VladimirAmiorkov commented Oct 5, 2018 •

edited

Loading

Uh oh!

VladimirAmiorkov commented Oct 8, 2018 •

edited

Loading

Uh oh!

VladimirAmiorkov commented Oct 8, 2018 •

edited

Loading

Uh oh!

lock bot commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

fix: Clear the "view" to avoid left over referenced once the View is detached and should be collected by GC #6353

fix: Clear the "view" to avoid left over referenced once the View is detached and should be collected by GC #6353

Uh oh!

Conversation

VladimirAmiorkov commented Oct 4, 2018

PR Checklist

What is the current behavior?

What is the new behavior?

Uh oh!

facetious Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

SvetoslavTsenov commented Oct 4, 2018

Uh oh!

VladimirAmiorkov commented Oct 5, 2018

Uh oh!

vakrilov commented Oct 5, 2018

Uh oh!

VladimirAmiorkov commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladimirAmiorkov commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladimirAmiorkov commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lock bot commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

facetious Oct 4, 2018 •

edited

Loading

VladimirAmiorkov commented Oct 5, 2018 •

edited

Loading

VladimirAmiorkov commented Oct 8, 2018 •

edited

Loading

VladimirAmiorkov commented Oct 8, 2018 •

edited

Loading