fix: correctly terminate connection on remote browser close (#318)

playwright/src/main/java/com/microsoft/playwright/impl/BrowserContextImpl.java

@@ -404,11 +404,15 @@ class BrowserContextImpl extends ChannelOwner implements BrowserContext {
         bindingCall.call(binding);
       }
     } else if ("close".equals(event)) {
-      isClosedOrClosing = true;
-      if (browser != null) {
-        browser.contexts.remove(this);
-      }
-      listeners.notify(EventType.CLOSE, this);
+      didClose();
     }
   }
+
+  void didClose() {
+    isClosedOrClosing = true;
+    if (browser != null) {
+      browser.contexts.remove(this);
+    }
+    listeners.notify(EventType.CLOSE, this);
+  }
 }

playwright/src/main/java/com/microsoft/playwright/impl/BrowserImpl.java

@@ -38,7 +38,7 @@ import static com.microsoft.playwright.impl.Utils.convertViaJson;
 import static com.microsoft.playwright.impl.Utils.isSafeCloseError;
 
 class BrowserImpl extends ChannelOwner implements Browser {
-  final Set<BrowserContext> contexts = new HashSet<>();
+  final Set<BrowserContextImpl> contexts = new HashSet<>();
   private final ListenerCollection<EventType> listeners = new ListenerCollection<>();
   public boolean isRemote;
   private boolean isConnected = true;
@@ -65,7 +65,17 @@ class BrowserImpl extends ChannelOwner implements Browser {
   public void close() {
     withLogging("Browser.close", () -> closeImpl());
   }
+
   private void closeImpl() {
+    if (isRemote) {
+      try {
+        connection.close();
+      } catch (IOException e) {
+        throw new PlaywrightException("Failed to close browser connection", e);
+      }
+      notifyRemoteClosed();
+      return;
+    }
     try {
       sendMessage("close");
     } catch (PlaywrightException e) {
@@ -75,6 +85,17 @@ class BrowserImpl extends ChannelOwner implements Browser {
     }
   }
 
+  void notifyRemoteClosed() {
+    // Emulate all pages, contexts and the browser closing upon disconnect.
+    for (BrowserContextImpl context : new ArrayList<>(contexts)) {
+      for (PageImpl page : new ArrayList<>(context.pages)) {
+        page.didClose();
+      }
+      context.didClose();
+    }
+    didClose();
+  }
+
   @Override
   public List<BrowserContext> contexts() {
     return new ArrayList<>(contexts);
@@ -185,8 +206,12 @@ class BrowserImpl extends ChannelOwner implements Browser {
   @Override
   void handleEvent(String event, JsonObject parameters) {
     if ("close".equals(event)) {
-      isConnected = false;
-      listeners.notify(EventType.DISCONNECTED, this);
+      didClose();
     }
   }
+
+  private void didClose() {
+    isConnected = false;
+    listeners.notify(EventType.DISCONNECTED, this);
+  }
 }

playwright/src/main/java/com/microsoft/playwright/impl/BrowserTypeImpl.java

@@ -27,6 +27,7 @@ import java.net.URI;
 import java.net.URISyntaxException;
 import java.nio.file.Path;
 import java.time.Duration;
+import java.util.function.Consumer;
 
 import static com.microsoft.playwright.impl.Serialization.gson;
 
@@ -60,19 +61,28 @@ class BrowserTypeImpl extends ChannelOwner implements BrowserType {
       if (options != null && options.timeout != null) {
         timeout = Duration.ofMillis(Math.round(options.timeout));
       }
-      Connection connection = new Connection(new WebSocketTransport(new URI(wsEndpoint), timeout));
+      WebSocketTransport transport = new WebSocketTransport(new URI(wsEndpoint), timeout);
+      Connection connection = new Connection(transport);
       RemoteBrowser remoteBrowser = (RemoteBrowser) connection.waitForObjectWithKnownName("remoteBrowser");
       PlaywrightImpl playwright = this.connection.getExistingObject("Playwright");
       SelectorsImpl selectors = remoteBrowser.selectors();
       playwright.sharedSelectors.addChannel(selectors);
       BrowserImpl browser = remoteBrowser.browser();
       browser.isRemote = true;
+      Consumer<WebSocketTransport> connectionCloseListener = new Consumer<WebSocketTransport>() {
+        @Override
+        public void accept(WebSocketTransport t) {
+          browser.notifyRemoteClosed();
+        }
+      };
+      transport.onClose(connectionCloseListener);
       browser.onDisconnected(b -> {
         playwright.sharedSelectors.removeChannel(selectors);
+        transport.offClose(connectionCloseListener);
         try {
           connection.close();
         } catch (IOException e) {
-          e.printStackTrace();
+          e.printStackTrace(System.err);
         }
       });
       return browser;

playwright/src/main/java/com/microsoft/playwright/impl/PageImpl.java

@@ -225,12 +225,16 @@ public class PageImpl extends ChannelOwner implements Page {
     } else if ("crash".equals(event)) {
       listeners.notify(EventType.CRASH, this);
     } else if ("close".equals(event)) {
-      isClosed = true;
-      browserContext.pages.remove(this);
-      listeners.notify(EventType.CLOSE, this);
+      didClose();
     }
   }
 
+  void didClose() {
+    isClosed = true;
+    browserContext.pages.remove(this);
+    listeners.notify(EventType.CLOSE, this);
+  }
+
   private void willAddFileChooserListener() {
     if (!listeners.hasListeners(EventType.FILECHOOSER)) {
       updateFileChooserInterception(true);

playwright/src/main/java/com/microsoft/playwright/impl/WebSocketTransport.java

@@ -26,6 +26,7 @@ import java.time.Duration;
 import java.util.concurrent.ArrayBlockingQueue;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
 
 
 class WebSocketTransport implements Transport {
@@ -33,6 +34,9 @@ class WebSocketTransport implements Transport {
   private final ClientConnection clientConnection;
   private boolean isClosed;
   private volatile Exception lastError;
+  ListenerCollection<EventType> listeners = new ListenerCollection<>();
+
+  private enum EventType { CLOSE }
 
   private class ClientConnection extends WebSocketClient {
     ClientConnection(URI serverUri) {
@@ -71,17 +75,13 @@ class WebSocketTransport implements Transport {
 
   @Override
   public void send(String message) {
-    if (clientConnection.isClosed()) {
-      throw new PlaywrightException("Playwright connection closed");
-    }
+    checkIfClosed();
     clientConnection.send(message);
   }
 
   @Override
   public String poll(Duration timeout) {
-    if (isClosed || clientConnection.isClosed()) {
-      throw new PlaywrightException("Playwright connection closed");
-    }
+    checkIfClosed();
     try {
       return incoming.poll(timeout.toMillis(), TimeUnit.MILLISECONDS);
     } catch (InterruptedException e) {
@@ -97,4 +97,23 @@ class WebSocketTransport implements Transport {
     isClosed = true;
     clientConnection.close();
   }
+
+  void onClose(Consumer<WebSocketTransport> handler) {
+    listeners.add(EventType.CLOSE, handler);
+  }
+
+  void offClose(Consumer<WebSocketTransport> handler) {
+    listeners.remove(EventType.CLOSE, handler);
+  }
+
+  private void checkIfClosed() {
+    if (isClosed) {
+      throw new PlaywrightException("Playwright connection closed");
+    }
+    if (clientConnection.isClosed()) {
+      isClosed = true;
+      listeners.notify(EventType.CLOSE, this);
+      throw new PlaywrightException("Playwright connection closed");
+    }
+  }
 }

playwright/src/test/java/com/microsoft/playwright/TestBase.java

@@ -60,11 +60,13 @@ public class TestBase {
     return options;
   }
 
-  static void launchBrowser(BrowserType.LaunchOptions launchOptions) {
+  static void initBrowserType() {
     playwright = Playwright.create();
-
     browserType = Utils.getBrowserTypeFromEnv(playwright);
+  }
 
+  static void launchBrowser(BrowserType.LaunchOptions launchOptions) {
+    initBrowserType();
     browser = browserType.launch(launchOptions);
   }
 

playwright/src/test/java/com/microsoft/playwright/TestBrowserTypeConnect.java

@@ -0,0 +1,211 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.microsoft.playwright;
+
+import com.microsoft.playwright.impl.Driver;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class TestBrowserTypeConnect extends TestBase {
+  private static Process browserServer;
+  private static String wsEndpoint;
+
+  private static class BrowserServer {
+    Process process;
+    String wsEndpoint;
+
+    void kill() throws InterruptedException {
+      process.destroy();
+      int exitCode = process.waitFor();
+      assertEquals(0, exitCode);
+    }
+  }
+
+  private static BrowserServer launchBrowserServer() {
+    try {
+      Path driver = Driver.ensureDriverInstalled();
+      Path dir = driver.getParent();
+      String node = dir.resolve(isWindows ? "node.exe" : "node").toString();
+      String cliJs = dir.resolve("package/lib/cli/cli.js").toString();
+      // We launch node process directly instead of using playwright.sh script as killing the script
+      // process will leave node process running and killing it would be more hassle.
+      ProcessBuilder pb = new ProcessBuilder(node, cliJs, "launch-server", browserType.name());
+      pb.directory(dir.toFile());
+      pb.redirectError(ProcessBuilder.Redirect.INHERIT);
+      BrowserServer result = new BrowserServer();
+      result.process = pb.start();
+      BufferedReader input =  new BufferedReader(new InputStreamReader(result.process.getInputStream()));
+      result.wsEndpoint = input.readLine();
+      if (!result.wsEndpoint.startsWith("ws://")) {
+        throw new RuntimeException("Invalid web socket address: " + result.wsEndpoint);
+      }
+      return result;
+    } catch (IOException e) {
+      throw new PlaywrightException("Failed to launch server", e);
+    }
+  }
+
+  @BeforeAll
+  // Hide base class method to launch browser server and connect to it.
+  static void launchBrowser() {
+    initBrowserType();
+    BrowserServer r = launchBrowserServer();
+    wsEndpoint = r.wsEndpoint;
+    browserServer = r.process;
+    browser = browserType.connect(wsEndpoint);
+    // Do not actually connect to browser, the tests will do it manually.
+  }
+
+  @AfterAll
+  static void closeBrowser() {
+    TestBase.closeBrowser();
+    if (browserServer != null) {
+      browserServer.destroyForcibly();
+      browserServer = null;
+      wsEndpoint = null;
+    }
+  }
+
+  @Test
+  void shouldBeAbleToReconnectToABrowser() {
+    {
+      Browser browser = browserType.connect(wsEndpoint);
+      BrowserContext browserContext = browser.newContext();
+      assertEquals(0, browserContext.pages().size());
+      Page page = browserContext.newPage();
+      assertEquals(121, page.evaluate("11 * 11"));
+      page.navigate(server.EMPTY_PAGE);
+      browser.close();
+    }
+    {
+      Browser browser = browserType.connect(wsEndpoint);
+      BrowserContext browserContext = browser.newContext();
+      Page page = browserContext.newPage();
+      page.navigate(server.EMPTY_PAGE);
+      browser.close();
+    }
+  }
+
+  @Test
+  void shouldBeAbleToConnectTwoBrowsersAtTheSameTime() {
+    Browser browser1 = browserType.connect(wsEndpoint);
+    assertEquals(0, browser1.contexts().size());
+    browser1.newContext();
+    assertEquals(1, browser1.contexts().size());
+
+    Browser browser2 = browserType.connect(wsEndpoint);
+    assertEquals(0, browser2.contexts().size());
+    browser2.newContext();
+    assertEquals(1, browser2.contexts().size());
+    assertEquals(1, browser1.contexts().size());
+
+    browser1.close();
+    Page page2 = browser2.newPage();
+    assertEquals(42, page2.evaluate("7 * 6")); // original browser should still work
+
+    browser2.close();
+  }
+
+  @Test
+  void disconnectedEventShouldBeEmittedWhenBrowserIsClosedOrServerIsClosed() throws InterruptedException {
+    // Launch another server to not affect other tests.
+    BrowserServer remote = launchBrowserServer();
+
+    Browser browser1 = browserType.connect(remote.wsEndpoint);
+    Browser browser2 = browserType.connect(remote.wsEndpoint);
+
+    int[] disconnected1 = {0};
+    int[] disconnected2 = {0};
+    browser1.onDisconnected(b -> ++disconnected1[0]);
+    browser2.onDisconnected(b -> ++disconnected2[0]);
+
+    Page page2 = browser2.newPage();
+
+    browser1.close();
+    assertEquals(1, disconnected1[0]);
+    assertEquals(0, disconnected2[0]);
+
+    remote.kill();
+    assertEquals(1, disconnected1[0]);
+    try {
+      // Tickle connection so that it gets a chance to dispatch disconnect event.
+      page2.title();
+      fail("did not throw");
+    } catch (PlaywrightException e) {
+    }
+    assertEquals(1, disconnected2[0]);
+  }
+
+  @Test
+  void disconnectedEventShouldHaveBrowserAsArgument() {
+    Browser browser = browserType.connect(wsEndpoint);
+    Browser[] disconnected = {null};
+    browser.onDisconnected(b -> disconnected[0] = b);
+    browser.close();
+    assertEquals(browser, disconnected[0]);
+  }
+
+  void shouldHandleExceptionsDuringConnect() {
+    // This is an implementation detail test
+  }
+
+  @Test
+  void shouldSetTheBrowserConnectedState() {
+    Browser remote = browserType.connect(wsEndpoint);
+    assertTrue(remote.isConnected());
+    remote.close();
+    assertFalse(remote.isConnected());
+  }
+
+  @Test
+  void shouldThrowWhenUsedAfterIsConnectedReturnsFalse() throws InterruptedException {
+    // Launch another server to not affect other tests.
+    BrowserServer server = launchBrowserServer();
+    Browser remote = browserType.connect(server.wsEndpoint);
+    Page page = remote.newPage();
+    server.kill();
+    try {
+      page.evaluate("1 + 1");
+    } catch (PlaywrightException e) {
+      assertTrue(e.getMessage().contains("Playwright connection closed"));
+    }
+    assertFalse(remote.isConnected());
+  }
+
+    @Test
+    void shouldRejectNavigationWhenBrowserCloses() {
+      Browser remote = browserType.connect(wsEndpoint);
+      Page page = remote.newPage();
+
+      server.setRoute("/one-style.css", r -> {});
+      page.onRequest(r -> remote.close());
+      try {
+        page.navigate(server.PREFIX + "/one-style.html", new Page.NavigateOptions().withTimeout(60000));
+        fail("did not throw");
+      } catch (PlaywrightException e) {
+        assertTrue(e.getMessage().contains("Playwright connection closed"));
+      }
+    }
+  }

scripts/CLI_VERSION

@@ -1 +1 @@
-1.9.1-1614817648000
+1.9.1-1614885198000