Repository navigation
fix: release right-clicked nodes after the context-menu event - #55502
Conversation
Electron emits 'context-menu' and leaves the menu to the app, so nothing calls WebContents::NotifyContextMenuClosed the way Chrome's RenderViewContextMenu does when its menu closes. Blink's ContextMenuController then keeps its hit-test result, the right-clicked node and every ancestor, until the next context menu. A page that removes those nodes can't collect them, or anything they reference. Notify the renderer once the event has been emitted. Electron shows its own menus and never sends the renderer a custom-item command, so nothing reads that state afterwards. Assisted-By: Claude Opus 5.5, Claude Code Co-authored-by: Eric Bidelman <238208+ebidel@users.noreply.github.com>
When a right-click lands on selected text that isn't editable, Blink starts generating a link-to-text selector, and its TextFragmentSelectorGenerator keeps the selection's nodes until the browser requests the selector or cancels. Chrome's LinkToTextMenuObserver requests it from its menu. Electron never does, so the nodes stay alive until the next right-click on text. Cancel the generation once the event has been emitted. Assisted-By: Claude Opus 5.5, Claude Code Co-authored-by: Eric Bidelman <238208+ebidel@users.noreply.github.com>
Blink also starts an AnnotationAgentGenerator over a right-clicked selection, and AnnotationAgentContainer has no browser-side cancel, so cancelling only the TextFragmentHandler generator still leaves the selection's nodes alive. Drop that cancel and its spec; releasing the selection needs a Blink change. Also drop the spec's focus step: the right mousedown already makes the frame the browser's focused frame, which is what NotifyContextMenuClosed checks. Assisted-By: Claude Opus 5.5, Claude Code Co-authored-by: Eric Bidelman <238208+ebidel@users.noreply.github.com>
| auto weak_this = WeakRef(); | ||
| Emit("context-menu", event_data); | ||
|
|
||
| if (!weak_this->Get() || !web_contents()) | ||
| return; | ||
|
|
||
| // Apps show their own menu, or none, in response to the event above, so | ||
| // nothing reports the renderer's context menu closed the way Chrome's | ||
| // RenderViewContextMenu does. Until it is, Blink's ContextMenuController | ||
| // keeps the right-clicked node and its ancestors alive, even after the page | ||
| // removes them. | ||
| web_contents()->NotifyContextMenuClosed(GURL()); |
There was a problem hiding this comment.
🟡 (optional) A DevTools custom context menu can stop working: an earlier right-click's late callback can close the menu, so picking an item does nothing. The clipboard read before OnReadAvailableTypes is async, and this path now calls NotifyContextMenuClosed(GURL()) whenever it finishes, even if a DevToolsContextMenu opened meanwhile. Fix: never close a menu this request does not own. Skip the call when web_contents()->IsShowingContextMenu() is true, which DevToolsContextMenu::RunMenuAt sets at devtools_context_menu.cc:65. Can you confirm content keeps a single context_menu_client_ per WebContents? I can't see it from this standalone checkout.
Why this was flagged
The trigger is a WebContents that hosts a DevTools frontend directly (webContents.setDevToolsWebContents()). An ordinary right-click there reaches HandleContextMenu, which starts ReadAvailableTypes at electron_api_web_contents.cc:2817-2820 and returns. If a custom-item request arrives during the wait, electron_api_web_contents.cc:2809-2813 creates a DevToolsContextMenu. That menu calls SetShowingContextMenu(true) (devtools_context_menu.cc:65). When the first read finishes, line 2851 calls NotifyContextMenuClosed(GURL()) on the same web_contents() with no IsShowingContextMenu() check, while the DevTools menu is still open. If content stores one ContextMenuClient per WebContents, that call closes the DevTools menu's client and resets the frontend's pending menu. The user's later pick is then dropped. On the base branch only DevToolsContextMenu ever called NotifyContextMenuClosed, so this race did not exist. SetShowingContextMenu(true) does not protect against it: content uses that flag to block new ShowContextMenu requests, and this callback was already queued.
Verification: The new code at electron_api_web_contents.cc:2851 calls web_contents()->NotifyContextMenuClosed(GURL()); with no condition. DevToolsContextMenu::RunMenuAt calls SetShowingContextMenu(true) (devtools_context_menu.cc:65) and is still waiting for a selection, so the stale callback reports a menu closed that it does not own. The base branch never made this call in OnReadAvailableTypes.
|
Release Notes Persisted
|
Backport of #55488
See that PR for details.
Notes: Fixed a leak where right-clicked elements stayed alive after removal.