Skip to content

Commit

Permalink
Reduce use of Java exceptions in prop parsing (#36160)
Browse files Browse the repository at this point in the history 
Summary:
Pull Request resolved: #36160

Changelog:
[Android][Fixed] - Invalid prop values no longer trigger Java exceptions in the legacy renderer

## Context

We are changing React Native to behave more like a browser in the sense that **bad style values are not runtime errors**. (See e.g. D43159284 (d6e9891), D43184380.) The recommended way for developers to ensure they are passing correct style values is to use a typechecker (TypeScript or Flow) in conjunction with E2E tests and manual spot checks.

## This diff

This change is similar to D43184380, but here we target the legacy renderer on Android by (1) replacing exceptions with logging calls, (2) adding fallback values (equivalent to resetting the respective props to `null`) where needed, and (3) replacing `null` checks in `ReactProp` converters with `instanceof` checks.

Leaving the logging call sites in place (as opposed to deleting them) will be helpful if we decide that we want to repurpose these checks for a new, more visible diagnostic (though we would likely only build such a diagnostic in Fabric at this point).

Reviewed By: javache

Differential Revision: D43274525

fbshipit-source-id: 9d1e7ca3b6299dd827e8667e3d542433ec896c0e
  • Loading branch information
@motiz88 @facebook-github-bot
motiz88 authored and facebook-github-bot committed Feb 15, 2023
1 parent cb28a2c commit e328fc2
Show file tree
Hide file tree
Showing 22 changed files with 168 additions and 113 deletions.
11 changes: 11 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/bridge/ColorPropConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import android.util.TypedValue;
import androidx.annotation.Nullable;
import androidx.core.content.res.ResourcesCompat;
import com.facebook.common.logging.FLog;
import com.facebook.react.common.ReactConstants;

public class ColorPropConverter {
private static final String JSON_KEY = "resource_paths";
Expand Down Expand Up @@ -61,6 +63,15 @@ public static Integer getColor(Object value, Context context) {
"ColorValue: the value must be a number or Object.");
}

public static Integer getColor(Object value, Context context, int defaultInt) {
try {
return getColor(value, context);
} catch (JSApplicationCausedNativeException e) {
FLog.w(ReactConstants.TAG, e, "Error converting ColorValue");
return defaultInt;
}
}

public static Integer resolveResourcePath(Context context, @Nullable String resourcePath) {
if (resourcePath == null || resourcePath.isEmpty()) {
return null;
Expand Down
36 changes: 23 additions & 13 deletions ReactAndroid/src/main/java/com/facebook/react/processing/ReactPropertyProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,15 @@ private static CodeBlock.Builder getPropertyExtractor(
ClassInfo classInfo, PropertyInfo info, CodeBlock.Builder builder) {
TypeName propertyType = info.propertyType;
if (propertyType.equals(STRING_TYPE)) {
return builder.add("($L)value", STRING_TYPE);
return builder.add("value instanceof $L ? ($L)value : null", STRING_TYPE, STRING_TYPE);
} else if (propertyType.equals(READABLE_ARRAY_TYPE)) {
return builder.add("($L)value", READABLE_ARRAY_TYPE); // TODO: use real type but needs import
return builder.add(
"value instanceof $L ? ($L)value : null",
READABLE_ARRAY_TYPE,
READABLE_ARRAY_TYPE); // TODO: use real type but needs import
} else if (propertyType.equals(READABLE_MAP_TYPE)) {
return builder.add("($L)value", READABLE_MAP_TYPE);
return builder.add(
"value instanceof $L ? ($L)value : null", READABLE_MAP_TYPE, READABLE_MAP_TYPE);
} else if (propertyType.equals(DYNAMIC_TYPE)) {
return builder.add("new $L(value)", DYNAMIC_FROM_OBJECT_TYPE);
} else if (propertyType.equals(YOGA_VALUE_TYPE)) {
Expand All @@ -380,40 +384,46 @@ private static CodeBlock.Builder getPropertyExtractor(
}

if (propertyType.equals(TypeName.BOOLEAN)) {
return builder.add("value == null ? $L : (boolean) value", info.mProperty.defaultBoolean());
return builder.add(
"!(value instanceof Boolean) ? $L : (boolean)value", info.mProperty.defaultBoolean());
}
if (propertyType.equals(TypeName.DOUBLE)) {
double defaultDouble = info.mProperty.defaultDouble();
if (Double.isNaN(defaultDouble)) {
return builder.add("value == null ? $T.NaN : (double) value", Double.class);
return builder.add("!(value instanceof Double) ? $T.NaN : (double)value", Double.class);
} else {
return builder.add("value == null ? $Lf : (double) value", defaultDouble);
return builder.add("!(value instanceof Double) ? $Lf : (double)value", defaultDouble);
}
}
if (propertyType.equals(TypeName.FLOAT)) {
float defaultFloat = info.mProperty.defaultFloat();
if (Float.isNaN(defaultFloat)) {
return builder.add("value == null ? $T.NaN : ((Double)value).floatValue()", Float.class);
return builder.add(
"!(value instanceof Double) ? $T.NaN : ((Double)value).floatValue()", Float.class);
} else {
return builder.add("value == null ? $Lf : ((Double)value).floatValue()", defaultFloat);
return builder.add(
"!(value instanceof Double) ? $Lf : ((Double)value).floatValue()", defaultFloat);
}
}
if ("Color".equals(info.mProperty.customType())) {
switch (classInfo.getType()) {
case VIEW_MANAGER:
return builder.add(
"value == null ? $L : $T.getColor(value, view.getContext())",
"value == null ? $L : $T.getColor(value, view.getContext(), $L)",
info.mProperty.defaultInt(),
com.facebook.react.bridge.ColorPropConverter.class);
com.facebook.react.bridge.ColorPropConverter.class,
info.mProperty.defaultInt());
case SHADOW_NODE:
return builder.add(
"value == null ? $L : $T.getColor(value, node.getThemedContext())",
"value == null ? $L : $T.getColor(value, node.getThemedContext(), $L)",
info.mProperty.defaultInt(),
com.facebook.react.bridge.ColorPropConverter.class);
com.facebook.react.bridge.ColorPropConverter.class,
info.mProperty.defaultInt());
}
} else if (propertyType.equals(TypeName.INT)) {
return builder.add(
"value == null ? $L : ((Double)value).intValue()", info.mProperty.defaultInt());
"!(value instanceof Double) ? $L : ((Double)value).intValue()",
info.mProperty.defaultInt());
}

throw new IllegalArgumentException();
Expand Down
54 changes: 34 additions & 20 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
package com.facebook.react.uimanager;

import androidx.annotation.Nullable;
import com.facebook.common.logging.FLog;
import com.facebook.react.bridge.Dynamic;
import com.facebook.react.bridge.JSApplicationIllegalArgumentException;
import com.facebook.react.bridge.ReadableType;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.modules.i18nmanager.I18nUtil;
import com.facebook.react.uimanager.annotations.ReactProp;
import com.facebook.react.uimanager.annotations.ReactPropGroup;
Expand Down Expand Up @@ -60,11 +61,14 @@ void setFromDynamic(Dynamic dynamic) {
unit = YogaUnit.PERCENT;
value = Float.parseFloat(s.substring(0, s.length() - 1));
} else {
throw new IllegalArgumentException("Unknown value: " + s);
FLog.w(ReactConstants.TAG, "Unknown value: " + s);
}
} else {
} else if (dynamic.getType() == ReadableType.Number) {
unit = YogaUnit.POINT;
value = PixelUtil.toPixelFromDIP(dynamic.asDouble());
} else {
unit = YogaUnit.UNDEFINED;
value = YogaConstants.UNDEFINED;
}
}
}
Expand Down Expand Up @@ -318,8 +322,9 @@ public void setFlexDirection(@Nullable String flexDirection) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for flexDirection: " + flexDirection);
FLog.w(ReactConstants.TAG, "invalid value for flexDirection: " + flexDirection);
setFlexDirection(YogaFlexDirection.COLUMN);
break;
}
}
}
Expand Down Expand Up @@ -353,8 +358,9 @@ public void setFlexWrap(@Nullable String flexWrap) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for flexWrap: " + flexWrap);
FLog.w(ReactConstants.TAG, "invalid value for flexWrap: " + flexWrap);
setFlexWrap(YogaWrap.NO_WRAP);
break;
}
}
}
Expand Down Expand Up @@ -413,8 +419,9 @@ public void setAlignSelf(@Nullable String alignSelf) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for alignSelf: " + alignSelf);
FLog.w(ReactConstants.TAG, "invalid value for alignSelf: " + alignSelf);
setAlignSelf(YogaAlign.AUTO);
return;
}
}
}
Expand Down Expand Up @@ -473,8 +480,9 @@ public void setAlignItems(@Nullable String alignItems) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for alignItems: " + alignItems);
FLog.w(ReactConstants.TAG, "invalid value for alignItems: " + alignItems);
setAlignItems(YogaAlign.STRETCH);
return;
}
}
}
Expand Down Expand Up @@ -533,8 +541,9 @@ public void setAlignContent(@Nullable String alignContent) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for alignContent: " + alignContent);
FLog.w(ReactConstants.TAG, "invalid value for alignContent: " + alignContent);
setAlignContent(YogaAlign.FLEX_START);
return;
}
}
}
Expand Down Expand Up @@ -583,8 +592,9 @@ public void setJustifyContent(@Nullable String justifyContent) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for justifyContent: " + justifyContent);
FLog.w(ReactConstants.TAG, "invalid value for justifyContent: " + justifyContent);
setJustifyContent(YogaJustify.FLEX_START);
break;
}
}
}
Expand Down Expand Up @@ -617,8 +627,9 @@ public void setOverflow(@Nullable String overflow) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for overflow: " + overflow);
FLog.w(ReactConstants.TAG, "invalid value for overflow: " + overflow);
setOverflow(YogaOverflow.VISIBLE);
break;
}
}
}
Expand Down Expand Up @@ -647,7 +658,9 @@ public void setDisplay(@Nullable String display) {
}
default:
{
throw new JSApplicationIllegalArgumentException("invalid value for display: " + display);
FLog.w(ReactConstants.TAG, "invalid value for display: " + display);
setDisplay(YogaDisplay.FLEX);
break;
}
}
}
Expand Down Expand Up @@ -820,8 +833,9 @@ public void setPosition(@Nullable String position) {
}
default:
{
throw new JSApplicationIllegalArgumentException(
"invalid value for position: " + position);
FLog.w(ReactConstants.TAG, "invalid value for position: " + position);
setPositionType(YogaPositionType.RELATIVE);
break;
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/TouchTargetHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
import android.view.View;
import android.view.ViewGroup;
import androidx.annotation.Nullable;
import com.facebook.react.bridge.JSApplicationIllegalArgumentException;
import com.facebook.common.logging.FLog;
import com.facebook.react.bridge.UiThreadUtil;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.touch.ReactHitSlopView;
import com.facebook.react.uimanager.common.ViewUtil;
import java.util.ArrayList;
Expand Down Expand Up @@ -366,7 +367,10 @@ && isTouchPointInView(eventCoords[0], eventCoords[1], view)) {

return null;

} else if (pointerEvents == PointerEvents.AUTO) {
} else {
if (pointerEvents != PointerEvents.AUTO) {
FLog.w(ReactConstants.TAG, "Unknown pointer event type: " + pointerEvents.toString());
}
// Either this view or one of its children is the target
if (view instanceof ReactCompoundViewGroup
&& isTouchPointInView(eventCoords[0], eventCoords[1], view)
Expand All @@ -387,10 +391,6 @@ && isTouchPointInView(eventCoords[0], eventCoords[1], view)
pathAccumulator.add(new ViewTarget(view.getId(), view));
}
return result;

} else {
throw new JSApplicationIllegalArgumentException(
"Unknown pointer event type: " + pointerEvents.toString());
}
}

