Skip to content

execute_3p_developer_tool disposes stashed element handles it needs (regression from #2443) #2453

Description

@ch-bas

Description of the bug

Regression introduced by #2443 (explicit resource management refactor).

In McpPage.executeThirdPartyDeveloperTool, when a third-party tool returns DOM element(s), each stashed element is resolved into an ElementHandle and handed to TextSnapshot.create as extraHandles, which stores them on page.extraHandles for later stashed-uid resolution. #2443 replaced the old "dispose the previous handles" logic with a DisposableStack, but registered the new handles instead of the old ones:

// src/McpPage.ts
if (elementHandles.length) {
  using stack = new DisposableStack();
  for (const handle of elementHandles) {   // the NEW handles
    stack.use(handle);
  }
  this.textSnapshot = await TextSnapshot.create(this, {extraHandles: elementHandles}); // stores them on page.extraHandles
  response.includeSnapshot();
}   // <-- stack disposes all elementHandles here
// used again immediately:
const cdpElementIds = await Promise.all(
  elementHandles.map(async h => await h.backendNodeId()),
);

So the stashed handles are disposed at the end of the block even though (a) they are used two lines later and (b) they are retained on page.extraHandles and referenced by the snapshot's stashed uids. Net effect: after execute_3p_developer_tool returns an element, the stashed handle is dead, and the previous snapshot's handles leak (the inverse of the intended behavior).

Pre-#2443 the code disposed oldHandles = [...this.extraHandles] (the replaced set) and kept the new handles, which was correct.

Reproduction

Deterministic test (fails on main, passes with the fix below):

// a 3P tool whose execute() returns a DOM element
await page.pptrPage.evaluate(() => {
  window.__dtmcp = {
    executeTool: async () => {
      const div = document.createElement('div');
      div.id = 'stashed-el';
      document.body.appendChild(div);
      return div;
    },
  };
});
await executeThirdPartyDeveloperTool.handler(
  {params: {toolName: 'get-element', params: '{}'}, page},
  response, context,
);
// page.extraHandles[0] is disposed:
await page.extraHandles[0].evaluate(el => el.id); // throws: JSHandle is disposed!

Note: backendNodeId() does not expose the bug because Puppeteer caches it; a live-remote-object operation (evaluate) is required.

Expectation

Stashed element handles stay alive after execution so follow-up tools can resolve the stashed uids; only the replaced previous handles are disposed.

Fix: register this.extraHandles (captured before TextSnapshot.create overwrites it) in the stack instead of elementHandles. I have this plus the regression test passing locally and can open a PR.

Chrome DevTools MCP version

main @ 5b25370 (1.6.0)

Chrome version

n/a (independent of Chrome version)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions