fix(page): collect DOM elements retained by locator hit-target interceptor#41575
fix(page): collect DOM elements retained by locator hit-target interceptor#41575Zelys-DFKH wants to merge 9 commits into
Conversation
…f detached elements
After a locator action, Blink's native input pipeline retains the last hit-tested DOM element as a C++ GC root that HeapProfiler.collectGarbage cannot see. Dispatching a no-op mouse move off-viewport (-1, -1) clears that reference before V8 GC runs, allowing detached elements to be collected. Fixes: microsoft#41462
|
|
||
| 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 }); |
There was a problem hiding this comment.
I don't think we can explain this line either to ourselves or to others.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What I meant was that it should be fixed in Chrome. We can accept clearing the field part though
There was a problem hiding this comment.
Dug into this a bit more. BoundaryEventDispatchTracksNodeRemoval is stable (shipped, public: true in runtime_enabled_features.json5) and already retargets element_under_mouse_ via MouseEventManager::NodeWillBeRemoved when a hovered node is removed. So the field the mouse-move hack is actually clearing isn't element_under_mouse_ — it's something else, likely tied to the CDP Input.dispatchMouseEvent path itself rather than the DOM-level hover tracking.
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 fixme for the known Firefox gap are independent of this line and ready to land. I'll follow up with the Chromium-side fix once I know what to target.
| const stop = () => { | ||
| if (this._hitTargetInterceptor === listener) | ||
| this._hitTargetInterceptor = undefined; | ||
| listener = undefined; |
…_under_mouse_ Blink's EventHandler caches the last hit-tested node for hover/mouseout tracking and only refreshes it on the next pointer event, so a detached element stays pinned by that native reference until a synthetic mouse move forces a fresh hit-test. Verified by removing the workaround (test fails ~70% of repeats), doubling collectGarbage (still fails), and dispatching an unrelated CDP command instead of a mouse move (still fails) -- only a new pointer event releases the reference, at any coordinate.
The previous comment named a specific private EventHandler field (node_under_mouse_) on the strength of one indirect web fetch, which overclaims precision the test evidence doesn't support -- the repeated test runs only show that a mouseMoved event before collectGarbage is sufficient to release the reference, not which exact internal field does it. Rephrased to describe the observed behavior instead of naming Blink internals.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The regression test added for the hit-target interceptor detach fix passes on Chromium but fails consistently on Firefox: the detached element is not released by Firefox's interceptor. That fix is out of scope here, so mark it fixme until addressed separately.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"2 failed 7758 passed, 1249 skipped Merge workflow run. |
Test results for "tests 1"4 flaky49521 passed, 1168 skipped Merge workflow run. |
3 participants
The hit-target interceptor's closure in injectedScript.ts held a direct reference to the target element, so releasing it there is half the fix. The other half is less obvious: Blink's native input pipeline retains the last hit-tested element as a C++ GC root that HeapProfiler.collectGarbage doesn't see, so dispatching a no-op mouse move off-viewport before triggering GC clears that root and lets V8 actually collect the element.
Firefox shows the same symptom from a different cause I couldn't verify with confidence, so it's left out of this PR.
Fixes #41462
Credit: @apocalyp0sys confirmed the Chromium leak and that a CDP mouse event dispatch clears it.