Bug 1759169 - [remote] Initialize the Remote Agent before the first top-level window has been opened. r=webdriver-reviewers,geckoview-reviewers,jdescottes,agi,mossop

Differential Revision: https://phabricator.services.mozilla.com/D143380

browser/components/BrowserGlue.jsm

@@ -2745,7 +2745,7 @@ BrowserGlue.prototype = {
         },
       },
 
-      // WebDriver components (Remote Agent and Marionette) need to be
+      // WebDriver components (Marionette) need to be
       // initialized as very last step.
       {
         condition: AppConstants.ENABLE_WEBDRIVER,
@@ -2758,9 +2758,6 @@ BrowserGlue.prototype = {
               "browser-startup-idle-tasks-finished"
             );
 
-            // Request startup of the Remote Agent (support for WebDriver BiDi
-            // and the partial Chrome DevTools protocol) before Marionette.
-            Services.obs.notifyObservers(null, "remote-startup-requested");
             Services.obs.notifyObservers(null, "marionette-startup-requested");
           });
         },

mobile/android/chrome/geckoview/geckoview.js

@@ -853,15 +853,13 @@ function startup() {
     );
 
     InitLater(() => {
-      // This lets Marionette and the Remote Agent (used for our CDP and the
-      // upcoming WebDriver BiDi implementation) start listening (when enabled).
-      // Both GeckoView and these two remote protocols do most of their
+      // This lets Marionette start listening (when enabled).
+      // Both GeckoView and this remote protocol do most of their
       // initialization in "profile-after-change", and there is no order enforced
-      // between them.  Therefore we defer asking both components to startup
-      // until after all "profile-after-change" handlers (including this one)
-      // have completed.
+      // between them.  Therefore we defer asking the Marionette component to
+      // startup until after all "profile-after-change" handlers (including this
+      // one) have completed.
       Services.obs.notifyObservers(null, "marionette-startup-requested");
-      Services.obs.notifyObservers(null, "remote-startup-requested");
     });
   });
 

remote/.gitignore

@@ -1,3 +1,4 @@
-test/puppeteer/package-lock.json
-test/puppeteer/node_modules/
 test/puppeteer/.local-chromium/
+test/puppeteer/lib/
+test/puppeteer/node_modules/
+test/puppeteer/package-lock.json

remote/cdp/CDP.jsm

@@ -75,6 +75,13 @@ class CDP {
       return;
     }
 
+    // Starting CDP too early can cause issues with clients like Puppeteer.
+    // Especially when closing the browser while it's still starting up, which
+    // can cause shutdown hangs. As such CDP will be started when all the
+    // browser windows are restored (workaround for bug 1764420).
+    logger.debug(`Delay start-up until all windows have been restored`);
+    await this.agent.browserStartupFinished;
+
     // Note: Ideally this would only be set at the end of the method. However
     // since start() is async, we prefer to set the flag early in order to
     // avoid potential race conditions.

remote/components/RemoteAgent.jsm

@@ -14,6 +14,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   Services: "resource://gre/modules/Services.jsm",
 
   CDP: "chrome://remote/content/cdp/CDP.jsm",
+  Deferred: "chrome://remote/content/shared/Sync.jsm",
   HttpServer: "chrome://remote/content/server/HTTPD.jsm",
   Log: "chrome://remote/content/shared/Log.jsm",
   WebDriverBiDi: "chrome://remote/content/webdriver-bidi/WebDriverBiDi.jsm",
@@ -44,6 +45,7 @@ class RemoteAgentClass {
   #enabled;
   #port;
   #server;
+  #browserStartupFinished;
 
   #cdp;
   #webDriverBiDi;
@@ -55,6 +57,7 @@ class RemoteAgentClass {
     this.#enabled = false;
     this.#port = DEFAULT_PORT;
     this.#server = null;
+    this.#browserStartupFinished = Deferred();
 
     // Supported protocols
     this.#cdp = null;
@@ -94,6 +97,17 @@ class RemoteAgentClass {
     return this.#allowOrigins;
   }
 
+  /**
+   * A promise resolved once initialization of the browser has completed and
+   * all the windows restored.
+   *
+   * @returns {Promise}
+   *     Promise that resolves when the application startup has finished.
+   */
+  get browserStartupFinished() {
+    return this.#browserStartupFinished.promise;
+  }
+
   get cdp() {
     return this.#cdp;
   }
@@ -201,8 +215,7 @@ class RemoteAgentClass {
 
       Services.obs.notifyObservers(null, "remote-listening", true);
 
-      await this.cdp?.start();
-      await this.webDriverBiDi?.start();
+      await Promise.all([this.webDriverBiDi?.start(), this.cdp?.start()]);
     } catch (e) {
       await this.close();
       logger.error(`Unable to start remote agent: ${e.message}`, e);
@@ -298,19 +311,19 @@ class RemoteAgentClass {
         this.#enabled = this.handleRemoteDebuggingPortFlag(subject);
 
         if (this.enabled) {
-          Services.obs.addObserver(this, "remote-startup-requested");
+          Services.obs.addObserver(this, "final-ui-startup");
 
           this.#allowHosts = this.handleAllowHostsFlag(subject);
           this.#allowOrigins = this.handleAllowOriginsFlag(subject);
         }
 
-        // Ideally we should only enable the Remote Agent when the command
+        // Ideally we should only initialize the Remote Agent when the command
         // line argument has been specified. But to allow Browser Chrome tests
-        // to run the Remote Agent and the supported protocols also need to be
-        // initialized.
+        // to run certain states have to be set and listeners registered.
+        // Once bug 1762647 is fixed all the next lines can be moved to
+        // "final-ui-startup".
 
-        // Listen for application shutdown to also shutdown the Remote Agent
-        // and a possible running instance of httpd.js.
+        Services.obs.addObserver(this, "sessionstore-windows-restored");
         Services.obs.addObserver(this, "quit-application");
 
         // With Bug 1717899 we will extend the lifetime of the Remote Agent to
@@ -335,7 +348,7 @@ class RemoteAgentClass {
 
         break;
 
-      case "remote-startup-requested":
+      case "final-ui-startup":
         Services.obs.removeObserver(this, topic);
 
         try {
@@ -347,6 +360,15 @@ class RemoteAgentClass {
 
         break;
 
+      // For now only used in CDP to wait until all the application windows
+      // have been opened and fully restored.
+      case "sessionstore-windows-restored":
+        Services.obs.removeObserver(this, topic);
+        this.#browserStartupFinished.resolve();
+        break;
+
+      // Listen for application shutdown to also shutdown the Remote Agent
+      // and a possible running instance of httpd.js.
       case "quit-application":
         Services.obs.removeObserver(this, "quit-application");