Skip to content

Conversation

@roomote
Copy link
Collaborator

@roomote roomote commented Jun 18, 2025

Summary

This PR fixes issue #4822 where Chrome helper processes were not being properly cleaned up when remote browser connection was enabled but no remote browser was available.

Problem

When remote browser connection is enabled in settings but no remote browser is actually available, the code would:

  1. Try to connect to a remote browser
  2. Fail and fall back to launching a local browser
  3. But during cleanup, still try to disconnect() instead of close() the browser
  4. This left Chrome helper processes running

Solution

  • Added isUsingRemoteBrowser flag to track the actual connection type (not just the setting)
  • Modified closeBrowser() to use the actual connection type instead of just checking the setting
  • Ensure resetBrowserState() is always called to properly clean up all state
  • Added comprehensive tests covering all browser connection scenarios

Testing

Added unit tests that verify:

  • Local browser is properly closed when remote connection is enabled but falls back to local
  • Remote browser is properly disconnected when actually connected to remote
  • Local browser is properly closed when remote connection is disabled

All tests pass and demonstrate the fix working correctly.

Changes

  • src/services/browser/BrowserSession.ts: Added connection tracking and fixed cleanup logic
  • src/services/browser/__tests__/BrowserSession.test.ts: Added comprehensive test coverage

Fixes #4822


Important

Fixes browser cleanup logic in BrowserSession by tracking actual connection type and updating tests.

  • Behavior:
    • Adds isUsingRemoteBrowser flag in BrowserSession to track actual connection type.
    • Modifies closeBrowser() in BrowserSession to use isUsingRemoteBrowser for cleanup logic.
    • Ensures resetBrowserState() is always called for proper cleanup.
  • Testing:
    • Adds tests in BrowserSession.test.ts to verify local browser closes when remote fallback is used.
    • Tests remote browser disconnects when connected, and local closes when remote is disabled.
  • Files:
    • BrowserSession.ts: Connection tracking and cleanup logic.
    • BrowserSession.test.ts: Comprehensive test coverage for connection scenarios.

This description was created by Ellipsis for d422915. You can customize this summary. It will automatically update as commits are pushed.

…ed but no remote browser available

- Add isUsingRemoteBrowser flag to track actual connection type vs setting
- Fix closeBrowser() to use actual connection type instead of just the setting
- Ensure resetBrowserState() is always called to properly clean up state
- Add comprehensive tests for all browser connection scenarios

The issue was that when remote browser connection was enabled but no remote browser was available, the code would fall back to launching a local browser but still try to disconnect() instead of close() during cleanup, leaving Chrome helper processes running.
@roomote roomote requested review from cte, jr and mrubens as code owners June 18, 2025 10:24
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Jun 18, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 18, 2025
@daniel-lxs
Copy link
Member

Closing in favor of #4823, while this PR has some good ideas the issue author wants to fix this issue.

@daniel-lxs daniel-lxs closed this Jun 18, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 18, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Jun 18, 2025
@roomote roomote deleted the fix-4822 branch June 19, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

When remote browser connection is used, but no remote browser is available, browser does not get closed

4 participants