fix(mcp): rebind page-registered WebMCP tools when the tab or frame changes - #42821
AbNoZ&v02 (Abnoz01) wants to merge 3 commits into
Conversation
…hanges The dynamic tool list was only replaced when the tool schemas changed, so handlers stayed bound to a detached frame after a reload, or to the previous tab when two tabs exposed the same tools. Fixes microsoft#42816
| const tools = this._currentTab?.webmcpTools()?.tools.map(tool => tool.mcpTool) ?? []; | ||
| // Handlers are bound to the tab and frame they were listed from, so always take | ||
| // the fresh ones. Only the listChanged notification depends on the schemas. | ||
| this._webmcpTools = tools; |
There was a problem hiding this comment.
A cleaner variant is to drop Context._webmcpTools and have currentWebMCPTools() read from _currentTab?.webmcpTools().
Dmitry Gozman (@dgozman) what do you think?
| }); | ||
| }); | ||
|
|
||
| test('a tool with the same name in two tabs runs in the current tab', async ({ startClient, server, mcpBrowser }) => { |
There was a problem hiding this comment.
just one test above is sufficient, drop this
|
|
||
| maybeNotifyWebMCPToolsChanged() { | ||
| const tools = this._currentTab?.webmcpTools()?.tools.map(tool => tool.mcpTool) ?? []; | ||
| // Handlers are bound to the tab and frame they were listed from, so always take |
There was a problem hiding this comment.
Shorten the comment or drop it entirely: "// Handlers are bound to a tab and frame, always take the fresh ones."
|
|
||
| maybeNotifyWebMCPToolsChanged() { | ||
| const tools = this._currentTab?.webmcpTools()?.tools.map(tool => tool.mcpTool) ?? []; | ||
| // Handlers are bound to the tab and frame they were listed from, so always take |
There was a problem hiding this comment.
Shorten the comment or drop it entirely: "// Handlers are bound to a tab and frame, always take the fresh ones."
This comment has been minimized.
This comment has been minimized.
Address review: drop the cached tool list on Context, keep only the schema signature for listChanged notifications, drop the redundant two-tab test and skip the iframe test on Firefox, which does not list tools registered in iframes.
|
Pushed a follow-up addressing the comments:
|
This comment has been minimized.
This comment has been minimized.
|
AbNoZ&v02 (@Abnoz01) |
|
Took a look. The same test failed the same way on #42818 (injected regex change only), and the CI results db shows all three iframe tests in I merged main into the branch to get a fresh run. Happy to send a separate PR that waits for the iframe's tools to show up before asserting, if that helps. |
Test results for "MCP"1 failed 8698 passed, 1480 skipped Merge workflow run. |
This branch has not been deployed
3 participants
Summary
maybeNotifyWebMCPToolsChangedkept the previous tool definitions whenever the schema signature was unchanged. Each definition's handler is a closure over the tab and frame it was listed from, so after a reload an iframe tool failed with "Frame was detached", and with two tabs exposing the same tools the call ran in the previous tab.listChangednotification on the schema signature. Add tests for both cases.Fixes #42816