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

fix(scope): do not remove the $listeners properties on $destroy #6908

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

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 #6897

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6908)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

if (hasOwnProperty.call(this, prop)) {
if (hasOwnProperty.call(this, prop) && prop !== '$$destroyed' &&
// Avoid removing the listener properties as this causes many issues like #6897
prop !== '$$listeners' && prop !== '$$listenerCount') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that keeping these properties around will cause the v8 leak to be still present.

Copy link
Contributor

Choose a reason for hiding this comment

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

For ie8 compatibility (which I know is no longer being officially supported), In my code base I have modified this to have

Object.prototype.hasOwnProperty.call(this,prop)

instead of

hasOwnProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

why wouldn't hasOwnProperty.call work on IE8?

Copy link
Contributor

Choose a reason for hiding this comment

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

hasOwnProperty is just:

var hasOwnProperty = Object.prototype.hasOwnProperty;

@lgalfaso
Copy link
Contributor Author

@IgorMinar I had an alternative patch that checked if this was a destroyed scope in several methods. If you think that approach would be best, then I can post the patch

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
@lgalfaso
Copy link
Contributor Author

6ecf4e7 is an alternative patch that fixes this issue while removing the $$listeners and $$listenersCount properties

@IgorMinar IgorMinar self-assigned this Apr 1, 2014
@IgorMinar IgorMinar added this to the 1.3.0-beta.5 milestone Apr 1, 2014
@lgalfaso
Copy link
Contributor Author

lgalfaso commented Apr 1, 2014

The patch that caused the issue that this PR is trying to fix was reverted. This is, this PR is no longer valid

@lgalfaso lgalfaso closed this Apr 1, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decrementListenerCount: TypeError: Cannot read property '$destroy' of null
4 participants