-
Notifications
You must be signed in to change notification settings - Fork 392
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
fix: mutationobserver memory leak - use weakmap for bookkeeping #1423
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,13 @@ | |
*/ | ||
import { | ||
ArrayIndexOf, | ||
ArrayReduce, | ||
ArrayPush, | ||
ArrayReduce, | ||
ArraySplice, | ||
create, | ||
defineProperty, | ||
defineProperties, | ||
forEach, | ||
isUndefined, | ||
isNull, | ||
} from '../../shared/language'; | ||
|
@@ -28,6 +30,8 @@ const { | |
const wrapperLookupField = '$$lwcObserverCallbackWrapper$$'; | ||
const observerLookupField = '$$lwcNodeObservers$$'; | ||
|
||
const observerToNodesMap: WeakMap<MutationObserver, Array<Node>> = new WeakMap(); | ||
|
||
/** | ||
* Retarget the mutation record's target value to its shadowRoot | ||
* @param {MutationRecord} originalRecord | ||
|
@@ -199,6 +203,21 @@ function PatchedMutationObserver( | |
|
||
function patchedDisconnect(this: MutationObserver): void { | ||
originalDisconnect.call(this); | ||
|
||
// Clear the node to observer reference which is a strong references | ||
const observedNodes = observerToNodesMap.get(this); | ||
if (!isUndefined(observedNodes)) { | ||
forEach.call(observedNodes, observedNode => { | ||
const observers = observedNode[observerLookupField]; | ||
if (!isUndefined(observers)) { | ||
const index = ArrayIndexOf.call(observers, this); | ||
if (index !== -1) { | ||
ArraySplice.call(observers, index, 1); | ||
} | ||
} | ||
}); | ||
observedNodes.length = 0; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -216,11 +235,26 @@ function patchedObserve( | |
if (isUndefined(target[observerLookupField])) { | ||
defineProperty(target, observerLookupField, { value: [] }); | ||
} | ||
ArrayPush.call(target[observerLookupField], this); | ||
// Same observer trying to observe the same node | ||
if (ArrayIndexOf.call(target[observerLookupField], this) === -1) { | ||
ArrayPush.call(target[observerLookupField], this); | ||
} // else There is more bookkeeping to do here https://dom.spec.whatwg.org/#dom-mutationobserver-observe Step #7 | ||
|
||
// If the target is a SyntheticShadowRoot, observe the host since the shadowRoot is an empty documentFragment | ||
if (target instanceof SyntheticShadowRoot) { | ||
target = (target as ShadowRoot).host; | ||
} | ||
|
||
// maintain a list of all nodes observed by this observer | ||
if (observerToNodesMap.has(this)) { | ||
const observedNodes = observerToNodesMap.get(this)!; | ||
if (ArrayIndexOf.call(observedNodes, target) === -1) { | ||
ArrayPush.call(observedNodes, target); | ||
} | ||
} else { | ||
observerToNodesMap.set(this, [target]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. set from lang, although we are not doing that today... we should probably start doing it at some point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its a polyfilled feature in IE11, wasn't sure if was okay to cache these. Let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it should be ok. they all run before everything else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets open a seprate issue for this in general. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
return originalObserve.call(this, target, options); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we use
ArrayPush.call()
but notWeakMapHas.call()
? I.e. we cache the Array prototype functions but not the WeakMap ones?