[lighthouse] Use CDPConnection instead of parallel connection

Both on the frontend-side and the worker-side, this CL updates
Lighthouse to use the new CDPConnection.

On the frontend-side, we create a new CDP session and then use
the standard CDPConnection to send/receive messages, which
are forwarded nearly 1:1 to the worker.

On the worker-side, we create a custom transport with
a CDPConnection, and then wrap it with the new puppeteer
adapter.

Bug: 453469270
Change-Id: I737816f4ce657210079413ba888e9c26897dac82
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7106261
Reviewed-by: Connor Clark <[email protected]>
Reviewed-by: Paul Irish <[email protected]>
Commit-Queue: Simon Zünd <[email protected]>

Author: szuend

front_end/entrypoints/lighthouse_worker/BUILD.gn

@@ -10,6 +10,7 @@ devtools_module("lighthouse_worker") {
   sources = [ "LighthouseWorkerService.ts" ]
 
   deps = [
+    "../../core/protocol_client:bundle",
     "../../core/root:bundle",
     "../../core/sdk:bundle",
     "../../services/puppeteer:bundle",

front_end/entrypoints/lighthouse_worker/LighthouseWorkerService.ts

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import * as ProtocolClient from '../../core/protocol_client/protocol_client.js';
 import * as Root from '../../core/root/root.js';
-import type * as SDK from '../../core/sdk/sdk.js';
 import * as PuppeteerService from '../../services/puppeteer/puppeteer.js';
 import * as ThirdPartyWeb from '../../third_party/third-party-web/third-party-web.js';
 
@@ -12,19 +12,12 @@ function disableLoggingForTest(): void {
 }
 
 /**
- * ConnectionProxy is a SDK interface, but the implementation has no knowledge it's a parallelConnection.
- * The CDP traffic is smuggled back and forth by the system described in LighthouseProtocolService
+ * WorkerConnectionTransport is a DevTools `ConnectionTransport` implementation that talks
+ * CDP via web worker postMessage. The system is described in LighthouseProtocolService.
  */
-class ConnectionProxy implements SDK.Connections.ParallelConnectionInterface {
-  sessionId: string;
-  onMessage: ((arg0: Object) => void)|null;
-  onDisconnect: ((arg0: string) => void)|null;
-
-  constructor(sessionId: string) {
-    this.sessionId = sessionId;
-    this.onMessage = null;
-    this.onDisconnect = null;
-  }
+class WorkerConnectionTransport implements ProtocolClient.ConnectionTransport.ConnectionTransport {
+  onMessage: ((arg0: Object) => void)|null = null;
+  onDisconnect: ((arg0: string) => void)|null = null;
 
   setOnMessage(onMessage: (arg0: Object|string) => void): void {
     this.onMessage = onMessage;
@@ -34,14 +27,6 @@ class ConnectionProxy implements SDK.Connections.ParallelConnectionInterface {
     this.onDisconnect = onDisconnect;
   }
 
-  getOnDisconnect(): (((arg0: string) => void)|null) {
-    return this.onDisconnect;
-  }
-
-  getSessionId(): string {
-    return this.sessionId;
-  }
-
   sendRawMessage(message: string): void {
     notifyFrontendViaWorkerMessage('sendProtocolMessage', {message});
   }
@@ -53,7 +38,7 @@ class ConnectionProxy implements SDK.Connections.ParallelConnectionInterface {
   }
 }
 
-let cdpConnection: ConnectionProxy|undefined;
+let cdpTransport: WorkerConnectionTransport|undefined;
 let endTimespan: (() => unknown)|undefined;
 
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -68,8 +53,8 @@ async function invokeLH(action: string, args: any): Promise<unknown> {
     notifyFrontendViaWorkerMessage('statusUpdate', {message: message[1]});
   });
 
-  let puppeteerHandle: Awaited<ReturnType<typeof PuppeteerService.PuppeteerConnection
-                                              .PuppeteerConnectionHelper['connectPuppeteerToConnectionViaTabLegacy']>>|
+  let puppeteerHandle: Awaited<ReturnType<
+      typeof PuppeteerService.PuppeteerConnection.PuppeteerConnectionHelper['connectPuppeteerToConnectionViaTab']>>|
       undefined;
 
   try {
@@ -98,11 +83,15 @@ async function invokeLH(action: string, args: any): Promise<unknown> {
     self.thirdPartyWeb.provideThirdPartyWeb(ThirdPartyWeb.ThirdPartyWeb);
 
     const {rootTargetId, mainSessionId} = args;
-    cdpConnection = new ConnectionProxy(mainSessionId);
+    cdpTransport = new WorkerConnectionTransport();
+    // TODO(crbug.com/453469270): Use "DevToolsCDPConnection" once we split SessionRouter into
+    //                            a connection handling part and a session handling part.
+    const connection = new ProtocolClient.InspectorBackend.SessionRouter(cdpTransport);
     puppeteerHandle =
-        await PuppeteerService.PuppeteerConnection.PuppeteerConnectionHelper.connectPuppeteerToConnectionViaTabLegacy({
-          connection: cdpConnection,
-          rootTargetId,
+        await PuppeteerService.PuppeteerConnection.PuppeteerConnectionHelper.connectPuppeteerToConnectionViaTab({
+          connection,
+          targetId: rootTargetId,
+          sessionId: mainSessionId,
           // Lighthouse can only audit normal pages.
           isPageTargetCallback: targetInfo => targetInfo.type === 'page',
         });
@@ -212,7 +201,7 @@ async function onFrontendMessage(event: MessageEvent): Promise<void> {
       break;
     }
     case 'dispatchProtocolMessage': {
-      cdpConnection?.onMessage?.(messageFromFrontend.args.message);
+      cdpTransport?.onMessage?.(messageFromFrontend.args.message);
       break;
     }
     default: {

front_end/panels/lighthouse/BUILD.gn

@@ -38,6 +38,7 @@ devtools_ui_module("lighthouse") {
     "../../core/protocol_client:bundle",
     "../../core/root:bundle",
     "../../core/sdk:bundle",
+    "../../generated:protocol",
     "../../models/geometry:bundle",
     "../../models/text_utils:bundle",
     "../../models/trace:bundle",

front_end/panels/lighthouse/LighthouseProtocolService.test.ts

@@ -2,10 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import type * as ProtocolClient from '../../core/protocol_client/protocol_client.js';
 import * as SDK from '../../core/sdk/sdk.js';
 import {createTarget} from '../../testing/EnvironmentHelpers.js';
-import {describeWithMockConnection} from '../../testing/MockConnection.js';
+import {describeWithMockConnection, setMockConnectionResponseHandler} from '../../testing/MockConnection.js';
 
 import type * as LighthouseModule from './lighthouse.js';
 
@@ -16,7 +15,6 @@ describeWithMockConnection('LighthouseProtocolService', () => {
   let rootTarget: SDK.Target.Target;
   let suspendAllTargets: sinon.SinonStub;
   let resumeAllTargets: sinon.SinonStub;
-  let createParallelConnection: sinon.SinonStub;
 
   beforeEach(async () => {
     Lighthouse = await import('./lighthouse.js');
@@ -33,19 +31,10 @@ describeWithMockConnection('LighthouseProtocolService', () => {
     assert.exists(childTargetManager);
 
     sinon.stub(childTargetManager, 'getParentTargetId').resolves(primaryTarget.targetInfo()?.targetId);
-    if (rootTarget === primaryTarget) {
-      createParallelConnection = sinon.stub(childTargetManager, 'createParallelConnection').resolves({
-        connection: {disconnect: () => {}} as ProtocolClient.ConnectionTransport.ConnectionTransport,
-        sessionId: 'foo',
-      });
-    } else {
+    if (rootTarget !== primaryTarget) {
       const rootChildTargetManager = rootTarget.model(SDK.ChildTargetManager.ChildTargetManager);
       assert.exists(rootChildTargetManager);
       sinon.stub(rootChildTargetManager, 'getParentTargetId').resolves(rootTarget.targetInfo()?.targetId);
-      createParallelConnection = sinon.stub(rootChildTargetManager, 'createParallelConnection').resolves({
-        connection: {disconnect: () => {}} as ProtocolClient.ConnectionTransport.ConnectionTransport,
-        sessionId: 'foo',
-      });
     }
   });
 
@@ -55,10 +44,15 @@ describeWithMockConnection('LighthouseProtocolService', () => {
     sinon.assert.calledOnce(suspendAllTargets);
   });
 
-  it('creates a parallel connection', async () => {
+  it('attaches to to the root target', async () => {
+    const attachedToTargetStub = sinon.stub();
+    setMockConnectionResponseHandler('Target.attachToTarget', attachedToTargetStub);
     const service = new Lighthouse.LighthouseProtocolService.ProtocolService();
+
     await service.attach();
-    sinon.assert.calledOnce(createParallelConnection);
+
+    sinon.assert.calledOnceWithExactly(
+        attachedToTargetStub, {targetId: rootTarget.targetInfo()?.targetId, flatten: true});
   });
 
   it('resumes all targets', async () => {

front_end/panels/lighthouse/LighthouseProtocolService.ts

@@ -6,16 +6,17 @@ import * as i18n from '../../core/i18n/i18n.js';
 import type * as Platform from '../../core/platform/platform.js';
 import type * as ProtocolClient from '../../core/protocol_client/protocol_client.js';
 import * as SDK from '../../core/sdk/sdk.js';
+import type * as Protocol from '../../generated/protocol.js';
 
 import type * as ReportRenderer from './LighthouseReporterTypes.js';
 
 /**
  * @file
- *                                                   ┌────────────┐
- *                                                   │CDP Backend │
- *                                                   └────────────┘
+ *                                                 ┌───────────────┐
+ *                                                 │ CDPConnection │
+ *                                                 └───────────────┘
  *                                                        │ ▲
- *                                                        │ │ parallelConnection
+ *                                                        │ │
  *                          ┌┐                            ▼ │                     ┌┐
  *                          ││   dispatchProtocolMessage     sendProtocolMessage  ││
  *                          ││                     │          ▲                   ││
@@ -28,7 +29,10 @@ import type * as ReportRenderer from './LighthouseReporterTypes.js';
  *                          ││   onFrontendMessage      notifyFrontendViaWorkerMessage  ││
  *                          ││                   │       ▲                              ││
  *                          ││                   ▼       │                              ││
- *  LighthouseWorkerService ││          Either ConnectionProxy or LegacyPort            ││
+ *  LighthouseWorkerService ││               WorkerConnectionTransport                  ││
+ *                          ││                           │ ▲                            ││
+ *                          ││                           ▼ │                            ││
+ *                          ││                     CDPConnection                        ││
  *                          ││                           │ ▲                            ││
  *                          ││     ┌─────────────────────┼─┼───────────────────────┐    ││
  *                          ││     │  Lighthouse    ┌────▼──────┐                  │    ││
@@ -37,10 +41,10 @@ import type * as ReportRenderer from './LighthouseReporterTypes.js';
  *                          └┘     └───────────────────────────────────────────────┘    └┘
  *
  * - All messages traversing the worker boundary are action-wrapped.
- * - All messages over the parallelConnection speak pure CDP.
- * - All messages within ConnectionProxy/LegacyPort speak pure CDP.
- * - The foundational CDP connection is `parallelConnection`.
- * - All connections within the worker are not actual ParallelConnection's.
+ * - All messages over the CDPConnection speak pure CDP.
+ * - Within the worker we also use a 'CDPConnection' but with a custom
+ *   transport called WorkerConnectionTransport.
+ * - All messages within WorkerConnectionTransport/LegacyPort speak pure CDP.
  */
 
 let lastId = 1;
@@ -57,14 +61,15 @@ export interface LighthouseRun {
 /**
  * ProtocolService manages a connection between the frontend (Lighthouse panel) and the Lighthouse worker.
  */
-export class ProtocolService {
-  private mainSessionId?: string;
+export class ProtocolService implements ProtocolClient.CDPConnection.CDPConnectionObserver {
+  private mainSessionId?: Protocol.Target.SessionID;
   private rootTargetId?: string;
-  private parallelConnection?: ProtocolClient.ConnectionTransport.ConnectionTransport;
+  private rootTarget?: SDK.Target.Target;
   private lighthouseWorkerPromise?: Promise<Worker>;
   private lighthouseMessageUpdateCallback?: ((arg0: string) => void);
   private removeDialogHandler?: () => void;
   private configForTesting?: object;
+  private connection?: ProtocolClient.CDPConnection.CDPConnection;
 
   async attach(): Promise<void> {
     await SDK.TargetManager.TargetManager.instance().suspendAllTargets();
@@ -90,12 +95,15 @@ export class ProtocolService {
       throw new Error('Could not find the child target manager class for the root target');
     }
 
-    const {connection, sessionId} = await rootChildTargetManager.createParallelConnection(message => {
-      if (typeof message === 'string') {
-        message = JSON.parse(message);
-      }
-      this.dispatchProtocolMessage(message);
-    });
+    const router = rootTarget.router();
+    if (!router) {
+      throw new Error('Expected root target to have a session router');
+    }
+
+    const rootTargetId = await rootChildTargetManager.getParentTargetId();
+    const {sessionId} = await rootTarget.targetAgent().invoke_attachToTarget({targetId: rootTargetId, flatten: true});
+    this.connection = router;
+    this.connection.observe(this);
 
     // Lighthouse implements its own dialog handler like this, however its lifecycle ends when
     // the internal Lighthouse session is disposed.
@@ -114,8 +122,8 @@ export class ProtocolService {
     this.removeDialogHandler = () =>
         resourceTreeModel.removeEventListener(SDK.ResourceTreeModel.Events.JavaScriptDialogOpening, dialogHandler);
 
-    this.parallelConnection = connection;
-    this.rootTargetId = await rootChildTargetManager.getParentTargetId();
+    this.rootTargetId = rootTargetId;
+    this.rootTarget = rootTarget;
     this.mainSessionId = sessionId;
   }
 
@@ -166,20 +174,22 @@ export class ProtocolService {
 
   async detach(): Promise<void> {
     const oldLighthouseWorker = this.lighthouseWorkerPromise;
-    const oldParallelConnection = this.parallelConnection;
+    const oldRootTarget = this.rootTarget;
 
     // When detaching, make sure that we remove the old promises, before we
     // perform any async cleanups. That way, if there is a message coming from
     // lighthouse while we are in the process of cleaning up, we shouldn't deliver
     // them to the backend.
     this.lighthouseWorkerPromise = undefined;
-    this.parallelConnection = undefined;
+    this.rootTarget = undefined;
+    this.connection?.unobserve(this);
+    this.connection = undefined;
 
     if (oldLighthouseWorker) {
       (await oldLighthouseWorker).terminate();
     }
-    if (oldParallelConnection) {
-      await oldParallelConnection.disconnect();
+    if (oldRootTarget && this.mainSessionId) {
+      await oldRootTarget.targetAgent().invoke_detachFromTarget({sessionId: this.mainSessionId});
     }
     await SDK.TargetManager.TargetManager.instance().resumeAllTargets();
     this.removeDialogHandler?.();
@@ -189,7 +199,11 @@ export class ProtocolService {
     this.lighthouseMessageUpdateCallback = callback;
   }
 
-  private dispatchProtocolMessage(message: string|object): void {
+  onEvent<T extends ProtocolClient.CDPConnection.Event>(event: ProtocolClient.CDPConnection.CDPEvent<T>): void {
+    this.dispatchProtocolMessage(event);
+  }
+
+  private dispatchProtocolMessage(message: ProtocolClient.CDPConnection.CDPReceivableMessage): void {
     // A message without a sessionId is the main session of the main target (call it "Main session").
     // A parallel connection and session was made that connects to the same main target (call it "Lighthouse session").
     // Messages from the "Lighthouse session" have a sessionId.
@@ -199,15 +213,15 @@ export class ProtocolService {
     //   * the message has a sessionId (is not for the "Main session")
     //   * the message does not have a sessionId (is for the "Main session"), but only for the Target domain
     //     (to kickstart autoAttach in LH).
-    const protocolMessage = message as {
-      sessionId?: string,
-      method?: string,
-    };
-    if (protocolMessage.sessionId || (protocolMessage.method?.startsWith('Target'))) {
+    if (message.sessionId || ('method' in message && message.method?.startsWith('Target'))) {
       void this.send('dispatchProtocolMessage', {message});
     }
   }
 
+  onDisconnect(): void {
+    // Do nothing.
+  }
+
   private initWorker(): Promise<Worker> {
     this.lighthouseWorkerPromise = new Promise<Worker>(resolve => {
       const workerUrl = new URL('../../entrypoints/lighthouse_worker/lighthouse_worker.js', import.meta.url);
@@ -255,9 +269,19 @@ export class ProtocolService {
   }
 
   private sendProtocolMessage(message: string): void {
-    if (this.parallelConnection) {
-      this.parallelConnection.sendRawMessage(message);
-    }
+    const {id, method, params, sessionId} = JSON.parse(message);
+    // CDPConnection manages it's own message IDs and it's important, otherwise we'd clash
+    // with the rest of the DevTools traffic.
+    // Instead, we ignore the ID coming from the worker when sending the command, but
+    // patch it back in when sending the response back to the worker.
+    void this.connection?.send(method, params, sessionId).then(response => {
+      this.dispatchProtocolMessage({
+        id,
+        sessionId,
+        result: 'result' in response ? response.result : undefined,
+        error: 'error' in response ? response.error : undefined,
+      });
+    });
   }
 
   private async send(action: string, args: Record<string, string|string[]|object> = {}): Promise<void> {