-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
un-monkey-patch OpenElementStack to avoid leaks #2831
Conversation
A monkey-patch as modified by commit 75a921e maintains a reference to the most recent JSDOMParse5Adapter (and all its options) through the push/pop closures. Because the monkey-patches are never cleaned up, there is always a reference to the most recent JSDOMParse5Adapter and its related objects. This fixes jestjs/jest#9507 This may fix jsdom#2757
I just noticed #2825 beat me to the punch by a few days. I'll happily close this if y'all maintainers prefer the other approach. I assume the unintentional test fix is due to how this code stacks parser adapters rather than keeping and overwriting one reference. |
The monkey-patch for maintaining the open element stack, as modified by commit 75a921e, maintained a reference to the most recent JSDOMParse5Adapter (and all its options) through the push/pop closures. This means there was always a reference from the rooted OpenElementStack.prototype.{push,pop} functions to the most recent JSDOMParse5Adapter and its related objects. This commit instead uses the this.treeAdapter pointer, which already exists in parse5. Doing so also allows us to move the monkeypatch back into the top level, instead of establishing it on construction of each tree adapter. Closes #2831. Closes #2825. Fixes jestjs/jest#9507.
// Horrible monkey-patch to implement https://github.com/inikulin/parse5/issues/237 | ||
OpenElementStack.prototype.push = function (...args) { | ||
OpenElementStackOriginalPush.apply(this, args); | ||
adapterStack[adapterStack.length - 1]._currentElement = this.current; |
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.
This should have a check for adapterStack
not empty, just in case parse5 is used elsewhere not in jsdom.
The monkey-patch for maintaining the open element stack, as modified by commit 75a921e, maintained a reference to the most recent JSDOMParse5Adapter through the push/pop closures. This means there was always a reference from the rooted OpenElementStack.prototype.{push,pop} functions to the most recent JSDOMParse5Adapter and its related objects. Since one of the related objects was an element, which has a pointer to a document and window, essentially the whole JSDOM would be retained. This commit instead uses the this.treeAdapter pointer, which already exists in parse5. Doing so also allows us to move the monkeypatch back into the top level, instead of establishing it on construction of each tree adapter. Closes #2831. Closes #2825. Helps with the root cause of jestjs/jest#9507.
A monkey-patch as modified by commit 75a921e
maintains a reference to the most recent JSDOMParse5Adapter (and all its
options) through the push/pop closures. Because the monkey-patches are
never cleaned up, there is always a reference to the most recent
JSDOMParse5Adapter and its related objects.
This fixes jestjs/jest#9507
This may fix #2757