Commit Graph

132 Commits

Author SHA1 Message Date
Rob Hogan
69400be4fc dev-middleware: Only rewrite hostnames if they match device connection hosts (#47685)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47685

Currently, we assume any URL with a hostname of `10.0.2.2` or `10.0.3.2` (device-relative) is eligible for rewriting to `localhost` (frontend-relative), because we assume the device is an Android emulator. We rewrite these URLs between device and dev machine so that the rewritten URLs are reachable from the dev machine.

This diff narrows this logic so that we'll only rewrite URLs where the hostname matches the pre-existing list *and* this matches the host the device is actually connected on, according to its headers from the original connection.

The main motivation for this change is to unblock removing assumptions about device-reachable vs server-reachable hosts. Later in the stack we'll drop the hardcoded listing of `10.0.2.2` etc in favour of identifying URLs that target the dev server, from whatever network.

There's also an edge case fix here that `10.0.2.2` etc might actually refer to a remote LAN server, and not be an Android emulator's alias for for an emulator host.

Changelog:
[General][Fixed] RN DevTools: Don't assume 10.0.2.2 is an alias for localhost unless it's used to establish a connection to the server

Reviewed By: huntie

Differential Revision: D66058704

fbshipit-source-id: bad28717b0c9b1ca43e2ea3391cef13f87892e6c
2024-11-20 06:56:21 -08:00
Rob Hogan
99767d43b0 Remove URL.canParse, fix older Node compatibility
Summary:
`URL.canParse` was added to Node.js [in v18.17.0](9586cd0cfd).

Internally, we have call sites using earlier versions of Node.js.

Changelog:
[General][Fixed]: dev-middleware: Remove URL.canParse, restore compat with Node < 18.17

Reviewed By: rshest

Differential Revision: D66158204

fbshipit-source-id: e7a846b536e76672cea9afd5bdc5041d50a8b587
2024-11-19 01:50:49 -08:00
Rob Hogan
d1b0e9a30b dev-middleware: Use serverBaseUrl for local->server fetches (#47653)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47653

## Context

Currently, when `nativeSourceCodeFetching == false`, `inspector-proxy` attempts to pre-fetch source maps, given the URL from a `Debugger.scriptParsed` event, and embeds them into `Debugger.scriptParsed`'s `sourceMapURL` using a data URI.

This was originally to support frontends that did not perform HTTP requests or were blocked (eg by CORS), but we're retaining it for the moment because it's more performant than lazy loading the source map.

Similarly, we perform middleware->server fetches to respond to `Debugger.getScriptSource` events.

To make these fetches for URLs that target `10.0.2.2` (ie, addressable from within an Android emulator) (etc), we rewrite `10.0.2.2`->`localhost` and perform a `fetch` from the Node process running dev-middleware.

## The problem

Consider a setup where:
 - Metro is running on a remote server, listening on `8081`.
 - Dev machine tunnels `localhost:8082` -> remote `8081`.
 - An app is running on an Android emulator on the dev machine, with bundle URL configured to `10.0.2.2:8082`.

In this case, we'll rewrite `10.0.2.2:8082` to `localhost:8082`, which *is* reachable and correct from the dev machine, but *not* from the machine where Metro is running, so the `fetch` of a source map from the inspector proxy will fail.

## Motivation

This might seem like a niche case, but it's part of fixing a series of unsafe assumptions that currently prevent us from running DevTools on an arbitrary port.

## This fix

Preserve the current behaviour (simple `10.0.2.2`<=>`localhost`) for URLs sent to the frontend, but construct a separate, server-relative URL, using the configured `serverBaseUrl`, for `fetch` calls within dev-middleware.

Changelog:
[General][Fixed] RN DevTools: Fix fetching sources and source maps when the dev-server is remote and not tunnelled via the same port+protocol.

Reviewed By: huntie

Differential Revision: D65993910

fbshipit-source-id: a0cdcf1644e97a2af3d8583f2da2aaa51276f68c
2024-11-18 16:23:53 -08:00
Rob Hogan
da62721061 dev-middleware: refactor tests to use undici.request (#47675)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47675

Use `request` over `fetch` in `dev-middleware`'s tests.

This is required by the next diff in the stack to spoof the `Host` header for testing purposes, which isn't permitted by the `fetch` spec.

The return type is a bit different (eg `statusCode` vs `status`, no `ok` prop), but the modifications needed are pretty straightforward.

Changelog: [Internal]

Reviewed By: huntie

Differential Revision: D66005427

fbshipit-source-id: f311b0188d6d0ec220a037774fca78df5373163a
2024-11-18 15:14:02 -08:00
Rob Hogan
acf384a72e dev-middleware: Redefine "serverBaseUrl" as server-relative, '/json/list' by requestor (#47628)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47628

`serverBaseUrl` is currently documented as:

> The base URL to the dev server, as addressible from the local developer machine

This is problematic in general because `dev-middleware` on a server doesn't necessarily know about where clients might be reaching it from, how tunnels or port-forwards are set up, etc., and this can change over the lifetime of the server and vary between clients.

Indeed, our own use of `serverBaseUrl` from both `community-cli-plugin` and internally simply sets it to the host and port the dev server is listening on - ie it's the address of the dev server accessible *from the server*.

This PR changes the docs, redefining `serverBaseUrl`, to match the way we currently specify it.

One usage where we *do* want the previously documented behaviour is in responses to `/json/list` (`getPageDescriptions`) where the URLs in the response should be reachable by a browser requesting `/json/list`.

Here, we use the request (host header, etc.) to attempt to get working base URL.

History:
It should be mentioned that this is the latest in a series of changes like this:
 - https://github.com/facebook/react-native/pull/39394
 - https://github.com/facebook/react-native/pull/39456

Learning from those:
 - This change does *not* break Android emulators, which routes `10.0.2.2` to localhost, or other routed devices, because `/open-debugger` still uses server-relative URLs, and now formally delegates to `BrowserLauncher` to decide what to do with those URLs (internally, VSCode / `xdg-open` handles port forwarding)
 - Middleware configuration is no longer required to specify how it is reachable from clients.

This sets up some subsequent changes for more robust handling of tunnelled connections.

Changelog:
[General][Breaking] dev-middleware: Frameworks should specify `serverBaseUrl` relative to the middleware host.

Reviewed By: huntie

Differential Revision: D65974487

fbshipit-source-id: 1face8fc7715df387f75b329e80932d8543ee419
2024-11-18 13:46:50 -08:00
Rob Hogan
ca9c56329f dev-middleware: Fix reliance on adb reverse when loading sources on Android (#47652)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47652

## Background
When the `nativeSourceCodeFetching` capability is disabled, `inspector-proxy` rewrites URLs exchanged over CDP between device and frontend so that URLs are addressable from CDT - in particular, when using an Android emulator `10.0.2.2` (host's address from within the emulator) is rewritten to and from `localhost` (the equivalent address reachable from the host).

Previously - before we implemented `Network.loadNetworkResource`, or on old frontends that don't attempt to use that method -  this worked reasonably well. A `fetch` from CDT to Metro would succeed on the rewritten URL.

## Problem
Since we implemented `Network.loadNetworkResource`, but disabled the `nativeSourceCodeFetching` capability, source fetching is broken under Android emulators. We're rewriting URLs to be frontend-relative, but then attempting to fetch them through the device, because as far as CDT is aware, `Network.loadNetworkResource` should still be tried first.

When `Network.loadNetworkResource` responds with a CDP *error*, CDT falls back to a local fetch (which would work), but when it responds with a CDP *result* of `success: false`, there is no fallback.

## Fix
This diff adds an interception guarded behind `nativeSourceCodeFetching == false`, which rejects any calls to `Network.loadNetworkResource` with a CDP error. This restores the previous behaviour from before `Network.loadNetworkResource` was implemented at all.

NOTE: An alternative approach would be to rewrite URLs back to device-relative for `Network.loadNetworkResource`, but IMO it's more correct for the frontend to respect that the device is asserting that it doesn't have that capability, and not to try to use it.

Changelog:
[Android][Fixed] RN DevTools: Fix source loading when using an Android emulator connecting to a dev server on the host.

Reviewed By: huntie

Differential Revision: D66074731

fbshipit-source-id: f2050c014cd5cfa546bff5e9d0412413a5daff35
2024-11-18 12:19:19 -08:00
Nicola Corti
50e38cc9f1 Bump serve-static to 1.16.2 to fix CVE-2024-43800 (#47289)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47289

Bump serve-static to 1.16.2 to fix CVE-2024-43800

Changelog:
[General] [Changed] - Bump serve-static to 1.16.2 to fix CVE-2024-43800

Reviewed By: cipolleschi

Differential Revision: D65146027

fbshipit-source-id: 2876a78f195230cc4cdaa14b0076ecbecc1ee6e4
2024-10-30 09:08:55 -07:00
Alex Hunt
6f421230c1 Drop appId from /open-debugger calls, disable matching for modern targets (#47120)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47120

Fixes no-op behaviour of the "Open DevTools" Dev Menu item (bug on `main` introduced with D63329456).

This was caused by a change to the `description` field contents in our CDP `/json/list` response, when under Fusebox. In the `/open-debugger` call from the Dev Menu, we were still using the older `appId` param.

This did not affect `j` to debug, which uses the `target` param.

{F1937186832}

Changes:

In short: Matching against the `description` string is now fully eliminated for modern debugger targets.

- Update native Dev Menu implementation to omit `appId` parameter (`device` param alone is sufficient and fully precise on these platforms).
- Update `/open-debugger` implementation to ignore the `appId` parameter for modern targets, and document this in the `dev-middleware` README.

Changelog: [Internal]

Reviewed By: robhogan

Differential Revision: D64597581

fbshipit-source-id: 46f536e7d0a4ececab0d52f4c0704e8698466cd0
2024-10-18 11:02:51 -07:00
Tim Yung
c9ea05552f RN: Fix lint/sort-imports Errors (#47109)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47109

Fixes the `lint/sort-imports` errors that are now surfaced after fixing the lint configuration.

For a couple files, I added lint suppressions instead because the unsorted import ordering is important due to interleaved calls with side effects.

Changelog:
[Internal]

Reviewed By: GijsWeterings

Differential Revision: D64569485

fbshipit-source-id: 26415d792e2b9efe08c05d1436f723faae549882
2024-10-18 04:07:02 -07:00
Edmond Chui
d11954e800 add appId to Telemetry (#47099)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47099

Changelog: [Internal]

As discussed in D64547367, recent changes caused `description` to stop reporting `appId`.

This impacted Telemetry's "top apps" dashboards.
{F1936011487}

Reviewed By: hoxyq

Differential Revision: D64548348

fbshipit-source-id: 46af650a89e2bef7be000482e7b774f56f84cac3
2024-10-17 12:47:05 -07:00
Edmond Chui
0d591d9dc3 add appId as a standalone field (#47098)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47098

Changelog: [Internal]

D63329456 updated the `description` field to be human-readable.
Unfortunately, InspectorProxy relies on this field to compare the incoming `/open-debugger` calls.

hoxyq discovered the symptom of Fusebox failing to launch with `No compatible apps connected. React Native DevTools can only be used with the Hermes engine.` in Metro

Reviewed By: hoxyq

Differential Revision: D64547367

fbshipit-source-id: deed6851f3ede2c74be2b492def1eba6e58c43e6
2024-10-17 12:47:05 -07:00
Cedric van Putten
eeb6122f39 fix(dev-middleware): respond with status code 200 when launching RNDT (#46814)
Summary:
This fixes an issue where `POST /open-debugger?appId&device&target` does not return a proper status code, meaning that the request will never be answered and clients might hang until the request timeout is hit.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[GENERAL] [FIXED] - Respond with status code `200` when successfully launching RNDT

Pull Request resolved: https://github.com/facebook/react-native/pull/46814

Test Plan:
- `curl -v -X POST "<deviceUrl>"`
- This should show a proper response for the request.

before | after
 --- | ---
![image](https://github.com/user-attachments/assets/5b820acd-1168-4642-90ec-f2eeec0afc16) | ![image](https://github.com/user-attachments/assets/82bb2a6c-3c7b-483f-a4a1-ad00e5ca0178)

Reviewed By: NickGerleman

Differential Revision: D63837025

Pulled By: huntie

fbshipit-source-id: ac72fc793e015f0eec498f4a35b4fb9e301c5b32
2024-10-04 02:45:59 -07:00
Alex Hunt
c430083fa0 Update modern inspector targets to directly send CDP title and description (#46780)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46780

This is primarily a debugger server change to better-align the `title` and `description` fields (visible in via the CDP `/json/list` endpoint), reporting them directly from the device.

For React Native users, the net effect of this change is to improve/align the display name for each debugger target in the CLI multi-select menu (`j` to debug). This change may also be useful for discovery in non-Fusebox frontends such as VS Code (`vscode-expo` extension).

Changes:
- Rename `title` prop on `InspectorPageDescription`/`IInspector::addPage` to `description` (no call site changes).
- Add `deviceName` param to `InspectorPackagerConnection`.
- Move the page `description` to the `description` JSON field.
- Update `InspectorPackagerConnection::Impl::pages()` to return new `title` and `description` fields.
- Align `OpenDebuggerKeyboardHandler` to display `title` field.
- Deprecate the nonstandard `deviceName` field.

**Before**

```
[
    {
        "id": "3c9f24bedab0e73fca6a1b295030e7af9346a8c0-1",
        "title": "React Native Bridgeless [C++ connection]",
        "description": "com.facebook.RNTester",
         ...
    }
```

The `description` field was previously the closest thing we had to a target identifier. Today, this is not needed, since we have the stable `reactNative.logicalDeviceId` field.

**After**

```
[
    {
        "id": "3c9f24bedab0e73fca6a1b295030e7af9346a8c0-1",
        "title": "com.facebook.RNTester (iPhone 16 Pro)",
        "description": "React Native Bridgeless [C++ connection]",
         ...
    }
```

The `title` field is now more human readable and aligned with what we render in the window title of React Native DevTools. The `description` field describes the type of debugger target (the specific React Native implementation).

Changelog: [Internal]

Reviewed By: vzaidman

Differential Revision: D63329456

fbshipit-source-id: cfe98f77e31c729431005925cfc66e2780ef8c72
2024-10-02 07:30:31 -07:00
Riccardo Cipolleschi
120558c0cd Bump main to 0.77
Summary:
This change bumps the React Native version in main to 0.77

bypass-github-export-checks

## Changelog:
[General][Changed] - Bump main to 0.77-main

## Facebook:
generated by running `js1 publish react-native 0.77.0-main`

Reviewed By: cortinico

Differential Revision: D62575939

fbshipit-source-id: 6d239fca2eed6cfe51f8c37f78d8dc8730c18b8c
2024-09-12 09:23:23 -07:00
Edmond Chui
5bbf5a4878 default embedder script for open-source (#46302)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46302

Changelog:
[General][Fixed] - Removed noisy ENOENT error message upon launching the debugger

As described in T200199544, Metro terminal in open-source would show an error message for the missing embedder script.

In this diff, we add a response of an empty file to open-source (no-op)

Reviewed By: hoxyq

Differential Revision: D62103015

fbshipit-source-id: 219bc398b7786527db00528cca175adc13a527a0
2024-09-02 10:33:57 -07:00
Alex Hunt
3077db32ef Update references to debugger frontend (#46232)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46232

Follows D61711818.

Changelog: [Internal]

Reviewed By: robhogan

Differential Revision: D61852414

fbshipit-source-id: a2d7f135a3ff03751c6d91978bebee007272f306
2024-08-28 03:20:17 -07:00
Alex Hunt
94e7a87f23 Remove --experimental-debugger option (#46231)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46231

Removes this option from `npx react-native start`. Flipper will no longer be the default launch flow in 0.76.

The debugger frontend variant remains controlled by `target.reactNative.capabilities?.prefersFuseboxFrontend`. This will always be Fusebox, since D60893243.

Changelog:
[General][Changed] Remove `--experimental-debugger` option from start command

Reviewed By: robhogan

Differential Revision: D61852415

fbshipit-source-id: 3351f0e12c24717916a70dd1ea28f8690bb5509f
2024-08-28 03:20:17 -07:00
Alex Hunt
773fc8d0f1 Remove useFuseboxInternalBranding experiment (#46230)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46230

Follows https://github.com/facebookexperimental/rn-chrome-devtools-frontend/pull/101.

Changelog: [Internal]

Reviewed By: hoxyq

Differential Revision: D61850578

fbshipit-source-id: 9283e74dd5fddac001f9748115dc8e85b8d0d491
2024-08-27 07:10:02 -07:00
Cedric van Putten
30a3e6e8df refactor(dev-middleware): drop node-fetch in favor of Node built-in fetch (#45227)
Summary:
Node 22 doesn't work well with `node-fetch@2`, as one of their polyfills is using the deprecated `punycode` module. This causes unnecessary warnings like:

<img width="986" alt="image" src="https://github.com/facebook/react-native/assets/1203991/13f66c5b-b6f4-4894-8576-ca9631d93f77">

Instead of upgrading to the [much larger `node-fetch@3`](https://packagephobia.com/result?p=node-fetch%403.3.2), this change drops `node-fetch` in favor of Node's own built-in `fetch` implementation (using [undici](https://github.com/nodejs/undici#readme)).

> Note, `react-native/dev-middleware` [already has the `engines.node >= 18`](c7988c9c82/packages/dev-middleware/package.json (L38-L40)) (which is required for fetch).

## Changelog:

[GENERAL] [CHANGED] - Drop `node-fetch` in favor of Node's built-in fetch from `undici` in `react-native/dev-middleware`

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: https://github.com/facebook/react-native/pull/45227

Test Plan: See CI for passing tests

Reviewed By: NickGerleman

Differential Revision: D59202017

Pulled By: robhogan

fbshipit-source-id: 15da5d5602e63958b8a3dc581dc1512649f16c12
2024-07-03 09:14:02 -07:00
Alex Hunt
ebf1a7b79b Bump packages for next major release (#45015)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45015

Set package versions to `0.76.0-main` (`0.75-stable` branch was cut).

Changelog: [Internal]

Reviewed By: cipolleschi

Differential Revision: D58687399

fbshipit-source-id: 27ed987e4557705845d57d64e7547cddbd982a03
2024-06-26 07:59:49 -07:00
Gijs Weterings
80cfacef78 Upgrade ws from 6.2.2 to 6.2.3 (#45130)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45130

Changelog: [General][Security] Update ws from 6.2.2 to 6.2.3 (CVE-2024-37890)

6.2.3 is a patch to mitigate a dos vuln https://github.com/websockets/ws/releases/tag/6.2.3

Reviewed By: hoxyq

Differential Revision: D58946681

fbshipit-source-id: ff73f31ffd9b14392e3bea4e616cb5f4c9b8d13f
2024-06-25 17:30:10 -07:00
Rob Hogan
88ba9a6042 Inspector: Support /open-debugger specifying target param (#45138)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45138

Add a new `/open-debugger` endpoint format that allows specifying `target` - the proxy-unique target `id`. This is logically equivalent to specifying both device and page.

Changelog:
[General][Added]: Inspector: Support `/open-debugger` specifying `target` param

Reviewed By: hoxyq

Differential Revision: D58950622

fbshipit-source-id: 9665f8a24ba2bb0561cc3c693dfb84bfffdeb4a4
2024-06-25 06:28:57 -07:00
Rob Hogan
c7970379a1 Inspector: Enforce device and appId filters if both are given to /open-debugger (#45140)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45140

Previously, if the `/open-debugger` endpoint was provided with both `device` and `appId` query params, we would:
 - Try to find a target with a matching `device` (note that these logical "devices" are unique per-app) - if found, use it. Otherwise,
 - Try to find a target with a matching `appId` - if found, use that.

This could go "wrong" in two ways:
 - If a `device` is given with a spurious `appId`, we'd open to a target with an `appId` differing from the one specified.
 - If the `device` has gone away but there is a different target with the same app, we'd use that as a fallback (right app, wrong device).

This applies the filters more strictly so that if both are given, both must match.

Changelog:
[General][Changed]: Inspector: Enforce device and appId filters if both are given to /open-debugger

Reviewed By: hoxyq

Differential Revision: D58951952

fbshipit-source-id: a95f1160e5c88f957445058f3273e922a5d28c1e
2024-06-25 06:28:57 -07:00
Moti Zilberman
a7adfef0bb Reject debugger connections to unknown page IDs (#45148)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45148

Changelog: [General][Fixed] Reconnecting to an unknown inspector page no longer results in a zombie connection

TSIA

Reviewed By: hoxyq

Differential Revision: D58954759

fbshipit-source-id: 99c5caccc3cc917e0691e94326c7a35874f9a385
2024-06-24 12:59:53 -07:00
Rob Hogan
2a6a895b17 Debugger: j opens debugger on most recently connected target (#45060)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45060

Currently, `j`, (i.e., `/open-debugger` with no parameters), connects the "first available" target, which in practice is the first page of the first connected device still connected.

In the absence of a target selection UI, a better guess at user intent is to use the *latest* target (most recently added page of most recently connected device).

Also slightly reduces CLI noise by not claiming that we're launching a debugger when there's no target, and not qualifying which target when there's only one.

Changelog:
[General][Changed] Debugger: `j` opens most recent (not first) target.

Reviewed By: huntie

Differential Revision: D58736151

fbshipit-source-id: 3d106a1fa958f9e5c91b16e04075609e1abf6e97
2024-06-19 11:28:36 -07:00
Rob Hogan
53951d7fec Debugger: Make /json/list ordered (#45069)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45069

Currently, `/json/list` returns pages within each device in the iteration order of a C++ `unordered_map`, which doesn't tell us anything useful. Page IDs happen to be sequential, but only as an implementation detail.

Change this contract so that we guarantee ordering reflects addition order, allowing clients to consistently select e.g. most recently added page for a given device.

The implementation of this is as simple as switching from an `unordered_map` to a key-ordered`map`, because we already assign keys (page IDs) with an incrementing integer. Within the inspector proxy, devices already use an insertion (connection)-ordered JS `Map`, so we just document this guarantee.

Changelog:
[General][Changed] Debugger: Make `/json/list` return connection-addition-ordered targets.

Reviewed By: huntie

Differential Revision: D58735947

fbshipit-source-id: 7a132cc5e750475792a2b845afc9a42424690bf1
2024-06-19 11:28:36 -07:00
Moti Zilberman
e55ea2daf1 Fix debugger handoff logic to prevent "zombie" state (#45035)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45035

Changelog: [General][Fixed] Avoid a zombie state when opening a second debugger frontend concurrently.

The problem here was that we were sending proxy-protocol messages to the device in the wrong order (`disconnect` *after* `connect`):

 {F1701266597}

The root cause was that we were depending on the outgoing debugger socket's async `close` event to trigger sending the `disconnect` message to the device. This would happen after we'd already (synchronously) sent the `connect` message.

With this diff, we send the `disconnect` message synchronously with calling `close()` on the debugger socket, which fixes the ordering problem at the source. To avoid sending duplicate `disconnect` messages (e.g. one before calling `close()` and one from the `close` event handler), we store some extra state on `Device` (`#connectedPageIds`).

Reviewed By: robhogan, huntie

Differential Revision: D58730634

fbshipit-source-id: 0f54af2e4f8071a8f6d97cc9e3d8a4ea89a46f43
2024-06-18 09:39:35 -07:00
Moti Zilberman
9cca4c1ec1 Add test for connecting multiple debuggers to a single page, reproing "zombie" state (#45034)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45034

Changelog: [Internal]

TSIA

Reviewed By: hoxyq

Differential Revision: D58728261

fbshipit-source-id: 32d4b05d69f88e68b5327dc5c8e837291e3d7726
2024-06-18 09:39:35 -07:00
Moti Zilberman
6695b6e3b8 Reuse Device instances when handing off connections based on device ID (#45027)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45027

Changelog: [Internal]

Changes the device ID collision handling logic to reuse `Device` instances instead of creating new ones. This enables further refactoring of `Device` to improve session state isolation.

Reviewed By: hoxyq

Differential Revision: D58724884

fbshipit-source-id: bc11ce45ce8c80c58c32dcd1b07b28f1d1753a62
2024-06-18 09:39:35 -07:00
Moti Zilberman
db2c44a7eb Make inspector-proxy protocol types mutable by default (#45021)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45021

Changelog: [Internal]

TSIA, hoisted from an upcoming refactor of `Device` in inspector-proxy.

Reviewed By: hoxyq

Differential Revision: D58723496

fbshipit-source-id: af272b08bc280d681bd7e1adb60d2a411a549864
2024-06-18 08:25:32 -07:00
Moti Zilberman
43d69ee26c Fix ReactNativeReloads test case (#45011)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45011

Changelog: [Internal]

Fixes an inspector-proxy test case that was (silently) incorrect. This is in preparation for an upcoming rewrite of the core of inspector-proxy to more strictly isolate session state, which causes the incorrect test to fail.

Reviewed By: hoxyq

Differential Revision: D58193527

fbshipit-source-id: bdc27179210117ca9249b272f2e4aff19ba8a06c
2024-06-18 02:53:18 -07:00
Moti Zilberman
4c6bff01b3 Fix bug in device ID collision handling, add tests (#45010)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/45010

D46482492 added logic for handing off state across "device" connections that have the same ID. This logic currently has no test coverage. It also contains a bug whereby the new device's pages are removed from the target listing endpoint (`/json`) when the *old* device's socket is closed.

This diff adds tests and fixes the bug.

Changelog: [General][Fixed] inspector-proxy no longer accidentally detaches connected devices.

## Next steps

It seems that the device ID handoff logic exists to paper over a deeper problem with the inspector proxy protocol (or its implementation in React Native): The React Native runtime should not routinely be creating new "device" connections without tearing down previous ones.

In followup diffs, I'll explore changing this behaviour for Fusebox, based on the new test coverage.

Reviewed By: robhogan

Differential Revision: D51013056

fbshipit-source-id: e0c17678cc747366a3b75cef18ca2a722fc93acd
2024-06-17 09:56:14 -07:00
Edmond Chui
20462ca984 fix timing of terminating an unresponsive debugger socket (#44811)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44811

Changelog:
[General][Fixed] - Debugger frontend socket-termination countdown now begins after the ping message is actually sent

The debugger is currently disconnected if a ping-pong message is missed.

This causes the debugger to be unusable if it happens to be lagging, e.g. when the initialisation is competing with the flood of log spam T191394188

There are a few ways to fix this as discused with motiz88 and robhogan:

1. Ensure the websocket has a chance to respond, e.g. in via web worker
1. Lengthen the time allowed for the pong resopnse

I've done some digging to find the root cause of the UI being blocked in CDT, However, profiling shows that most of the work is not simple to break up, i.e. the number of expensive re-layout calls. Diving into that rabbit hole could mean accidentally writing React.

Because we ping every 10 seconds, we could get un/lucky where CDT happens to be busy _at that exact moment_, making this a flaky symptom to fix, even if we lengthen the allowed time-to-respond.

# V2+

So upon further investigation, CDT websocket is actually responding to the pings in due time:

{F1679132204}

(CDT doesn't show the ping/pong API as frames, so a custom tick/tock message was used to visualise the timing)

Over here in dev-middleware, we currently start a timeout to terminate the socket after sending the ping:

https://www.internalfb.com/code/fbsource/[813870db697a8701f2512d25a7fed730f0ec6ed9]/xplat/js/react-native-github/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js?lines=306-307

If CDT doesn't respond in time, websocket would be terminated.

But we saw CDT respond immediately above, even during the log spam, so the delay must be coming from somewhere else.

The intuition is that during the log-spam, the middleware takes a perf hit too when it's processing the spam from the device and forwarding it to the CDT websocket.

We can confirm this by passing a "sent" callback via `socket.ping(cb)`:

9bdb58070d/lib/websocket.js (L246-L254)

This gives us the timing between calling `socket.ping()` and when the ping is actually sent.

Regular, stress-free operation without log-spam shows most pings are sent within the same millisecond:

 {F1679223326}

With the pong response grace period at 5 seconds, there's plenty of time for CDT to `pong` back. That's why it has been working in most cases.

However, during the log-spam, we easily see this send-sent delay over 5 seconds. In extreme cases, almost 30 seconds would have passed before middleware sent a message to CDT, which then responded under 2 seconds:

 {F1679163335}

This means while CDT is getting flooded and has observable lag in the UI, the smoking gun is actually the middleware.

Digging a little deeper, we know that incoming messages from the target goes into a Promise queue, including the console logs:

https://www.internalfb.com/code/fbsource/[d5d312082e9c]/xplat/js/react-native-github/packages/dev-middleware/src/inspector-proxy/Device.js?lines=155-157

This means during the flood of logs from the target, the Promise queue keeps getting chained rapidly for each message.

Meanhile, the `ws` lib uses the underlying NodeJS `Socket.write` method for `ping(…)` and `send(…)`:

9bdb58070d/lib/sender.js (L349)

…which is guaranteed to fire the callback asynchronously:

https://github.com/nodejs/help/issues/1504#issuecomment-422879594

Promise queue is in the macro task queue, which gets priority before the micro task queue. So if the Promise queue is not cleared yet, the websocket queue will have a hard time getting executed in time – explaining the extreme send-sent durations during a log spam.

The fix is simple:

1. Start the terminate-socket-timer until the `ping` is actually sent
1. Treat any incoming message (along with `pong`s) as a terminate-socket-timer reset
    1. This also applies if `pong` comes in between `send` and `sent`, which can happen sometimes due to the async nature of the callback:

 {F1679288626}

# V1

~~In this diff, a more forgiving mechanism is introduced, i.e. CDT is allowed to miss a ping-pong roundtrip 3 times before the websocket connection is terminated.~~

~~This allows a bit more breathing room for CDT's initialisation during log spam while maintaining the same ping-pong interval for VS Code to keep the auto SSH tunnel alive.~~

Reviewed By: huntie

Differential Revision: D58220230

fbshipit-source-id: 7111c9878492d8755a6110a5cdf4ef622265001d
2024-06-12 09:45:34 -07:00
Alex Hunt
d72ac96e8e Remove nonstandard "vm" field from modern CDP targets (#44835)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44835

As titled. The `vm` field is not part of the CDP spec and will not be used by the modern debugger frontend or proxy.

This change affects modern CDP targets only (using `InspectorPackagerConnection`). We aim to enable sharing of more detailed metadata over 1/ a new, dedicated CDP domain, and 2/ namespaced under the existing `reactNative` field (for the latter, strictly limited to metadata necessary for dev server functionality).

Changelog: [Internal]

(Note: `/json` endpoint behaviour is unchanged for legacy CDP targets)

Reviewed By: robhogan

Differential Revision: D58285587

fbshipit-source-id: dfef3a56b20486ba11891df9940f6c7bef59528e
2024-06-11 08:48:45 -07:00
Alex Hunt
df19e597e3 Remove faviconUrl field from CDP list response (#44834)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44834

- Remove the nonstandard, unused `faviconUrl` field from CDP `/json` response targets (note: both legacy and modern targets).
- Reorder `PageDescription` members.

Changelog:
[General][Removed] - `react-native/dev-middleware`: Remove nonstandard `faviconUrl` field from CDP `/json` response

Reviewed By: hoxyq

Differential Revision: D58092090

fbshipit-source-id: a593be00464853a3fe179305efae5643d616573b
2024-06-07 08:23:45 -07:00
Edmond Chui
82a3bb8f75 add fusebox flag to error case of launch debugger event (#44696)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44696

Changelog: [Internal]

Add `prefers_fusebox_frontend` annotation to the error case for the launch debugger event

Reviewed By: hoxyq

Differential Revision: D57866634

fbshipit-source-id: fcd3655539c936d9965b0c79a47b1b58bb9a4e48
2024-05-29 06:00:36 -07:00
Alex Hunt
352d4b1c83 Add default browser fallback to debugger launch flow (#44673)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44673

Changelog: [Internal]

Reviewed By: hoxyq

Differential Revision: D57726648

fbshipit-source-id: 66eaab1dd0d53fe2befb3dc96d5deb426b86e3e7
2024-05-27 02:53:36 -07:00
Alex Hunt
5d827d83c1 Simplify debugger launch flow to invoke Chrome directly, drop kill API (#44672)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44672

Swaps out and simplifies the internals of the debugger launch flow.

We observed that we could achieve better launch/windowing behaviour by passing the `--app` argument directly to the detected Chrome path.

This shares the user's default Chrome profile:
- Fixes unwanted behaviour such as a separate dock icon on macOS (which, when clicked, would launch an unwanted empty window).
- Enables settings persistence.

This change also removes the `LaunchedBrowser.kill` API.

Changelog: [Internal]

Reviewed By: hoxyq

Differential Revision: D57726649

fbshipit-source-id: fc3a715dc852a50559048d1d1c378f64aeb2013f
2024-05-27 02:53:36 -07:00
Alex Hunt
7e0f86f34b Revert debugger launch behaviour to default profile (#44638)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44638

Reverts the debugger launch flow to use the default `ChromeLauncher` profile. This is the approach used in the current `--experimental-debugger` experiment and by Expo.

This is motivated after a review of the tradeoffs of a guest profile — which allow us to programatically quit the browser process, however takes over system URL handling.

Changelog: [Internal]

Reviewed By: hoxyq

Differential Revision: D57619542

fbshipit-source-id: 3713e1cf8eed61e7a70ed1e4eb58f02da845155f
2024-05-27 02:53:36 -07:00
Fabrizio Cucci
95de14dc53 Drop unused code + minor lint fixes (#44645)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44645

Changelog: [Internal]

As per title.

Reviewed By: NickGerleman

Differential Revision: D57664819

fbshipit-source-id: 2388bd01c00d814a12de7f2e285b78f207e6012c
2024-05-22 12:11:33 -07:00
Edmond Chui
cc99e924ca add prefersFuseboxFrontend field to debugger_command (#44499)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44499

Changelog: [internal]

As [discussed](https://fb.workplace.com/groups/react.devx.team/permalink/930483712103526/), we'll begin segmenting Telemetry signals by Fusebox/non-Fusebox.

* Add new flag in event reporter for `debugger_command` (other events in subsequent diffs)
* Add new column to the Scuba destination

Reviewed By: blakef

Differential Revision: D57140479

fbshipit-source-id: 7ea813b1b4d53a282873fa95c8ee82e5d6f3d1d3
2024-05-09 03:43:38 -07:00
Edmond Chui
75dc42f4df extract param type (#44498)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44498

Changelog: [internal]

Quick refactor to reduce commit noise in the following diff in the stack

Reviewed By: blakef

Differential Revision: D57140480

fbshipit-source-id: aa1fef83d5347b8a11651d3d5c4112b4adf7a7d5
2024-05-09 03:43:38 -07:00
Blake Friedman
3ddf509924 Back out "conditional Babel registration no longer necessary"
Summary:
Original commit changeset: 6bec706c639f

Original Phabricator Diff: D56839521

Changelog: [Internal]

Reviewed By: robhogan

Differential Revision: D57052476

fbshipit-source-id: 7c598f43b23959f1accf555be38abc95e4ef35fd
2024-05-07 08:36:04 -07:00
Blake Friedman
47848ad15f conditional Babel registration no longer necessary (#44362)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44362

Packages that are built and directly run in the monorepo no longer need to worry about
conditionally registering themselves to transpile Flow -> JS at runtime. Our build step
strips this file now.

Changelog: [Internal] changes in published packages no longer require conditional calls to Babel register.

Reviewed By: huntie

Differential Revision: D56839521

fbshipit-source-id: 6bec706c639f1ab4138e0b790be8a07654333046
2024-05-07 03:16:00 -07:00
Alex Hunt
185f63b0db Add experiment to configure internal debugger branding (#44388)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44388

Towards the open source rollout of the `rn_fusebox.ts` entry point.

NOTE: Requires https://github.com/facebookexperimental/rn-chrome-devtools-frontend/pull/59, but can (and should) be landed safely beforehand.

Changelog: [Internal]

Reviewed By: motiz88

Differential Revision: D56883040

fbshipit-source-id: acee5d53c0f93ef9dc2d498001291b1e35237824
2024-05-02 09:15:30 -07:00
Edmond Chui
b7de916664 move launch ID gen out of dev-middleware
Summary: Changelog: [General][Removed] `launchId` query param for `/debugger-frontend` is no longer generated automatically for each `/open-debugger` call. Caller of `/open-debugger` is now responsible for generating the `launchId`, which will be passed along to `/debugger-frontend`.

Reviewed By: robhogan

Differential Revision: D55164645

fbshipit-source-id: b83303eda77b6fb86ebf50f699d9f308676533c6
2024-04-29 11:55:28 -07:00
Edmond Chui
820d1e1ff4 add fields to launch_debugger_frontend
Summary: Changelog: [Internal]

Reviewed By: robhogan

Differential Revision: D56610268

fbshipit-source-id: 37abee41dd068f3cafa19466b914545f6460a3f0
2024-04-29 11:55:28 -07:00
Alex Hunt
548fcd8b3b Update "Open Debugger" to print extended Flipper guidance
Summary:
Supports the removal of Flipper from the template in 0.74, paried with additional blog post messaging: https://reactnative.dev/blog/2024/04/22/release-0.74#removal-of-flipper-react-native-plugin.

Changelog:
[General][Changed] - Update "Open Debugger" action to print extended Flipper guidance

Reviewed By: cipolleschi

Differential Revision: D56705236

fbshipit-source-id: d7e869625262ebb02bc2454c924f832cccfbcd31
2024-04-29 10:18:45 -07:00
Moti Zilberman
b1bb0bee41 Align debugger Chrome launch flags with Meta-internal version (#44180)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44180

Changelog: [General][Changed] Update Chrome launch flags for `--experimental-debugger` launch flow

Internally at Meta, we've been testing the experimental debugger launch flow with a different set of Chrome flags than are currently shipped in open source. This diff fixes those differences:

* Removes `--disable-backgrounding-occluded-windows`
* Adds `--guest`

Reviewed By: EdmondChuiHW

Differential Revision: D56418271

fbshipit-source-id: 884c5746e93cad89f17e4ef9e3ef193a2a454eb5
2024-04-22 08:37:32 -07:00
Rob Hogan
a52bd7dd08 Inspector proxy: Rewrite 127.0.0.1 to localhost in source map URLs to support IPv4->IPv6 tunnels (#44092)
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/44092

In a setup where a device retrieves a bundle from `http://127.0.0.1:8081`, but this is tunnelled to a remote host with only an IPv6 stack (eg, FB dev servers), the host running the inspector-proxy will fail to fetch source or source maps from 127.0.0.1 despite typically being on the same host (indeed, process) as Metro.

This causes a surprising inconsistency where using a bundler URL of `localhost` from the device results in source maps being inlined into `Debugger.scriptParsed`, but using a bundler URL of `127.0.0.1` causes inspector-proxy to fall back to preserving URLs, which are typically fetched lazily by CDT later.

This should be unnecessary once we've implemented CDP `Network.loadNetworkResource` and removed `Debugger.scriptParsed` rewriting, but for now it brings IPv6 tunnelled servers in line with local servers.

Changelog:
[General][Changed] Inspector proxy: Rewrite 127.0.0.1 to localhost in source map URLs for better IPv4->IPv6 tunnelling support.

Reviewed By: motiz88

Differential Revision: D56138742

fbshipit-source-id: b65c9cc8225a0ed54cf32171f640ef9e6408c762
2024-04-16 03:00:13 -07:00