Expand Down
6 changes: 3 additions & 3 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/TransformHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@

package com.facebook.react.uimanager;

import com.facebook.react.bridge.JSApplicationIllegalArgumentException;
import com.facebook.common.logging.FLog;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.ReadableType;
import com.facebook.react.common.ReactConstants;

/**
* Class providing helper methods for converting transformation list (as accepted by 'transform'
Expand Down Expand Up @@ -100,8 +101,7 @@ public static void processTransform(ReadableArray transforms, double[] result) {
} else if ("skewY".equals(transformType)) {
MatrixMathHelper.applySkewY(helperMatrix, convertToRadians(transform, transformType));
} else {
throw new JSApplicationIllegalArgumentException(
"Unsupported transform type: " + transformType);
FLog.w(ReactConstants.TAG, "Unsupported transform type: " + transformType);
}

MatrixMathHelper.multiplyInto(result, result, helperMatrix);
Expand Down
16 changes: 11 additions & 5 deletions ReactAndroid/src/main/java/com/facebook/react/views/drawer/ReactDrawerLayoutManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.drawerlayout.widget.DrawerLayout;
import com.facebook.common.logging.FLog;
import com.facebook.react.bridge.Dynamic;
import com.facebook.react.bridge.JSApplicationIllegalArgumentException;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableType;
import com.facebook.react.common.MapBuilder;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.uimanager.PixelUtil;
import com.facebook.react.uimanager.ThemedReactContext;
Expand Down Expand Up @@ -90,13 +92,14 @@ public void setDrawerPosition(ReactDrawerLayout view, Dynamic drawerPosition) {
if (Gravity.START == drawerPositionNum || Gravity.END == drawerPositionNum) {
view.setDrawerPosition(drawerPositionNum);
} else {
throw new JSApplicationIllegalArgumentException(
"Unknown drawerPosition " + drawerPositionNum);
FLog.w(ReactConstants.TAG, "Unknown drawerPosition " + drawerPositionNum);
view.setDrawerPosition(Gravity.START);
}
} else if (drawerPosition.getType() == ReadableType.String) {
setDrawerPositionInternal(view, drawerPosition.asString());
} else {
throw new JSApplicationIllegalArgumentException("drawerPosition must be a string or int");
FLog.w(ReactConstants.TAG, "drawerPosition must be a string or int");
view.setDrawerPosition(Gravity.START);
}
}

Expand All @@ -106,8 +109,10 @@ private void setDrawerPositionInternal(ReactDrawerLayout view, String drawerPosi
} else if (drawerPosition.equals("right")) {
view.setDrawerPosition(Gravity.END);
} else {
throw new JSApplicationIllegalArgumentException(
FLog.w(
ReactConstants.TAG,
"drawerPosition must be 'left' or 'right', received" + drawerPosition);
view.setDrawerPosition(Gravity.START);
}
}

Expand Down Expand Up @@ -139,7 +144,8 @@ public void setDrawerLockMode(ReactDrawerLayout view, @Nullable String drawerLoc
} else if ("locked-open".equals(drawerLockMode)) {
view.setDrawerLockMode(DrawerLayout.LOCK_MODE_LOCKED_OPEN);
} else {
throw new JSApplicationIllegalArgumentException("Unknown drawerLockMode " + drawerLockMode);
FLog.w(ReactConstants.TAG, "Unknown drawerLockMode " + drawerLockMode);
view.setDrawerLockMode(DrawerLayout.LOCK_MODE_UNLOCKED);
}
}

Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/image/BUCK
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ rn_android_library(
],
deps = [
YOGA_TARGET,
react_native_dep("libraries/fbcore/src/main/java/com/facebook/common/logging:logging"),
react_native_dep("libraries/fresco/fresco-react-native:fbcore"),
react_native_dep("libraries/fresco/fresco-react-native:fresco-drawee"),
react_native_dep("libraries/fresco/fresco-react-native:fresco-react-native"),
Expand Down

0 comments on commit e328fc2

Please sign in to comment.