From 1bcd28636faf2f4de2dca043afdc61c647ffb4a7 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 8 Nov 2023 07:19:40 -0800 Subject: [PATCH] Handle source file / source map fetch errors correctly, add tests (#41342) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41342 While rewriting `Debugger.getScriptSource` messages to fetch code and source map over HTTP, we weren't checking the status code of the fetch calls. This diff fixes that and adds corresponding tests (as well as for the filesystem error case). Changelog: [Internal] Reviewed By: robhogan Differential Revision: D51013054 fbshipit-source-id: 58e7bb9fcd6a3cf92329b43fb8a139093c80d305 --- .../InspectorProxyCdpRewritingHacks-test.js | 134 ++++++++++++++++++ .../src/inspector-proxy/Device.js | 3 + 2 files changed, 137 insertions(+) diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js index ce2438ddd0f..43570d15271 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js @@ -89,6 +89,57 @@ describe.each(['HTTP', 'HTTPS'])( } }); + test('handling of failure to fetch source map', async () => { + const {device, debugger_} = await createAndConnectTarget( + serverRef, + autoCleanup.signal, + { + app: 'bar-app', + id: 'page1', + title: 'bar-title', + vm: 'bar-vm', + }, + ); + try { + const scriptParsedMessage = await sendFromTargetToDebugger( + device, + debugger_, + 'page1', + { + method: 'Debugger.scriptParsed', + params: { + sourceMapURL: `${serverRef.serverBaseUrl}/source-map-missing`, + }, + }, + ); + + // We don't rewrite the message in this case. + expect(scriptParsedMessage.params.sourceMapURL).toEqual( + `${serverRef.serverBaseUrl}/source-map-missing`, + ); + + // We send an error through to the debugger as a console message. + expect(debugger_.handle).toBeCalledWith( + expect.objectContaining({ + method: 'Runtime.consoleAPICalled', + params: { + args: [ + { + type: 'string', + value: expect.stringMatching('Failed to fetch source map'), + }, + ], + executionContextId: 0, + type: 'error', + }, + }), + ); + } finally { + device.close(); + debugger_.close(); + } + }); + describe.each(['10.0.2.2', '10.0.3.2'])( '%s aliasing to and from localhost', sourceHost => { @@ -326,6 +377,89 @@ describe.each(['HTTP', 'HTTPS'])( debugger_.close(); } }); + + test.each(['url', 'file'])( + 'reports %s fetch error back to debugger', + async resourceType => { + const {device, debugger_} = await createAndConnectTarget( + serverRef, + autoCleanup.signal, + { + app: 'bar-app', + id: 'page1', + title: 'bar-title', + vm: 'bar-vm', + }, + ); + try { + await sendFromTargetToDebugger(device, debugger_, 'page1', { + method: 'Debugger.scriptParsed', + params: { + scriptId: 'script1', + url: + resourceType === 'url' + ? `${serverRef.serverBaseUrl}/source-missing` + : '__fixtures__/mock-source-file.does-not-exist', + startLine: 0, + endLine: 0, + startColumn: 0, + endColumn: 0, + hash: createHash('sha256').update('foo').digest('hex'), + }, + }); + const response = await debugger_.sendAndGetResponse({ + id: 1, + method: 'Debugger.getScriptSource', + params: { + scriptId: 'script1', + }, + }); + + // We mark the request as failed. + expect(response).toEqual({ + id: 1, + result: { + error: { + message: expect.stringMatching( + `Failed to fetch source ${resourceType}`, + ), + }, + }, + }); + + // We also send an error through to the debugger as a console message. + expect(debugger_.handle).toBeCalledWith( + expect.objectContaining({ + method: 'Runtime.consoleAPICalled', + params: { + args: [ + { + type: 'string', + value: expect.stringMatching( + `Failed to fetch source ${resourceType}`, + ), + }, + ], + executionContextId: 0, + type: 'error', + }, + }), + ); + + // The device does not receive the getScriptSource request, since it + // is handled by the proxy. + expect(device.wrappedEventParsed).not.toBeCalledWith({ + pageId: 'page1', + wrappedEvent: expect.objectContaining({ + method: 'Debugger.getScriptSource', + }), + }); + } finally { + device.close(); + debugger_.close(); + } + }, + ); }); }, ); diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index 35430f9b8a4..fae783cb416 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -680,6 +680,9 @@ export default class Device { // $FlowFixMe[incompatible-call] Suppress arvr node-fetch flow error const response = await fetch(url); + if (!response.ok) { + throw new Error('HTTP ' + response.status + ' ' + response.statusText); + } const text = await response.text(); // Restrict the length to well below the 500MB limit for nodejs (leaving // room some some later manipulation, e.g. base64 or wrapping in JSON)