-
Notifications
You must be signed in to change notification settings - Fork 6.1k
fix(page): collect DOM elements retained by locator hit-target interceptor #41575
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
Changes from 2 commits
b58a32b
4c98845
ce12be5
62c60e1
94e13b0
ab382d2
2895b03
3149c13
dff3919
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 |
|---|---|---|
|
|
@@ -221,6 +221,8 @@ export class CRPage implements PageDelegate { | |
| } | ||
|
|
||
| async requestGC(): Promise<void> { | ||
| // A no-op mouse move off-viewport clears Blink's native hit-target state, which retains the last-targeted element as a GC root invisible to V8. | ||
| await this._mainFrameSession._client.send('Input.dispatchMouseEvent', { type: 'mouseMoved', x: -1, y: -1 }); | ||
|
Member
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. I don't think we can explain this line either to ourselves or to others.
Contributor
Author
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. You're right that the original comment didn't hold up. I dug into it: removing the mouse move, doubling collectGarbage, and swapping in an unrelated CDP command all still leave the test failing most of the time; only dispatching a new mouseMoved event (any coordinate) reliably releases the reference. That points to Chromium's own hover/mouseout tracking holding onto the last hit-tested node until the next pointer event, which HeapProfiler.collectGarbage can't see. Rewrote the comment to describe that behavior instead of naming Blink internals I can't fully verify from here.
Member
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. What I meant was that it should be fixed in Chrome. We can accept clearing the field part though
Contributor
Author
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. Dug into this a bit more. Can you point at which field you mean? If there's a known direct way to clear it I'll patch that instead of the synthetic event. If not, I'd rather file a proper Chromium bug than carry something I can't explain. In the meantime, would you take this split? The regression test, the Firefox closure fix, and the |
||
| await this._mainFrameSession._client.send('HeapProfiler.collectGarbage'); | ||
| } | ||
|
|
||
|
|
||
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.
looks good