From 15b38958b007977023f7c4424e26fbaf707eb607 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 3 Sep 2019 21:44:10 -0700 Subject: [PATCH] Log soft exceptions but don't crash when viewState is missing for commands or view deletion Summary: We probably don't always need to crash in these cases. This should replace all prod/dev crashes with logged SoftExceptions. Note that this is currently only used for Fabric. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D17170459 fbshipit-source-id: 70757ae88deb8c893b36971d879174e25a194fb9 --- .../react/bridge/ReactSoftException.java | 53 +++++++++++++++++ .../fabric/mounting/MountingManager.java | 58 ++++++++++++++++--- 2 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSoftException.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSoftException.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSoftException.java new file mode 100644 index 00000000000..aef20efac9c --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSoftException.java @@ -0,0 +1,53 @@ +// Copyright (c) Facebook, Inc. and its affiliates. + +// This source code is licensed under the MIT license found in the +// LICENSE file in the root directory of this source tree. + +package com.facebook.react.bridge; + +import com.facebook.common.logging.FLog; +import com.facebook.proguard.annotations.DoNotStrip; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +@DoNotStrip +@Deprecated +// TODO T53446395: unify ReactSoftException with a more generic exception-handling utility +public class ReactSoftException { + public interface ReactSoftExceptionListener { + void logSoftException(final String category, final Throwable cause); + } + + // Use a list instead of a set here because we expect the number of listeners + // to be very small, and we want listeners to be called in a deterministic + // order. + private static final List sListeners = new CopyOnWriteArrayList<>(); + + @DoNotStrip + public static void addListener(ReactSoftExceptionListener listener) { + if (!sListeners.contains(listener)) { + sListeners.add(listener); + } + } + + @DoNotStrip + public static void removeListener(ReactSoftExceptionListener listener) { + sListeners.remove(listener); + } + + @DoNotStrip + public static void clearListeners() { + sListeners.clear(); + } + + @DoNotStrip + public static void logSoftException(final String category, final Throwable cause) { + if (sListeners.size() > 0) { + for (ReactSoftExceptionListener listener : sListeners) { + listener.logSoftException(category, cause); + } + } else { + FLog.e(category, "Unhandled SoftException", cause); + } + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 5c668434c60..c482eabdc60 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -14,6 +14,7 @@ import androidx.annotation.AnyThread; import androidx.annotation.Nullable; import androidx.annotation.UiThread; import com.facebook.infer.annotation.Assertions; +import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.ReadableNativeMap; @@ -40,6 +41,7 @@ import java.util.concurrent.ConcurrentHashMap; * FabricUIManager#scheduleMountItems(int, MountItem[])} on the UI thread. */ public class MountingManager { + public static final String TAG = MountingManager.class.getSimpleName(); private final ConcurrentHashMap mTagToViewState; private final JSResponderHandler mJSResponderHandler = new JSResponderHandler(); @@ -82,7 +84,7 @@ public class MountingManager { ViewGroupManager viewGroupManager = getViewGroupManager(state); for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) { View child = viewGroupManager.getChildAt(viewGroup, i); - if (mTagToViewState.get(child.getId()) != null) { + if (getNullableViewState(child.getId()) != null) { dropView(child); } viewGroupManager.removeViewAt(viewGroup, i); @@ -114,9 +116,21 @@ public class MountingManager { return viewState; } + private @Nullable ViewState getNullableViewState(int tag) { + return mTagToViewState.get(tag); + } + @Deprecated public void receiveCommand(int reactTag, int commandId, @Nullable ReadableArray commandArgs) { - ViewState viewState = getViewState(reactTag); + ViewState viewState = getNullableViewState(reactTag); + + if (viewState == null) { + ReactSoftException.logSoftException( + MountingManager.TAG, + new IllegalStateException( + "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId)); + return; + } if (viewState.mViewManager == null) { throw new IllegalStateException("Unable to find viewState manager for tag " + reactTag); @@ -130,7 +144,15 @@ public class MountingManager { } public void receiveCommand(int reactTag, String commandId, @Nullable ReadableArray commandArgs) { - ViewState viewState = getViewState(reactTag); + ViewState viewState = getNullableViewState(reactTag); + + if (viewState == null) { + ReactSoftException.logSoftException( + MountingManager.TAG, + new IllegalStateException( + "Unable to find viewState for tag: " + reactTag + " for commandId: " + commandId)); + return; + } if (viewState.mViewManager == null) { throw new IllegalStateException("Unable to find viewState manager for tag " + reactTag); @@ -168,8 +190,18 @@ public class MountingManager { @UiThread public void removeViewAt(int parentTag, int index) { UiThreadUtil.assertOnUiThread(); - ViewState viewState = getViewState(parentTag); + ViewState viewState = getNullableViewState(parentTag); + + if (viewState == null) { + ReactSoftException.logSoftException( + MountingManager.TAG, + new IllegalStateException( + "Unable to find viewState for tag: " + parentTag + " for removeViewAt")); + return; + } + final ViewGroup parentView = (ViewGroup) viewState.mView; + if (parentView == null) { throw new IllegalStateException("Unable to find view for tag " + parentTag); } @@ -185,7 +217,7 @@ public class MountingManager { @Nullable ReadableMap props, @Nullable StateWrapper stateWrapper, boolean isLayoutable) { - if (mTagToViewState.get(reactTag) != null) { + if (getNullableViewState(reactTag) != null) { return; } @@ -281,7 +313,17 @@ public class MountingManager { @UiThread public void deleteView(int reactTag) { UiThreadUtil.assertOnUiThread(); - View view = getViewState(reactTag).mView; + ViewState viewState = getNullableViewState(reactTag); + + if (viewState == null) { + ReactSoftException.logSoftException( + MountingManager.TAG, + new IllegalStateException( + "Unable to find viewState for tag: " + reactTag + " for deleteView")); + return; + } + + View view = viewState.mView; if (view != null) { dropView(view); } else { @@ -351,7 +393,7 @@ public class MountingManager { @Nullable StateWrapper stateWrapper, boolean isLayoutable) { - if (mTagToViewState.get(reactTag) != null) { + if (getNullableViewState(reactTag) != null) { throw new IllegalStateException( "View for component " + componentName + " with tag " + reactTag + " already exists."); } @@ -438,7 +480,7 @@ public class MountingManager { @AnyThread public @Nullable EventEmitterWrapper getEventEmitter(int reactTag) { - ViewState viewState = mTagToViewState.get(reactTag); + ViewState viewState = getNullableViewState(reactTag); return viewState == null ? null : viewState.mEventEmitter; }