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

[BUGFIX release] Fix observer leaks #18767

Merged
merged 1 commit into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions packages/@ember/-internals/meta/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1 @@
export {
counters,
deleteMeta,
Meta,
meta,
MetaCounters,
peekMeta,
setMeta,
UNDEFINED,
} from './lib/meta';
export { counters, Meta, meta, MetaCounters, peekMeta, setMeta, UNDEFINED } from './lib/meta';
32 changes: 4 additions & 28 deletions packages/@ember/-internals/meta/lib/meta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ export class Meta {
}

destroy() {
if (DEBUG) {
counters!.deleteCalls++;
}

if (this.isMetaDestroyed()) {
return;
}
Expand Down Expand Up @@ -647,34 +651,6 @@ export function peekMeta(obj: object): Meta | null {
return null;
}

/**
Tears down the meta on an object so that it can be garbage collected.
Multiple calls will have no effect.

@method deleteMeta
@for Ember
@param {Object} obj the object to destroy
@return {void}
@private
*/
export function deleteMeta(obj: object) {
assert('Cannot call `deleteMeta` on null', obj !== null);
assert('Cannot call `deleteMeta` on undefined', obj !== undefined);
assert(
`Cannot call \`deleteMeta\` on ${typeof obj}`,
typeof obj === 'object' || typeof obj === 'function'
);

if (DEBUG) {
counters!.deleteCalls++;
}

let meta = peekMeta(obj);
if (meta !== null) {
meta.destroy();
}
}

/**
Retrieves the meta hash for an object. If `writable` is true ensures the
hash is writable for this object as well.
Expand Down
10 changes: 9 additions & 1 deletion packages/@ember/-internals/metal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,15 @@ export { default as getProperties } from './lib/get_properties';
export { default as setProperties } from './lib/set_properties';
export { default as expandProperties } from './lib/expand_properties';

export { addObserver, activateObserver, removeObserver, flushAsyncObservers } from './lib/observer';
export { destroy } from './lib/destroy';
export {
ASYNC_OBSERVERS,
SYNC_OBSERVERS,
addObserver,
activateObserver,
removeObserver,
flushAsyncObservers,
} from './lib/observer';
export { Mixin, aliasMethod, mixin, observer, applyMixin } from './lib/mixin';
export { default as inject, DEBUG_INJECTION_FUNCTIONS } from './lib/injected_property';
export { tagForProperty, tagForObject, markObjectAsDirty, CUSTOM_TAG_FOR } from './lib/tags';
Expand Down
37 changes: 37 additions & 0 deletions packages/@ember/-internals/metal/lib/destroy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Meta, peekMeta } from '@ember/-internals/meta/lib/meta';
import { assert } from '@ember/debug';
import { schedule } from '@ember/runloop';
import { destroyObservers } from './observer';

/**
Enqueues finalization on an object so that it can be garbage collected.
Multiple calls will have no effect.

@method destroy
@for Ember
@param {Object} obj the object to destroy
@return {boolean} true if the object went from not destroying to destroying.
@private
*/
export function destroy(obj: object): boolean {
krisselden marked this conversation as resolved.
Show resolved Hide resolved
assert('Cannot call `destroy` on null', obj !== null);
assert('Cannot call `destroy` on undefined', obj !== undefined);
assert(
`Cannot call \`destroy\` on ${typeof obj}`,
typeof obj === 'object' || typeof obj === 'function'
);

const m = peekMeta(obj);
if (m === null || m.isSourceDestroying()) {
return false;
}
m.setSourceDestroying();
destroyObservers(obj);
schedule('destroy', m, finalize);
return true;
}

function finalize(this: Meta) {
this.setSourceDestroyed();
this.destroy();
}
3 changes: 1 addition & 2 deletions packages/@ember/-internals/metal/lib/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,7 @@ export function sendEvent(
) {
if (actions === undefined) {
let meta = _meta === undefined ? peekMeta(obj) : _meta;
actions =
typeof meta === 'object' && meta !== null ? meta.matchingListeners(eventName) : undefined;
actions = meta !== null ? meta.matchingListeners(eventName) : undefined;
}

if (actions === undefined || actions.length === 0) {
Expand Down
20 changes: 13 additions & 7 deletions packages/@ember/-internals/metal/lib/observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ interface ActiveObserver {
}

const SYNC_DEFAULT = !ENV._DEFAULT_ASYNC_OBSERVERS;
const SYNC_OBSERVERS: Map<object, Map<string, ActiveObserver>> = new Map();
const ASYNC_OBSERVERS: Map<object, Map<string, ActiveObserver>> = new Map();
export const SYNC_OBSERVERS: Map<object, Map<string, ActiveObserver>> = new Map();
export const ASYNC_OBSERVERS: Map<object, Map<string, ActiveObserver>> = new Map();

/**
@module @ember/object
Expand Down Expand Up @@ -153,15 +153,16 @@ export function revalidateObservers(target: object) {
let lastKnownRevision = 0;

export function flushAsyncObservers(shouldSchedule = true) {
if (lastKnownRevision === value(CURRENT_TAG)) {
let currentRevision = value(CURRENT_TAG);
if (lastKnownRevision === currentRevision) {
return;
}

lastKnownRevision = value(CURRENT_TAG);
lastKnownRevision = currentRevision;

ASYNC_OBSERVERS.forEach((activeObservers, target) => {
let meta = peekMeta(target);

// if observer target is destroyed remove observers
if (meta && (meta.isSourceDestroying() || meta.isMetaDestroyed())) {
ASYNC_OBSERVERS.delete(target);
return;
Expand All @@ -171,7 +172,7 @@ export function flushAsyncObservers(shouldSchedule = true) {
if (!validate(observer.tag, observer.lastRevision)) {
let sendObserver = () => {
try {
sendEvent(target, eventName, [target, observer.path]);
sendEvent(target, eventName, [target, observer.path], undefined, meta);
} finally {
observer.tag = combine(getChainTagsForKey(target, observer.path));
observer.lastRevision = value(observer.tag);
Expand Down Expand Up @@ -205,7 +206,7 @@ export function flushSyncObservers() {
if (!observer.suspended && !validate(observer.tag, observer.lastRevision)) {
try {
observer.suspended = true;
sendEvent(target, eventName, [target, observer.path]);
sendEvent(target, eventName, [target, observer.path], undefined, meta);
} finally {
observer.tag = combine(getChainTagsForKey(target, observer.path));
observer.lastRevision = value(observer.tag);
Expand All @@ -229,3 +230,8 @@ export function setObserverSuspended(target: object, property: string, suspended
observer.suspended = suspended;
}
}

export function destroyObservers(target: object) {
if (SYNC_OBSERVERS.size > 0) SYNC_OBSERVERS.delete(target);
if (ASYNC_OBSERVERS.size > 0) ASYNC_OBSERVERS.delete(target);
}
5 changes: 5 additions & 0 deletions packages/@ember/-internals/metal/tests/accessors/get_test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ENV } from '@ember/-internals/environment';
import { Object as EmberObject } from '@ember/-internals/runtime';
import { destroy } from '@ember/-internals/metal';
import { get, set, getWithDefault, Mixin, observer, computed } from '../..';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
import { run } from '@ember/runloop';
Expand Down Expand Up @@ -196,6 +197,8 @@ moduleFor(
'foo',
'should return the set value, not false'
);

run(() => destroy(baseObject));
}
}
);
Expand Down Expand Up @@ -332,6 +335,8 @@ moduleFor(
'foo',
'should return the set value, not false'
);

run(() => destroy(baseObject));
}

['@test should respect prototypical inheritance when subclasses override CPs'](assert) {
Expand Down
7 changes: 6 additions & 1 deletion packages/@ember/-internals/metal/tests/alias_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
alias,
computed,
defineProperty,
destroy,
get,
set,
addObserver,
Expand All @@ -27,7 +28,11 @@ moduleFor(
}

afterEach() {
obj = null;
if (obj !== undefined) {
destroy(obj);
obj = undefined;
return runLoopSettled();
}
}

['@test should proxy get to alt key'](assert) {
Expand Down
Loading