Fix launcher sandbox defaults

Author: pirate

go/modcdp/launcher/LocalBrowserLauncher.go

@@ -102,7 +102,11 @@ func (l *LocalBrowserLauncher) Launch(options LaunchOptions) (*LaunchedBrowser,
 	if headless {
 		args = append(args, "--headless=new")
 	}
-	if options.Sandbox == nil || !*options.Sandbox {
+	sandbox := runtime.GOOS != "linux"
+	if options.Sandbox != nil {
+		sandbox = *options.Sandbox
+	}
+	if !sandbox {
 		args = append(args, "--no-sandbox")
 	}
 	args = append(args, options.Args...)

go/modcdp/launcher/LocalBrowserLauncher_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"os"
+	"runtime"
 	"strconv"
 	"strings"
 	"testing"
@@ -23,7 +24,7 @@ func TestLocalBrowserLauncherClassHelpersMatchLocalLauncherSurface(t *testing.T)
 	}
 }
 
-func TestLocalBrowserLauncherLaunchesRealBrowserAndSpeaksCDP(t *testing.T) {
+func TestLocalBrowserLauncherLaunchesRealBrowserOverChosenCDPPortAndHonorsLaunchOptions(t *testing.T) {
 	headless := true
 	profileDir := t.TempDir()
 	port, err := freePort()
@@ -101,6 +102,36 @@ func TestLocalBrowserLauncherLaunchesRealBrowserAndSpeaksCDP(t *testing.T) {
 	if response.Result.ProtocolVersion == "" {
 		t.Fatal("expected protocolVersion")
 	}
+	if err := wsutil.WriteClientText(conn, []byte(`{"id":2,"method":"SystemInfo.getInfo","params":{}}`)); err != nil {
+		t.Fatal(err)
+	}
+	systemInfoBody, err := wsutil.ReadServerText(conn)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var systemInfoResponse struct {
+		ID     int `json:"id"`
+		Result struct {
+			CommandLine string `json:"commandLine"`
+		} `json:"result"`
+	}
+	if err := json.Unmarshal(systemInfoBody, &systemInfoResponse); err != nil {
+		t.Fatal(err)
+	}
+	if systemInfoResponse.ID != 2 {
+		t.Fatalf("unexpected SystemInfo.getInfo response id %d", systemInfoResponse.ID)
+	}
+	command := systemInfoResponse.Result.CommandLine
+	if !strings.Contains(command, "--window-size=900,700") {
+		t.Fatalf("expected browser command to include --window-size=900,700: %s", command)
+	}
+	if runtime.GOOS == "linux" {
+		if !strings.Contains(command, "--no-sandbox") {
+			t.Fatalf("expected Linux browser command to include --no-sandbox: %s", command)
+		}
+	} else if strings.Contains(command, "--no-sandbox") {
+		t.Fatalf("expected browser command not to include --no-sandbox: %s", command)
+	}
 }
 
 func TestLocalBrowserLauncherLaunchesRealBrowserOverRemoteDebuggingPipe(t *testing.T) {

js/src/launcher/LocalBrowserLauncher.ts

@@ -261,7 +261,7 @@ export class LocalBrowserLauncher extends BrowserLauncher {
       port,
       user_data_dir,
       headless = process.platform === "linux" && !process.env.DISPLAY,
-      sandbox = false,
+      sandbox = process.platform !== "linux",
       args = [],
       extra_args = [],
       remote_debugging = "port",

js/test/test.LocalBrowserLauncher.ts

@@ -60,9 +60,20 @@ describe("LocalBrowserLauncher", () => {
         );
         if (process.platform === "linux") {
           expect((chrome.proc as { spawnargs?: string[] }).spawnargs ?? []).toContain("--no-sandbox");
+        } else {
+          expect((chrome.proc as { spawnargs?: string[] }).spawnargs ?? []).not.toContain("--no-sandbox");
         }
         await expect(stat(userDataDir)).resolves.toBeTruthy();
         cdp = await CdpSocket.connect(chrome.cdp_url!);
+        const systemInfo = await cdp.send("SystemInfo.getInfo");
+        const commandLine = systemInfo.commandLine;
+        expect(commandLine).toEqual(expect.any(String));
+        expect(commandLine).toContain("--window-size=900,700");
+        if (process.platform === "linux") {
+          expect(commandLine).toContain("--no-sandbox");
+        } else {
+          expect(commandLine).not.toContain("--no-sandbox");
+        }
         await expectCdpBrowserSurface(cdp);
       } finally {
         await cdp?.close();

package.json

@@ -1,13 +1,15 @@
 {
   "name": "modcdp",
-  "version": "0.0.16",
+  "version": "0.0.17",
   "repository": {
     "type": "git",
     "url": "git+https://github.com/pirate/modcdp.git"
   },
   "files": [
     "js/scripts/**/*.mjs",
     "dist/**/*",
+    "dist/extension/**/*",
+    "dist/js/**/*",
     "dist/extension.zip",
     "extension/src/**/*.ts",
     "extension/src/pages/**/*",
@@ -132,7 +134,7 @@
     "clean": "rm -rf dist",
     "build": "pnpm run build:package && pnpm run build:extension-assets && pnpm run build:python-readme",
     "build:clean": "pnpm run clean && pnpm run build",
-    "build:package": "tsc -p tsconfig.json && rm -rf dist/js/test",
+    "build:package": "tsc -p tsconfig.json && rm -rf dist/js/test && rm -f dist/.gitignore",
     "build:extension-assets": "node js/scripts/build-extension-assets.mjs",
     "build:python-readme": "mkdir -p python && if [ README.md -ef python/README.md ]; then :; else cp README.md python/README.md; fi",
     "typecheck": "tsc -p tsconfig.json --noEmit",

python/modcdp/launcher/LocalBrowserLauncher.py

@@ -75,7 +75,8 @@ def launch(self, options: BrowserLaunchOptions | None = None) -> LaunchedBrowser
         default_headless = sys.platform.startswith("linux") and not os.environ.get("DISPLAY")
         if merged.get("headless", default_headless):
             args.append("--headless=new")
-        if merged.get("sandbox", False) is False:
+        default_sandbox = not sys.platform.startswith("linux")
+        if merged.get("sandbox", default_sandbox) is False:
             args.append("--no-sandbox")
         args.extend(list(merged.get("args") or []))
         args.extend(list(merged.get("extra_args") or []))
@@ -85,10 +86,8 @@ def launch(self, options: BrowserLaunchOptions | None = None) -> LaunchedBrowser
             child_read, parent_write = os.pipe()
             parent_read = _move_fd_if_needed(parent_read, {3, 4})
             parent_write = _move_fd_if_needed(parent_write, {3, 4})
-            if child_read == 4:
-                child_read = _move_fd_if_needed(child_read, {4})
-            if child_write == 3:
-                child_write = _move_fd_if_needed(child_write, {3})
+            child_read = _move_fd_to(child_read, 3)
+            child_write = _move_fd_to(child_write, 4)
             process = _spawn_chrome_with_pipe_fds(executable_path, args, child_read, child_write)
             os.close(child_read)
             os.close(child_write)
@@ -224,61 +223,23 @@ def _move_fd_if_needed(fd: int, reserved: set[int]) -> int:
     return moved
 
 
-class _SpawnedProcess:
-    def __init__(self, pid: int) -> None:
-        self.pid = pid
-        self.returncode: int | None = None
-
-    def terminate(self) -> None:
-        self._signal(signal.SIGTERM)
-
-    def kill(self) -> None:
-        self._signal(signal.SIGKILL)
-
-    def wait(self, timeout: float | None = None) -> int:
-        deadline = None if timeout is None else time.monotonic() + timeout
-        while True:
-            try:
-                waited_pid, status = os.waitpid(self.pid, os.WNOHANG)
-            except ChildProcessError:
-                if self.returncode is None:
-                    self.returncode = 0
-                return self.returncode
-            if waited_pid == self.pid:
-                self.returncode = os.waitstatus_to_exitcode(status)
-                return self.returncode
-            if deadline is not None and time.monotonic() >= deadline:
-                assert timeout is not None
-                raise subprocess.TimeoutExpired([str(self.pid)], timeout)
-            time.sleep(0.05)
-
-    def _signal(self, sig: int) -> None:
-        if self.returncode is not None:
-            return
-        try:
-            os.kill(self.pid, sig)
-        except ProcessLookupError:
-            self.returncode = 0
+def _move_fd_to(fd: int, target: int) -> int:
+    if fd == target:
+        return fd
+    os.dup2(fd, target)
+    os.close(fd)
+    return target
 
 
 def _spawn_chrome_with_pipe_fds(executable_path: str, args: list[str], child_read: int, child_write: int) -> _ChromeProcess:
-    if not hasattr(os, "posix_spawn"):
-        raise RuntimeError("remote_debugging='pipe' requires os.posix_spawn support in the Python client.")
-    devnull = os.open(os.devnull, os.O_RDWR)
-    file_actions: list[tuple[int, int] | tuple[int, int, int]] = [
-        (os.POSIX_SPAWN_DUP2, devnull, 0),
-        (os.POSIX_SPAWN_DUP2, devnull, 1),
-        (os.POSIX_SPAWN_DUP2, devnull, 2),
-        (os.POSIX_SPAWN_DUP2, child_read, 3),
-        (os.POSIX_SPAWN_DUP2, child_write, 4),
-    ]
-    for fd in {devnull, child_read, child_write} - {0, 1, 2, 3, 4}:
-        file_actions.append((os.POSIX_SPAWN_CLOSE, fd))
-    try:
-        pid = os.posix_spawn(executable_path, [executable_path, *args], os.environ, file_actions=file_actions, setsid=True)
-        return _SpawnedProcess(pid)
-    finally:
-        os.close(devnull)
+    return subprocess.Popen(
+        [executable_path, *args],
+        stdin=subprocess.DEVNULL,
+        stdout=subprocess.DEVNULL,
+        stderr=subprocess.DEVNULL,
+        pass_fds=(child_read, child_write),
+        start_new_session=not sys.platform.startswith("win"),
+    )
 
 
 class _ChromeProcess(Protocol):

python/pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
 
 [project]
 name = "modcdp"
-version = "0.0.16"
+version = "0.0.17"
 description = "Python client for ModCDP."
 readme = "README.md"
 requires-python = ">=3.11"

python/tests/test_LocalBrowserLauncher.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import json
+import sys
 import tempfile
 import unittest
 from pathlib import Path
@@ -15,7 +16,7 @@ def test_class_helpers_match_ts_surface(self) -> None:
         self.assertIsInstance(LocalBrowserLauncher.findChromeBinary(), str)
         self.assertIsInstance(LocalBrowserLauncher.freePort(), int)
 
-    def test_launches_real_browser_and_speaks_cdp(self) -> None:
+    def test_launches_real_browser_over_chosen_cdp_port_and_honors_launch_options(self) -> None:
         with tempfile.TemporaryDirectory(prefix="modcdp-python-local-profile-") as user_data_dir:
             chrome = LocalBrowserLauncher(
                 {
@@ -36,6 +37,16 @@ def test_launches_real_browser_and_speaks_cdp(self) -> None:
                 self.assertEqual(version["id"], 1)
                 self.assertIn("Chrome", version["result"]["product"])
                 self.assertIsInstance(version["result"]["protocolVersion"], str)
+                ws.send(json.dumps({"id": 2, "method": "SystemInfo.getInfo", "params": {}}))
+                system_info = json.loads(ws.recv())
+                self.assertEqual(system_info["id"], 2)
+                command_line = system_info["result"]["commandLine"]
+                self.assertIsInstance(command_line, str)
+                self.assertIn("--window-size=900,700", command_line)
+                if sys.platform.startswith("linux"):
+                    self.assertIn("--no-sandbox", command_line)
+                else:
+                    self.assertNotIn("--no-sandbox", command_line)
             finally:
                 ws.close()
                 chrome["close"]()