From 228193dad5369cdb5e13ba8a926e0ab9d12429e7 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 20 Nov 2023 13:01:04 -0800 Subject: [PATCH] Do not suppress IllegalArgumentExceptions during TurboModule creation (#41509) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41509 We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165 fbshipit-source-id: 1eea1db6c7e3313a36d24e7837b36a3d0fccc718 --- .../com/facebook/react/DebugCorePackage.java | 5 ++- ...eactPackageTurboModuleManagerDelegate.java | 36 ++++++++++--------- .../facebook/react/bridge/ModuleHolder.java | 3 +- .../react/runtime/CoreReactPackage.java | 6 ++-- 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java index 0168fce4f5ba7f..398b69ac1a5684 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java @@ -43,13 +43,12 @@ class DebugCorePackage extends TurboReactPackage implements ViewManagerOnDemandR public DebugCorePackage() {} @Override - public NativeModule getModule(String name, ReactApplicationContext reactContext) { + public @Nullable NativeModule getModule(String name, ReactApplicationContext reactContext) { switch (name) { case JSCHeapCapture.NAME: return new JSCHeapCapture(reactContext); default: - throw new IllegalArgumentException( - "In DebugCorePackage, could not find Native module for " + name); + return null; } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java index c05f893d1c10f1..30ef9e019b3783 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java @@ -8,11 +8,13 @@ package com.facebook.react; import androidx.annotation.Nullable; +import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.CxxModuleWrapper; import com.facebook.react.bridge.ModuleSpec; import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.ReactApplicationContext; +import com.facebook.react.common.ReactConstants; import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.internal.turbomodule.core.TurboModuleManagerDelegate; import com.facebook.react.internal.turbomodule.core.interfaces.TurboModule; @@ -187,11 +189,12 @@ public TurboModule getModule(String moduleName) { return nativeModule; } } catch (IllegalArgumentException ex) { - /* - TurboReactPackages can throw an IllegalArgumentException when a module isn't found. If - this happens, it's safe to ignore the exception because a later TurboReactPackage could - provide the module. - */ + // TODO T170570617: remove this catch statement and let exception bubble up + FLog.e( + ReactConstants.TAG, + ex, + "Caught exception while constructing module '%s'. This was previously ignored but will not be caught in the future.", + moduleName); } } else { throw new IllegalArgumentException( @@ -217,11 +220,12 @@ public TurboModule getModule(String moduleName) { } } catch (IllegalArgumentException ex) { - /* - TurboReactPackages can throw an IllegalArgumentException when a module isn't found. If - this happens, it's safe to ignore the exception because a later TurboReactPackage could - provide the module. - */ + // TODO T170570617: remove this catch statement and let exception bubble up + FLog.e( + ReactConstants.TAG, + ex, + "Caught exception while constructing module '%s'. This was previously ignored but will not be caught in the future.", + moduleName); } } @@ -294,13 +298,13 @@ public NativeModule getLegacyModule(String moduleName) { resolvedModule = module; } } - } catch (IllegalArgumentException ex) { - /* - * TurboReactPackages can throw an IllegalArgumentException when a module isn't found. If - * this happens, it's safe to ignore the exception because a later TurboReactPackage could - * provide the module. - */ + // TODO T170570617: remove this catch statement and let exception bubble up + FLog.e( + ReactConstants.TAG, + ex, + "Caught exception while constructing module '%s'. This was previously ignored but will not be caught in the future.", + moduleName); } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java index 731aa04fa3cf1b..25f170f1134a92 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java @@ -19,6 +19,7 @@ import com.facebook.debug.tags.ReactDebugOverlayTags; import com.facebook.infer.annotation.Assertions; import com.facebook.proguard.annotations.DoNotStrip; +import com.facebook.react.common.ReactConstants; import com.facebook.react.internal.turbomodule.core.interfaces.TurboModule; import com.facebook.react.module.model.ReactModuleInfo; import com.facebook.systrace.SystraceMessage; @@ -204,7 +205,7 @@ private NativeModule create() { * * @todo(T53311351) */ - FLog.e("NativeModuleInitError", "Failed to create NativeModule \"" + getName() + "\"", ex); + FLog.e(ReactConstants.TAG, ex, "Failed to create NativeModule '%s'", mName); throw ex; } finally { ReactMarker.logMarker(CREATE_MODULE_END, mName, mInstanceKey); diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java index 2550520a48641e..cb8b9c13af09ae 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java @@ -7,6 +7,7 @@ package com.facebook.react.runtime; +import androidx.annotation.Nullable; import com.facebook.infer.annotation.Nullsafe; import com.facebook.react.TurboReactPackage; import com.facebook.react.bridge.NativeModule; @@ -51,7 +52,7 @@ public CoreReactPackage( } @Override - public NativeModule getModule(String name, ReactApplicationContext reactContext) { + public @Nullable NativeModule getModule(String name, ReactApplicationContext reactContext) { switch (name) { case AndroidInfoModule.NAME: return new AndroidInfoModule(reactContext); @@ -68,8 +69,7 @@ public NativeModule getModule(String name, ReactApplicationContext reactContext) case ExceptionsManagerModule.NAME: return new ExceptionsManagerModule(mDevSupportManager); default: - throw new IllegalArgumentException( - "In BridgelessReactPackage, could not find Native module for " + name); + return null; } }