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

Potential Memory Leak With Inactive md-tab #1986

Closed
nkoterba opened this issue Mar 20, 2015 · 8 comments
Closed

Potential Memory Leak With Inactive md-tab #1986

nkoterba opened this issue Mar 20, 2015 · 8 comments
Assignees
Milestone

Comments

@nkoterba
Copy link

In relation to #1982, I'm including code in my directives' controllers that prints out when controllers are destroyed.

For example:

       $scope.$on('$destroy', function () {
        $log.debug('Destroying: file-grid-ctrl');

        // Add any cleanup/tear-down logic here
    });

Based on my exploration of the code and the documentation, internal non-active tab content is removed from the $scope\$digest cycle:

https://material.angularjs.org/#/api/material.components.tabs/directive/mdTabs:

As a performance bonus, if the tab content is managed internally then the non-active (non-visible) tab
contents are temporarily disconnected from the $scope.$digest() processes; which restricts and optimizes
DOM updates to only the currently active tab.

Since Material Design detaches inactive Tab content from the $scope/$digest cycle, none of my controllers for all my "inactive" tabs print out Destorying: file-grid-ctrl. This leads me to believe that any resources or other things I may be "destroying/tearing down/disposing" of in my $destroy callback are not being called, leading to potential memory issues.

Perhaps instead when tabs are removed or destroyed, they should be "re-connected" to the $scope/$digest cycle right before destruction?

robertmesserle added a commit that referenced this issue Mar 20, 2015
Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
@robertmesserle robertmesserle added this to the 0.9.0 milestone Mar 20, 2015
@sondreb
Copy link

sondreb commented Mar 22, 2015

Is there any workaround to this issue? I have a bunch of directives within tabs that needs the $destroy event to clean up a lot of resources. Will this behavior change?

@sondreb
Copy link

sondreb commented Mar 22, 2015

LumX behaves as expected, where loaded directives within the tabs all receive the $destroy event. Following this issue and reverting back to Angular Md tab-control whenever there is a workaround available.

robertmesserle added a commit that referenced this issue Mar 24, 2015
BREAKING CHANGE: Generated HTML structure has changed, so custom styles
will need to be updated to match the new HTML structure.

Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
robertmesserle added a commit that referenced this issue Mar 24, 2015
BREAKING CHANGE: Generated HTML structure has changed, so custom styles
will need to be updated to match the new HTML structure.

Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
robertmesserle added a commit that referenced this issue Mar 25, 2015
BREAKING CHANGE: Generated HTML structure has changed, so custom styles
will need to be updated to match the new HTML structure.

Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
robertmesserle added a commit that referenced this issue Mar 25, 2015
BREAKING CHANGE: Generated HTML structure has changed, so custom styles
will need to be updated to match the new HTML structure.

Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
Closes #2020
robertmesserle added a commit that referenced this issue Mar 26, 2015
BREAKING CHANGE: Generated HTML structure has changed, so custom styles
will need to be updated to match the new HTML structure.

Closes #1087
Closes #1107
Closes #1140
Closes #1247
Closes #1261
Closes #1380
Closes #1387
Closes #1403
Closes #1443
Closes #1505
Closes #1506
Closes #1516
Closes #1518
Closes #1564
Closes #1570
Closes #1620
Closes #1626
Closes #1698
Closes #1777
Closes #1788
Closes #1850
Closes #1959
Closes #1986
Closes #2020
@sondreb
Copy link

sondreb commented Mar 28, 2015

Was this behavior changed, so it always calls $destroy now?

@sondreb
Copy link

sondreb commented Mar 30, 2015

I read through all the latest code changes related to the closing of this issue, are suppose to listen to tab deactivate to destroy resources? I mean, using directives within the tabs is something I'm sure many will do and at least for my use, my directive are themselves responsible for cleaning up resources during $destroy. I can't use this control until there is some workaround, it gives my app memory leak that uses hundreds of megabytes.

@robertmesserle
Copy link
Contributor

@sondreb The tabs code has been recently rewritten in master for 0.9.0 (not released yet). In the process, I tried to address as many existing issues as possible. One of these decisions was to remove our scope disconnect logic, which was likely the cause of your issue.

Once 0.9.0 is released, you shouldn't have any issues; however, if the issue does show up, please either comment here to let me know or open a new ticket.

@nkoterba
Copy link
Author

nkoterba commented Apr 1, 2015

@robertmesserle So I'm glad to hear the memory leak is most likely fixed in 0.9.0.

However, I did like the idea of how non - displayed tabs were removed from scope for performance reasons...is there any plan to re - add this feature?

@robertmesserle
Copy link
Contributor

Possibly, or it could be accomplished though ng-if. We plan to look into
our options at some point before v1.
On Wed, Apr 1, 2015 at 2:49 PM nkoterba notifications@github.com wrote:

@robertmesserle https://github.com/robertmesserle So I'm glad to hear
the memory leak is most likely fixed in 0.9.0.

However, I did like the idea of how non - displayed tabs were removed from
scope for performance reasons...is there any plan to re - add this feature?


Reply to this email directly or view it on GitHub
#1986 (comment).

@nkoterba
Copy link
Author

nkoterba commented Apr 2, 2015

@robertmesserle Ok great! I originally thought removing the tab from the digest cycle was a clever way to improve overall app performance until I stumbled across the destroy issue mentioned above.

Regarding ng-if, I would vote for another option (like removing and re-adding to the scope digest cycle) because ngIf behavior based on the Angular documents does not sound ideal: https://docs.angularjs.org/api/ng/directive/ngIf

Note that when an element is removed using ngIf its scope is destroyed and a new scope is 
created when the element is restored

and

Also, ngIf recreates elements using their compiled state. An example of this behavior is if 
an element's class attribute is directly modified after it's compiled, using something like jQuery's
.addClass() method, and the element is later removed. When ngIf recreates the element the 
added class will be lost because the original compiled state is used to regenerate the element.

We like how the old md-tabs implementation preserved the state of the controller even if the tab was inactive. If you switch to ngIf, then the controller state of each tab will be lost as the user navigates back and forth and we will have to figure out some way to persist tab content state...ugh.

My understanding is that ngHide and ngShow would maintain controller state but would not remove the internal tab contents from the digest cycle.

So our vote is for something other than ngIf.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants