From 82b945baa4853ac62d2c5f54dba97d27d08ace2a Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 25 Mar 2024 05:41:41 -0700 Subject: [PATCH] Inspector proxy: preserve ordering of messages from device (#43639) Summary: Currently, the `react-native/dev-middleware` inspector proxy intercepts `Debugger.scriptParsed` notifications from the target and replaces `sourceMapURL` with a data uri, via an async fetch from Metro. During this async fetch, other notifications from the debugger may pass through the proxy, which results in the frontend receiving them before `Debugger.scriptParsed`. This reordering causes problems in breakpoint resolution and pausing, because `Debugger.breakpointResolved` and `Debugger.paused` events may reference `scriptId`s unknown to the frontend while the corresponding `Debugger.scriptParsed` is delayed. In particular, breakpoint UI state and backend state can fall out of sync, and breakpoints hit may open to the incorrect source location. This diff modifies the proxy to use a simple per-target promise queue to ensure messages are handled in the order they were received from the target. Changelog: [General][Fixed] Fix breakpoints opening to incorrect location or disappearing from debugger frontend UI. Differential Revision: D55200617 --- .../InspectorProxyCdpRewritingHacks-test.js | 46 ++++++++++++++++ .../src/inspector-proxy/Device.js | 52 +++++++++++-------- 2 files changed, 76 insertions(+), 22 deletions(-) diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js index 51545ecd9be702..b2d83c053d32bc 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js @@ -95,6 +95,52 @@ describe.each(['HTTP', 'HTTPS'])( } }); + test('async source map fetching does not reorder events', async () => { + serverRef.app.use( + '/source-map', + serveStaticJson({ + version: 3, + // Mojibake insurance. + file: '\u2757.js', + }), + ); + const {device, debugger_} = await createAndConnectTarget( + serverRef, + autoCleanup.signal, + { + app: 'bar-app', + id: 'page1', + title: 'bar-title', + vm: 'bar-vm', + }, + ); + try { + await Promise.all([ + sendFromTargetToDebugger(device, debugger_, 'page1', { + method: 'Debugger.scriptParsed', + params: { + sourceMapURL: `${serverRef.serverBaseUrl}/source-map`, + }, + }), + sendFromTargetToDebugger(device, debugger_, 'page1', { + method: 'Debugger.aSubsequentEvent', + }), + ]); + expect(debugger_.handle).toHaveBeenNthCalledWith(1, { + method: 'Debugger.scriptParsed', + params: { + sourceMapURL: expect.stringMatching(/^data:/), + }, + }); + expect(debugger_.handle).toHaveBeenNthCalledWith(2, { + method: 'Debugger.aSubsequentEvent', + }); + } finally { + device.close(); + debugger_.close(); + } + }); + test('handling of failure to fetch source map', async () => { const {device, debugger_} = await createAndConnectTarget( serverRef, diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index b4ef0ad731cad8..f5f779cff3731c 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -76,6 +76,11 @@ export default class Device { // Package name of the app. #app: string; + // Sequences async processing of messages from device to preserve order. Only + // necessary while we need to accommodate #processMessageFromDeviceLegacy's + // async fetch. + #messageFromDeviceQueue: Promise = Promise.resolve(); + // Stores socket connection between Inspector Proxy and device. #deviceSocket: WS; @@ -135,20 +140,26 @@ export default class Device { // $FlowFixMe[incompatible-call] this.#deviceSocket.on('message', (message: string) => { - const parsedMessage = JSON.parse(message); - if (parsedMessage.event === 'getPages') { - // There's a 'getPages' message every second, so only show them if they change - if (message !== this.#lastGetPagesMessage) { - debug( - '(Debugger) (Proxy) <- (Device), getPages ping has changed: ' + - message, - ); - this.#lastGetPagesMessage = message; - } - } else { - debug('(Debugger) (Proxy) <- (Device): ' + message); - } - this.#handleMessageFromDevice(parsedMessage); + this.#messageFromDeviceQueue = this.#messageFromDeviceQueue.then( + async () => { + const parsedMessage = JSON.parse(message); + if (parsedMessage.event === 'getPages') { + // There's a 'getPages' message every second, so only show them if they change + if (message !== this.#lastGetPagesMessage) { + debug( + '(Debugger) (Proxy) <- (Device), getPages ping has changed: ' + + message, + ); + this.#lastGetPagesMessage = message; + } + } else { + debug('(Debugger) (Proxy) <- (Device): ' + message); + } + if (parsedMessage.event === 'wrappedEvent') { + } + await this.#handleMessageFromDevice(parsedMessage); + }, + ); }); // Sends 'getPages' request to device every PAGES_POLLING_INTERVAL milliseconds. this.#pagesPollingIntervalId = setInterval( @@ -395,7 +406,7 @@ export default class Device { // In the future more logic will be added to this method for modifying // some of the messages (like updating messages with source maps and file // locations). - #handleMessageFromDevice(message: MessageFromDevice) { + async #handleMessageFromDevice(message: MessageFromDevice) { if (message.event === 'getPages') { this.#pages = new Map( message.payload.map(({capabilities, ...page}) => [ @@ -498,16 +509,13 @@ export default class Device { return; } - // Wrapping just to make flow happy :) - // $FlowFixMe[unused-promise] - this.#processMessageFromDeviceLegacy( + await this.#processMessageFromDeviceLegacy( parsedPayload, debuggerConnection, pageId, - ).then(() => { - const messageToSend = JSON.stringify(parsedPayload); - debuggerSocket.send(messageToSend); - }); + ); + const messageToSend = JSON.stringify(parsedPayload); + debuggerSocket.send(messageToSend); } else { debuggerSocket.send(message.payload.wrappedEvent); }