From db3069b7ca29bf8bb4df6ae6194c4e5742a1514c Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Wed, 7 Aug 2024 09:08:28 -0700 Subject: [PATCH] Schedule IWebSocketDelegate event calls on inspector thread (#45849) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/45849 This fixes a bug where we were calling `delegate.didReceiveMessage` (and other handlers) from multiple threads on Android. In particular, with the addition of `Network.loadNetworkResource` in D54496969, we observed memory access issues in the implementation for `IO.read` in `NetworkIOAgent` after multiple successive requests are received. This approach updates the Android-specific implementation of `IWebSocketDelegate` to schedule delegate handler and `close` calls on the inspector thread. Changelog: [Internal] Differential Revision: D60520747 fbshipit-source-id: 459b44b424157793faaf5967435e1303a0061292 --- .../CxxInspectorPackagerConnection.java | 39 ++++++++++++++----- .../InspectorPackagerConnection.h | 9 ++--- .../jsinspector-modern/WebSocketInterfaces.h | 6 +++ 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/CxxInspectorPackagerConnection.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/CxxInspectorPackagerConnection.java index 0dd97395584..8f03af06f99 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/CxxInspectorPackagerConnection.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/CxxInspectorPackagerConnection.java @@ -101,24 +101,43 @@ import okhttp3.WebSocketListener; new WebSocketListener() { @Override public void onFailure(WebSocket _unused, Throwable t, @Nullable Response response) { - @Nullable String message = t.getMessage(); - delegate.didFailWithError(null, message != null ? message : ""); - // "No further calls to this listener will be made." -OkHttp docs for - // WebSocketListener.onFailure - delegate.close(); + scheduleCallback( + new Runnable() { + public void run() { + @Nullable String message = t.getMessage(); + delegate.didFailWithError( + null, message != null ? message : ""); + // "No further calls to this listener will be made." -OkHttp docs for + // WebSocketListener.onFailure + delegate.close(); + } + }, + 0); } @Override public void onMessage(WebSocket _unused, String text) { - delegate.didReceiveMessage(text); + scheduleCallback( + new Runnable() { + public void run() { + delegate.didReceiveMessage(text); + } + }, + 0); } @Override public void onClosed(WebSocket _unused, int code, String reason) { - delegate.didClose(); - // "No further calls to this listener will be made." -OkHttp docs for - // WebSocketListener.onClosed - delegate.close(); + scheduleCallback( + new Runnable() { + public void run() { + delegate.didClose(); + // "No further calls to this listener will be made." -OkHttp docs for + // WebSocketListener.onClosed + delegate.close(); + } + }, + 0); } }); return new IWebSocket() { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.h index 8752cc7fd6f..06ad56b5566 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.h @@ -71,11 +71,10 @@ class InspectorPackagerConnectionDelegate { /** * Schedules a function to run after a delay. If the function is called * asynchronously, the implementer of InspectorPackagerConnectionDelegate - * is responsible for thread safety (e.g. scheduling the callback on a thread - * that has unique access to the InspectorPackagerConnection instance, or - * otherwise ensuring synchronization). The callback MAY be dropped and never - * called if no further callbacks are being accepted, e.g. if the application - * is terminating. + * is responsible for thread safety and should schedule the callback on + * the inspector queue. The callback MAY be dropped and never called if no + * further callbacks are being accepted, e.g. if the application is + * terminating. */ virtual void scheduleCallback( std::function callback, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h b/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h index a9606127d3b..bd79e8af184 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h @@ -39,6 +39,8 @@ class IWebSocketDelegate { /** * Called when the socket has encountered an error. + * This method must be called on the inspector queue, and the + * WebSocketDelegate may not be destroyed while it is executing. * \param posixCode POSIX errno value if available, otherwise nullopt. * \param error Error description. */ @@ -48,6 +50,8 @@ class IWebSocketDelegate { /** * Called when a message has been received from the socket. + * This method must be called on the inspector queue, and the + * WebSocketDelegate may not be destroyed while it is executing. * \param message Message received, in UTF-8 encoding. */ virtual void didReceiveMessage(std::string_view message) = 0; @@ -55,6 +59,8 @@ class IWebSocketDelegate { /** * Called when the socket has been closed. The call is not required if * didFailWithError was called instead. + * This method must be called on the inspector queue, and the + * WebSocketDelegate may not be destroyed while it is executing. */ virtual void didClose() = 0; };