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

decrementListenerCount: TypeError: Cannot read property '$destroy' of null #6897

Closed
ppawel opened this issue Mar 28, 2014 · 14 comments
Closed

Comments

@ppawel
Copy link

ppawel commented Mar 28, 2014

With AngularJS v1.3.0-build.2533+sha.ff5cf73 I get the following in my application. It occurs during navigation, it looks as if Angular tries to destroy one of the scopes in my application twice.

TypeError: Cannot read property '$destroy' of null
    at decrementListenerCount (http://localhost:9000/bower_components/angular/angular.js:12686:32)
    at http://localhost:9000/bower_components/angular/angular.js:1012:18
    at forEach (http://localhost:9000/bower_components/angular/angular.js:330:20)
    at Scope.$destroy (http://localhost:9000/bower_components/angular/angular.js:12306:9)
    at HTMLDivElement.<anonymous> (http://localhost:9000/bower_components/angular/angular.js:1017:18)
    at HTMLDivElement.jQuery.event.dispatch (http://localhost:9000/bower_components/jquery/jquery.js:5095:9)
    at HTMLDivElement.elemData.handle (http://localhost:9000/bower_components/jquery/jquery.js:4766:28)
    at Object.jQuery.event.trigger (http://localhost:9000/bower_components/jquery/jquery.js:5007:12)
    at jQuery.fn.extend.triggerHandler (http://localhost:9000/bower_components/jquery/jquery.js:5697:24)
    at removePatch [as remove] (http://localhost:9000/bower_components/angular/angular.js:2213:21) angular.js:9735
(anonymous function) angular.js:9735
(anonymous function) angular.js:7169
Scope.$broadcast angular.js:12640
$state.transition.resolved.then.$state.transition angular-ui-router.js:2114
wrappedCallback angular.js:11272
(anonymous function) angular.js:11358
Scope.$eval angular.js:12359
Scope.$digest angular.js:12177
Scope.$apply angular.js:12463
(anonymous function) angular.js:9524
jQuery.event.dispatch jquery.js:5095
elemData.handle

Scope looks like this at this point:

ChildScope {$id: null, this: null, $$listeners: null, $$listenerCount: null, $parent: null…}
$$childHead: null
$$childTail: null
$$destroyed: true
$$listenerCount: null
$$listeners: null
$$nextSibling: null
$$prevSibling: null
$$watchers: null
$id: null
$parent: null
data: null
selectSection: null
selectTab: null
tabs: null
this: null
@Narretz
Copy link
Contributor

Narretz commented Mar 28, 2014

Suspect: f552f25

@ppawel
Copy link
Author

ppawel commented Mar 28, 2014

BTW it works with 1.2.15 and also I'm pretty sure that it worked with 1.3 at some point.

@Narretz
Copy link
Contributor

Narretz commented Mar 28, 2014

Does it happen in FF and Chrome (or other browsers)?

@ppawel
Copy link
Author

ppawel commented Mar 28, 2014

Happens on both Firefox 28 and Chromium 33.0.1750.152 (OS is Linux).

@Narretz
Copy link
Contributor

Narretz commented Mar 28, 2014

Can you try with v1.3.0-build.2531+sha.ec8e395? If it doesn't happen there it's 50% in the commit I referenced. It'll also be helpful if you had a minimal reproduction.

@lgalfaso
Copy link
Contributor

looking into this

@lgalfaso
Copy link
Contributor

Ok, was able to reproduce this, there are a few small issues here:

  • Trying to listen to an event on a child scope of an already destroyed scope throws
  • Trying to deregister an event on a child scope of an already destroyed scope throws

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 28, 2014
When trying to register an event on a child scope of an already
destroyed scope, make this a noop
When trying to deregister an event on a child scope of an already
destroyed scope, make this a noop

Closes angular#6897
@lgalfaso lgalfaso removed their assignment Mar 28, 2014
@ppawel
Copy link
Author

ppawel commented Mar 29, 2014

Just curious, shouldn't you fix the cause and not the symptoms here? I mean it looks like attempt to destroy a scope twice is the real bug here and not the exception which is just a symptom.

Anyway, I won't be able to prepare a minimal test case for this, best I can do is try different builds to help you bisect if needed.

@lgalfaso
Copy link
Contributor

@ppawel a scope cannot be destroyed twice, the first check that is done on the $destroy method is that the scope is not destroyed. The issue is on the the event system when it tries to update the number of listeners for a particular event. There are several occasions when this can happens, when destroying a scope, when adding a listener or when removing a listener (in a strict sense, the case when removing a listener and destroying the scope is the same case).

The fact that you are seeing a scope that is destroyed, does not mean that this is the scope that we are attempting to destroy twice, but the fact that when some of the previous operations is performed on a child scope, it needs to modify the parent hierarchy.

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 29, 2014
Destroying the listener properties cases many issues when any of these
operations is being performed on a destroyed scope or a child scope
* Registering a new event
* Deregistering an existing event
* Destroying an insolated child scope with a destroyed parent
* Sending an `$emit` from a child scope with a destroyed parent

Closes angular#6897
@IgorMinar IgorMinar added this to the 1.3.0-beta.5 milestone Mar 31, 2014
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Mar 31, 2014
Add several checks on the methods that handle listeners so they are
able to handle destroyed scopes that now do not have the references
to the `$$listeners` and `$$listenersCount` when destroyed

Closes angular#6897
@IgorMinar
Copy link
Contributor

@jeffbcross this is related to the mem leak issue I'm dealing with, so I'll take over the ownership of this one.

@IgorMinar IgorMinar assigned IgorMinar and unassigned jeffbcross Apr 1, 2014
IgorMinar added a commit that referenced this issue Apr 1, 2014
…ze leaks

This reverts commit f552f25.

The commit is causing regressions.

Closes #6897
@IgorMinar
Copy link
Contributor

ok, I reverted the commit that introduced this. we still need some solution to deal with the V8 bug that the reverted commit worked around, but we need to do implement it better.

@lgalfaso I'm trying other strategies, but will keep your PR in mind in case I don't come up with another workaround.

IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.

V8 leaks are due to strong references from optimized code (fixed in M34) and inline
caches (fix in works). Inline caches are caches that the virtual machine builds on the
fly to speed up property access for javascript objects. These caches contain strong
references to objects so under certain conditions this can create a leak.

The reason why these leaks are extra bad for Scope instances is that scopes hold on
to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak.

This change removes references to objects that might be holding other big
objects. This means that even if the destroyed scope leaks, the child scopes
should not leak because we are not explicitly holding onto them.

Additionally in  theory we should also help make the current scope eligible for GC
by changing properties of the current Scope object.

I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se

Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.

This is a second attempt to enhance the cleanup, the first one failed  and was
reverted because it was too aggressive and caused problems for existing apps.
See: angular#6897

[1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073

Closes angular#6794
Closes angular#6856
IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.

V8 leaks are due to strong references from optimized code (fixed in M34) and inline
caches (fix in works). Inline caches are caches that the virtual machine builds on the
fly to speed up property access for javascript objects. These caches contain strong
references to objects so under certain conditions this can create a leak.

The reason why these leaks are extra bad for Scope instances is that scopes hold on
to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak.

This change removes references to objects that might be holding other big
objects. This means that even if the destroyed scope leaks, the child scopes
should not leak because we are not explicitly holding onto them.

Additionally in  theory we should also help make the current scope eligible for GC
by changing properties of the current Scope object.

I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se

Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.

This is a second attempt to enhance the cleanup, the first one failed  and was
reverted because it was too aggressive and caused problems for existing apps.
See: angular#6897

[1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073

Closes angular#6794
Closes angular#6856
IgorMinar added a commit that referenced this issue Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.

V8 leaks are due to strong references from optimized code (fixed in M34) and inline
caches (fix in works). Inline caches are caches that the virtual machine builds on the
fly to speed up property access for javascript objects. These caches contain strong
references to objects so under certain conditions this can create a leak.

The reason why these leaks are extra bad for Scope instances is that scopes hold on
to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak.

This change removes references to objects that might be holding other big
objects. This means that even if the destroyed scope leaks, the child scopes
should not leak because we are not explicitly holding onto them.

Additionally in  theory we should also help make the current scope eligible for GC
by changing properties of the current Scope object.

I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se

Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.

This is a second attempt to enhance the cleanup, the first one failed  and was
reverted because it was too aggressive and caused problems for existing apps.
See: #6897

[1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073

Closes #6794
Closes #6856
Closes #6968
IgorMinar added a commit that referenced this issue Apr 3, 2014
Due to a known V8 memory leak[1] we need to perform extra cleanup to make it easier
for GC to collect this scope object.

V8 leaks are due to strong references from optimized code (fixed in M34) and inline
caches (fix in works). Inline caches are caches that the virtual machine builds on the
fly to speed up property access for javascript objects. These caches contain strong
references to objects so under certain conditions this can create a leak.

The reason why these leaks are extra bad for Scope instances is that scopes hold on
to ton of stuff, so when a single scope leaks, it makes a ton of other stuff leak.

This change removes references to objects that might be holding other big
objects. This means that even if the destroyed scope leaks, the child scopes
should not leak because we are not explicitly holding onto them.

Additionally in  theory we should also help make the current scope eligible for GC
by changing properties of the current Scope object.

I was able to manually verify that this fixes the problem for the following
example app: http://plnkr.co/edit/FrSw6SCEVODk02Ljo8se

Given the nature of the problem I'm not 100% sure that this will work around
the V8 problem in scenarios common for Angular apps, but I guess it's better
than nothing.

This is a second attempt to enhance the cleanup, the first one failed  and was
reverted because it was too aggressive and caused problems for existing apps.
See: #6897

[1] V8 bug: https://code.google.com/p/v8/issues/detail?id=2073

Closes #6794
Closes #6856
Closes #6968
@ppawel
Copy link
Author

ppawel commented May 4, 2014

Just to confirm - it works for me now.

@e-oz
Copy link

e-oz commented Aug 5, 2014

I see this message in v1.3.0-build.3024+sha.60c13a1
code is pretty simple:

    $scope.$on('$destroy', function () {
      cancelComet = true;
    });

@yu521088
Copy link

I am still seeing this issue when doing like these:

  • set title like below:
    <ion-nav-title ng-if="boolean">xxx</ion-nav-title>
    <ion-nav-title ng-if="!boolean">xxxxx</ion-nav-title>
  • go to this page, and go back to parent view, this message will occur

here is the error message:

TypeError: Cannot read property '$destroy' of undefined
    at Object.IonicModule.controller.self.createHeaderBar.headerBarInstance.removeItem (ionic.bundle.js:47770)
    at Object.IonicModule.controller.self.createHeaderBar.headerBarInstance.setItem (ionic.bundle.js:47746)
    at ionic.bundle.js:47890
    at forEach (ionic.bundle.js:9150)
    at IonicModule.controller.self.update (ionic.bundle.js:47889)
    at IonicModule.controller.self.beforeEnter (ionic.bundle.js:48315)
    at IonicModule.controller.self.beforeEnter (ionic.bundle.js:50259)
    at Scope.$get.Scope.$emit (ionic.bundle.js:23474)
    at Object.IonicModule.factory.ionicViewSwitcher.create.switcher.emit (ionic.bundle.js:46814)
    at Object.IonicModule.factory.ionicViewSwitcher.create.switcher.transition (ionic.bundle.js:46672)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.