From 11a1fb38c3e51a35d3f02cb9a52367ab660b8773 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 18 Jan 2024 07:46:41 -0800 Subject: [PATCH] Distinguish between modern/legacy targets, conditionally disable hacks (#42303) Summary: Changelog: [Internal] Adds a coarse-grained mechanism to `inspector-proxy` for distinguishing between legacy and modern debug targets. The guiding principles are: 1. `inspector-proxy` does not interfere in the CDP message stream between the debugger frontend and a modern target, or in the lifecycle of a target. 2. Legacy runtimes (current React Native, React Native Desktop, etc) that rely on `inspector-proxy`'s existing invasive semantics must continue to work seamlessly for now. We'll decide on the right time to deprecate/remove this legacy code in the future. NOTE: This is an experimental addition to the proxy protocol that may be replaced at any time. Reviewed By: hoxyq Differential Revision: D50967795 --- packages/dev-middleware/package.json | 1 + .../src/__tests__/InspectorProtocolUtils.js | 4 +- .../__tests__/InspectorProxyHttpApi-test.js | 2 + .../InspectorProxyReactNativeReloads-test.js | 82 ++++++++++++++++++ .../src/inspector-proxy/Device.js | 83 +++++++++++++------ .../src/inspector-proxy/InspectorProxy.js | 2 + .../src/inspector-proxy/types.js | 11 ++- 7 files changed, 153 insertions(+), 32 deletions(-) diff --git a/packages/dev-middleware/package.json b/packages/dev-middleware/package.json index bb2de19e4dbf..7f092ba3429c 100644 --- a/packages/dev-middleware/package.json +++ b/packages/dev-middleware/package.json @@ -29,6 +29,7 @@ "connect": "^3.6.5", "debug": "^2.2.0", "node-fetch": "^2.2.0", + "nullthrows": "^1.1.1", "open": "^7.0.3", "selfsigned": "^2.4.1", "serve-static": "^1.13.1", diff --git a/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js b/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js index 1cc66e61bba2..b537323e7f47 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js +++ b/packages/dev-middleware/src/__tests__/InspectorProtocolUtils.js @@ -11,8 +11,8 @@ import type { JSONSerializable, - Page, PageDescription, + PageFromDevice, } from '../inspector-proxy/types'; import type {DebuggerMock} from './InspectorDebuggerUtils'; import type {DeviceMock} from './InspectorDeviceUtils'; @@ -125,7 +125,7 @@ export async function createAndConnectTarget( ... }>, signal: AbortSignal, - page: Page, + page: PageFromDevice, ): Promise<{device: DeviceMock, debugger_: DebuggerMock}> { let device; let debugger_; diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyHttpApi-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyHttpApi-test.js index bd2991977745..89aebdc712a6 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyHttpApi-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyHttpApi-test.js @@ -193,6 +193,7 @@ xdescribe('inspector proxy HTTP API', () => { id: 'device1-page1', reactNative: { logicalDeviceId: 'device1', + type: 'Legacy', }, title: 'bar-title', type: 'node', @@ -207,6 +208,7 @@ xdescribe('inspector proxy HTTP API', () => { id: 'device2-page1', reactNative: { logicalDeviceId: 'device2', + type: 'Legacy', }, title: 'bar-title', type: 'node', diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyReactNativeReloads-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyReactNativeReloads-test.js index e8ca5607e916..011340b40e24 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyReactNativeReloads-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyReactNativeReloads-test.js @@ -397,4 +397,86 @@ xdescribe('inspector proxy React Native reloads', () => { device.close(); } }); + + test('disabled for modern targets', async () => { + let device1; + try { + /*** + * Connect a device with one React Native page. + */ + device1 = await createDeviceMock( + `${serverRef.serverBaseWsUrl}/inspector/device?device=device1&name=foo&app=bar`, + autoCleanup.signal, + ); + device1.getPages.mockImplementation(() => [ + { + app: 'bar', + id: 'originalPage-initial', + // NOTE: 'React' is a magic string used to detect React Native pages + // in legacy mode. + title: 'React Native (mock)', + type: 'Modern', + vm: 'vm', + }, + ]); + let pageList; + await until(async () => { + pageList = (await fetchJson( + `${serverRef.serverBaseUrl}/json`, + // $FlowIgnore[unclear-type] + ): any); + expect(pageList.length).toBeGreaterThan(0); + }); + invariant(pageList != null, ''); + + /** + * The proxy reports just one page, without the synthetic page reported + * in legacy mode. + */ + + expect(pageList).toEqual([ + expect.objectContaining({ + id: expect.stringContaining('originalPage-initial'), + title: 'React Native (mock)', + }), + ]); + + /** + * Replace our original page with a new one. + */ + device1.getPages.mockImplementation(() => [ + { + app: 'bar', + id: 'originalPage-updated', + // NOTE: 'React' is a magic string used to detect React Native pages. + title: 'React Native (mock)', + type: 'Modern', + vm: 'vm', + }, + ]); + await until(async () => { + pageList = (await fetchJson( + `${serverRef.serverBaseUrl}/json`, + // $FlowIgnore[unclear-type] + ): any); + expect(pageList).toContainEqual( + expect.objectContaining({ + id: expect.stringContaining('originalPage-updated'), + }), + ); + }); + + /** + * There's still just one page reported. + */ + expect(pageList).toEqual([ + expect.objectContaining({ + id: expect.stringContaining('originalPage-updated'), + title: 'React Native (mock)', + }), + ]); + } finally { + device1?.close(); + } + }); }); diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index c890e8ed15aa..a0d4855abc3e 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -34,7 +34,7 @@ const PAGES_POLLING_INTERVAL = 1000; // Android's stock emulator and other emulators such as genymotion use a standard localhost alias. const EMULATOR_LOCALHOST_ADDRESSES: Array = ['10.0.2.2', '10.0.3.2']; -// Prefix for script URLs that are alphanumeric IDs. See comment in #processMessageFromDevice method for +// Prefix for script URLs that are alphanumeric IDs. See comment in #processMessageFromDeviceLegacy method for // more details. const FILE_PREFIX = 'file://'; @@ -77,7 +77,7 @@ export default class Device { // Last known Page ID of the React Native page. // This is used by debugger connections that don't have PageID specified // (and will interact with the latest React Native page). - #lastConnectedReactNativePage: ?Page = null; + #lastConnectedLegacyReactNativePage: ?Page = null; // Whether we are in the middle of a reload in the REACT_NATIVE_RELOADABLE_PAGE. #isReloading: boolean = false; @@ -159,12 +159,13 @@ export default class Device { } getPagesList(): $ReadOnlyArray { - if (this.#lastConnectedReactNativePage) { + if (this.#lastConnectedLegacyReactNativePage) { const reactNativeReloadablePage = { id: REACT_NATIVE_RELOADABLE_PAGE_ID, title: 'React Native Experimental (Improved Chrome Reloads)', vm: "don't use", app: this.#app, + type: 'Legacy', }; return [...this.#pages.values(), reactNativeReloadablePage]; } else { @@ -203,6 +204,11 @@ export default class Device { pageId, userAgent: metadata.userAgent, }; + + // TODO(moti): Handle null case explicitly, e.g. refuse to connect to + // unknown pages. + const page: ?Page = this.#pages.get(pageId); + this.#debuggerConnection = debuggerInfo; debug(`Got new debugger connection for page ${pageId} of ${this.#name}`); @@ -222,11 +228,14 @@ export default class Device { pageId: this.#debuggerConnection?.pageId ?? null, frontendUserAgent: metadata.userAgent, }); - const processedReq = this.#interceptMessageFromDebugger( - debuggerRequest, - debuggerInfo, - socket, - ); + let processedReq = debuggerRequest; + if (!page || page.type === 'Legacy') { + processedReq = this.#interceptMessageFromDebuggerLegacy( + debuggerRequest, + debuggerInfo, + socket, + ); + } if (processedReq) { this.#sendMessageToDevice({ @@ -302,7 +311,15 @@ export default class Device { // locations). #handleMessageFromDevice(message: MessageFromDevice) { if (message.event === 'getPages') { - this.#pages = new Map(message.payload.map(page => [page.id, page])); + this.#pages = new Map( + message.payload.map(({type, ...page}) => [ + page.id, + { + ...page, + type: type ?? 'Legacy', + }, + ]), + ); if (message.payload.length !== this.#pages.size) { const duplicateIds = new Set(); const idsSeen = new Set(); @@ -320,15 +337,15 @@ export default class Device { ); } - // Check if device have new React Native page. + // Check if device has a new legacy React Native page. // There is usually no more than 2-3 pages per device so this operation // is not expensive. // TODO(hypuk): It is better for VM to send update event when new page is // created instead of manually checking this on every getPages result. for (const page of this.#pages.values()) { - if (page.title.indexOf('React') >= 0) { - if (page.id !== this.#lastConnectedReactNativePage?.id) { - this.#newReactNativePage(page); + if (page.type === 'Legacy' && page.title.indexOf('React') >= 0) { + if (page.id !== this.#lastConnectedLegacyReactNativePage?.id) { + this.#newLegacyReactNativePage(page); break; } } @@ -337,15 +354,19 @@ export default class Device { // Device sends disconnect events only when page is reloaded or // if debugger socket was disconnected. const pageId = message.payload.pageId; + // TODO(moti): Handle null case explicitly, e.g. swallow disconnect events + // for unknown pages. + const page: ?Page = this.#pages.get(pageId); const debuggerSocket = this.#debuggerConnection ? this.#debuggerConnection.socket : null; if (debuggerSocket && debuggerSocket.readyState === WS.OPEN) { if ( this.#debuggerConnection != null && - this.#debuggerConnection.pageId !== REACT_NATIVE_RELOADABLE_PAGE_ID + this.#debuggerConnection.pageId !== REACT_NATIVE_RELOADABLE_PAGE_ID && + (!page || page.type === 'Legacy') ) { - debug(`Page ${pageId} is reloading.`); + debug(`Legacy page ${pageId} is reloading.`); debuggerSocket.send(JSON.stringify({method: 'reload'})); } } @@ -356,6 +377,7 @@ export default class Device { // FIXME: Is it possible that we received message for pageID that does not // correspond to current debugger connection? + // TODO(moti): yes, fix multi-debugger case const debuggerSocket = this.#debuggerConnection.socket; if (debuggerSocket == null || debuggerSocket.readyState !== WS.OPEN) { @@ -364,23 +386,30 @@ export default class Device { } const parsedPayload = JSON.parse(message.payload.wrappedEvent); + const pageId = this.#debuggerConnection?.pageId ?? null; + // TODO(moti): Handle null case explicitly, or ideally associate a copy + // of the page metadata object with the connection so this can never be + // null. + const page: ?Page = pageId != null ? this.#pages.get(pageId) : null; if ('id' in parsedPayload) { this.#deviceEventReporter?.logResponse(parsedPayload, 'device', { - pageId: this.#debuggerConnection?.pageId ?? null, + pageId, frontendUserAgent: this.#debuggerConnection?.userAgent ?? null, }); } - if (this.#debuggerConnection) { + if (this.#debuggerConnection && (!page || page.type === 'Legacy')) { // Wrapping just to make flow happy :) // $FlowFixMe[unused-promise] - this.#processMessageFromDevice( + this.#processMessageFromDeviceLegacy( parsedPayload, this.#debuggerConnection, ).then(() => { const messageToSend = JSON.stringify(parsedPayload); debuggerSocket.send(messageToSend); }); + } else { + debuggerSocket.send(message.payload.wrappedEvent); } } } @@ -396,7 +425,7 @@ export default class Device { } // We received new React Native Page ID. - #newReactNativePage(page: Page) { + #newLegacyReactNativePage(page: Page) { debug(`React Native page updated to ${page.id}`); if ( this.#debuggerConnection == null || @@ -405,11 +434,11 @@ export default class Device { // We can just remember new page ID without any further actions if no // debugger is currently attached or attached debugger is not // "Reloadable React Native" connection. - this.#lastConnectedReactNativePage = page; + this.#lastConnectedLegacyReactNativePage = page; return; } - const oldPageId = this.#lastConnectedReactNativePage?.id; - this.#lastConnectedReactNativePage = page; + const oldPageId = this.#lastConnectedLegacyReactNativePage?.id; + this.#lastConnectedLegacyReactNativePage = page; this.#isReloading = true; // We already had a debugger connected to React Native page and a @@ -454,7 +483,7 @@ export default class Device { } // Allows to make changes in incoming message from device. - async #processMessageFromDevice( + async #processMessageFromDeviceLegacy( payload: {method: string, params: {sourceMapURL: string, url: string}}, debuggerInfo: DebuggerInfo, ) { @@ -555,7 +584,7 @@ export default class Device { // Allows to make changes in incoming messages from debugger. Returns a boolean // indicating whether the message has been handled locally (i.e. does not need // to be forwarded to the target). - #interceptMessageFromDebugger( + #interceptMessageFromDebuggerLegacy( req: DebuggerRequest, debuggerInfo: DebuggerInfo, socket: WS, @@ -587,7 +616,7 @@ export default class Device { processedReq.params.url.startsWith(FILE_PREFIX) && debuggerInfo.prependedFilePrefix ) { - // Remove fake URL prefix if we modified URL in #processMessageFromDevice. + // Remove fake URL prefix if we modified URL in #processMessageFromDeviceLegacy. // $FlowFixMe[incompatible-use] processedReq.params.url = processedReq.params.url.slice( FILE_PREFIX.length, @@ -665,9 +694,9 @@ export default class Device { #mapToDevicePageId(pageId: string): string { if ( pageId === REACT_NATIVE_RELOADABLE_PAGE_ID && - this.#lastConnectedReactNativePage != null + this.#lastConnectedLegacyReactNativePage != null ) { - return this.#lastConnectedReactNativePage.id; + return this.#lastConnectedLegacyReactNativePage.id; } else { return pageId; } diff --git a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js index 3cab2c701584..22abbe3384c6 100644 --- a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js +++ b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js @@ -20,6 +20,7 @@ import type { import type {IncomingMessage, ServerResponse} from 'http'; import Device from './Device'; +import nullthrows from 'nullthrows'; import url from 'url'; import WS from 'ws'; @@ -151,6 +152,7 @@ export default class InspectorProxy implements InspectorProxyQueries { deviceName: device.getName(), reactNative: { logicalDeviceId: deviceId, + type: nullthrows(page.type), }, }; } diff --git a/packages/dev-middleware/src/inspector-proxy/types.js b/packages/dev-middleware/src/inspector-proxy/types.js index 2f8053575243..e8835032ad66 100644 --- a/packages/dev-middleware/src/inspector-proxy/types.js +++ b/packages/dev-middleware/src/inspector-proxy/types.js @@ -12,13 +12,17 @@ // Page information received from the device. New page is created for // each new instance of VM and can appear when user reloads React Native // application. -export type Page = $ReadOnly<{ + +export type PageFromDevice = $ReadOnly<{ id: string, title: string, vm: string, app: string, + type?: 'Legacy' | 'Modern', }>; +export type Page = Required; + // Chrome Debugger Protocol message/event passed between device and debugger. export type WrappedEvent = $ReadOnly<{ event: 'wrappedEvent', @@ -48,7 +52,7 @@ export type GetPagesRequest = {event: 'getPages'}; // Response to GetPagesRequest containing a list of page infos. export type GetPagesResponse = { event: 'getPages', - payload: $ReadOnlyArray, + payload: $ReadOnlyArray, }; // Union type for all possible messages sent from device to Inspector Proxy. @@ -78,10 +82,11 @@ export type PageDescription = $ReadOnly<{ // Metadata specific to React Native reactNative: $ReadOnly<{ logicalDeviceId: string, + type: $NonMaybeType, }>, }>; -export type JsonPagesListResponse = $ReadOnlyArray; +export type JsonPagesListResponse = Array; // Response to /json/version HTTP request from the debugger specifying browser type and // Chrome protocol version.