[e2e] Try to restart the browser if it's disconnected

When the browser crashes the underlying Transport (WebSockets) should
close, so we can look at this signal, and try to create a new
browser. For this we need to try to get/check the browser before
each test, using maps to reduce unnecessary check in information.

Also as drive-by clean up some extra issue:
When we try to get screenshot with from closed browser.
When we try to close a closed browser.

Fixed: 430539152
Change-Id: I7c1c3d2037beb6e689d888027303935e5d8b4a4c
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6718828
Reviewed-by: Alex Rudenko <[email protected]>
Commit-Queue: Nikolay Vitkov <[email protected]>
Reviewed-by: Philip Pfaffe <[email protected]>
Auto-Submit: Nikolay Vitkov <[email protected]>

Author: Lightning00Blade

test/e2e_non_hosted/conductor/mocha-interface-helpers.ts

@@ -11,12 +11,12 @@ import {TestConfig} from '../../conductor/test_config.js';
 
 import {StateProvider} from './state-provider.js';
 
-async function takeScreenshots(state: E2E.State): Promise<{target?: string, frontend?: string}> {
+async function takeScreenshots(state: E2E.State): Promise<{inspectedPage?: string, devToolsPage?: string}> {
   try {
     const {devToolsPage, inspectedPage} = state;
-    const targetScreenshot = await inspectedPage.screenshot();
-    const frontendScreenshot = await devToolsPage.screenshot();
-    return {target: targetScreenshot, frontend: frontendScreenshot};
+    const inspectedPageScreenshot = await inspectedPage.screenshot();
+    const devToolsPageScreenshot = await devToolsPage.screenshot();
+    return {inspectedPage: inspectedPageScreenshot, devToolsPage: devToolsPageScreenshot};
   } catch (err) {
     console.error('Error taking a screenshot', err);
     return {};
@@ -39,12 +39,17 @@ async function finalizeTestError(state: ExtendedState|undefined, error: Error):
 }
 
 export async function screenshotError(state: E2E.State, error: Error) {
+  if (!state.browser.connected) {
+    console.error('Browser was disconnected, skipping screenshots');
+    return error;
+  }
+
   console.error('Taking screenshots for the error:', error);
   if (!TestConfig.debug) {
     try {
       const screenshotTimeout = 5_000;
       let timer: ReturnType<typeof setTimeout>;
-      const {target, frontend} = await Promise.race([
+      const {inspectedPage, devToolsPage} = await Promise.race([
         takeScreenshots(state).then(result => {
           clearTimeout(timer);
           return result;
@@ -53,10 +58,10 @@ export async function screenshotError(state: E2E.State, error: Error) {
           timer = setTimeout(resolve, screenshotTimeout);
         }).then(() => {
           console.error(`Could not take screenshots within ${screenshotTimeout}ms.`);
-          return {target: undefined, frontend: undefined};
+          return {inspectedPage: undefined, devToolsPage: undefined};
         }),
       ]);
-      return ScreenshotError.fromBase64Images(error, target, frontend);
+      return ScreenshotError.fromBase64Images(error, inspectedPage, devToolsPage);
     } catch (e) {
       console.error('Unexpected error saving screenshots', e);
       return e;
@@ -121,7 +126,9 @@ export class InstrumentedTestFunction {
     // including all error and timeout handling that still might rely
     // on the browserContext and pages.
     try {
-      await this.state?.browsingContext.close();
+      if (this.state?.state.browser.connected) {
+        await this.state?.browsingContext.close();
+      }
     } catch (e) {
       console.error('Unexpected error during cleanup', e);
     }

test/e2e_non_hosted/conductor/mocha-interface.ts

@@ -49,7 +49,7 @@ function devtoolsTestInterface(rootSuite: Mocha.Suite) {
             mochaGlobals.describe = customDescribe(defaultImplementation.suite, '', thisSuite);
             // @ts-expect-error Custom interface.
             mochaGlobals.setup = function(suiteSettings: SuiteSettings) {
-              StateProvider.instance.registerSettingsCallback(thisSuite, suiteSettings);
+              StateProvider.instance.registerSuiteSettings(thisSuite, suiteSettings);
             };
             // @ts-expect-error Custom interface.
             mochaGlobals.it = customIt(defaultImplementation.test, thisSuite, thisSuite.file || '', mochaRoot);
@@ -67,7 +67,7 @@ function devtoolsTestInterface(rootSuite: Mocha.Suite) {
         });
 
         if (!suite.isPending()) {
-          suite.beforeAll(async function(this: Mocha.Context) {
+          suite.beforeEach(async function(this: Mocha.Context) {
             this.timeout(0);
             await StateProvider.instance.resolveBrowser(suite);
           });

test/e2e_non_hosted/conductor/state-provider.ts

@@ -19,41 +19,71 @@ const DEFAULT_SETTINGS = {
 
 export class StateProvider {
   static instance = new StateProvider();
-
-  #settingsCallbackMap = new Map<Mocha.Suite, E2E.SuiteSettings>();
-  #browserMap = new Map<string, BrowserWrapper>();
   static serverPort: number;
 
+  /**
+   * This provides a mapping whenever we have called the
+   * {@link Mocha.setup | setup()} function with custom settings.
+   */
+  #suiteToSettingsMap = new WeakMap<Mocha.Suite, E2E.SuiteSettings>();
+  /**
+   * This provides a quick link between suite and browser with
+   * the correct setting allowing us to skip some check
+   * because we try to run the creation of browser in a beforeEach
+   */
+  #suiteToBrowser = new WeakMap<Mocha.Suite, BrowserWrapper>();
+  /**
+   * Provides a mapping between a stable key (sorted JSON)
+   * created from the settings and a browser.
+   * This allows us to have a single browser between test
+   * that require the same settings.
+   */
+  #settingToBrowser = new Map<string, BrowserWrapper>();
+
   private constructor() {
   }
 
-  registerSettingsCallback(suite: Mocha.Suite, suiteSettings: E2E.SuiteSettings) {
-    this.#settingsCallbackMap.set(suite, suiteSettings);
+  registerSuiteSettings(suite: Mocha.Suite, suiteSettings: E2E.SuiteSettings): void {
+    this.#suiteToSettingsMap.set(suite, suiteSettings);
   }
 
-  async resolveBrowser(suite: Mocha.Suite) {
+  async resolveBrowser(suite: Mocha.Suite): Promise<void> {
     if (!StateProvider.serverPort) {
       StateProvider.serverPort = await StateProvider.#globalSetup();
     }
-    const settings = await this.#getSettings(suite);
+    let browser = this.#suiteToBrowser.get(suite);
+    if (browser?.connected) {
+      return;
+    }
+
+    const settings = this.#getSettings(suite);
     const browserSettings = {
       enabledBlinkFeatures: (settings.enabledBlinkFeatures ?? []).toSorted(),
       disabledFeatures: (settings.disabledFeatures ?? []).toSorted(),
     };
     const browserKey = JSON.stringify(browserSettings);
-    let browser = this.#browserMap.get(browserKey);
-    if (!browser) {
+    browser = this.#settingToBrowser.get(browserKey);
+    if (browser && !browser.connected) {
+      browser?.browser.process()?.kill();
+    }
+
+    if (!browser?.connected) {
       browser = await Launcher.browserSetup(browserSettings);
-      this.#browserMap.set(browserKey, browser);
+      this.#settingToBrowser.set(browserKey, browser);
     }
+    this.#suiteToBrowser.set(suite, browser);
     // Suite needs to be aware of the browser instance to be able to create the
     // full state for the tests
     suite.browser = browser;
   }
 
   async getState(suite: Mocha.Suite) {
-    const settings = await this.#getSettings(suite);
+    const settings = this.#getSettings(suite);
     const browser = suite.browser;
+    if (!browser.connected) {
+      throw new Error('Browser disconnected unexpectedly');
+    }
+
     const browsingContext = await browser.createBrowserContext();
     const inspectedPage = await setupInspectedPage(browsingContext, StateProvider.serverPort);
     const devToolsPage = await setupDevToolsPage(browsingContext, settings);
@@ -68,8 +98,8 @@ export class StateProvider {
     return {state, browsingContext};
   }
 
-  async #getSettings(suite: Mocha.Suite): Promise<E2E.HarnessSettings> {
-    const settings = this.#settingsCallbackMap.get(suite);
+  #getSettings(suite: Mocha.Suite): E2E.HarnessSettings {
+    const settings = this.#suiteToSettingsMap.get(suite);
     if (settings) {
       return mergeSettings(settings, DEFAULT_SETTINGS);
     }
@@ -88,7 +118,7 @@ export class StateProvider {
   }
 
   async closeBrowsers() {
-    await Promise.all([...this.#browserMap.values()].map(async browser => {
+    await Promise.allSettled([...this.#settingToBrowser.values()].map(async browser => {
       await browser.browser.close();
     }));
   }

test/e2e_non_hosted/shared/browser-helper.ts

@@ -17,6 +17,10 @@ export class BrowserWrapper {
     this.browser = b;
   }
 
+  get connected() {
+    return this.browser.connected;
+  }
+
   async createBrowserContext() {
     return await this.browser.createBrowserContext();
   }