From a7f56e73e4ea0d382a706403260b2d153569e3f0 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 5 Nov 2019 13:17:08 -0800 Subject: [PATCH] Add UI-Thread assertions in methods that mutate Views Summary: As part of `T54997838` we're auditing where removeView could possibly be called in a background thread, and adding annotations to indicate places where we don't think it's possible. Changelog: [internal] Reviewed By: makovkastar Differential Revision: D18320461 fbshipit-source-id: 84b6b9e293d903f835fc42bc98614efb54158986 --- .../facebook/react/uimanager/ViewGroupManager.java | 9 +++++++++ .../react/views/modal/ReactModalHostView.java | 11 +++++++++++ .../react/views/view/ReactClippingViewManager.java | 9 +++++++++ .../com/facebook/react/views/view/ReactViewGroup.java | 9 +++++++++ 4 files changed, 38 insertions(+) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java index 9b22349e96f827..a0c249d4d1b59a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java @@ -10,6 +10,7 @@ import android.view.View; import android.view.ViewGroup; import androidx.annotation.Nullable; +import com.facebook.react.bridge.UiThreadUtil; import java.util.List; import java.util.WeakHashMap; @@ -44,6 +45,8 @@ public void addView(T parent, View child, int index) { * @param views the set of views to add */ public void addViews(T parent, List views) { + UiThreadUtil.assertOnUiThread(); + for (int i = 0, size = views.size(); i < size; i++) { addView(parent, views.get(i), i); } @@ -66,10 +69,14 @@ public View getChildAt(T parent, int index) { } public void removeViewAt(T parent, int index) { + UiThreadUtil.assertOnUiThread(); + parent.removeViewAt(index); } public void removeView(T parent, View view) { + UiThreadUtil.assertOnUiThread(); + for (int i = 0; i < getChildCount(parent); i++) { if (getChildAt(parent, i) == view) { removeViewAt(parent, i); @@ -79,6 +86,8 @@ public void removeView(T parent, View view) { } public void removeAllViews(T parent) { + UiThreadUtil.assertOnUiThread(); + for (int i = getChildCount(parent) - 1; i >= 0; i--) { removeViewAt(parent, i); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java b/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java index 5d761b78d5dac1..6d93ccc85b04c6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java @@ -27,6 +27,7 @@ import com.facebook.react.bridge.GuardedRunnable; import com.facebook.react.bridge.LifecycleEventListener; import com.facebook.react.bridge.ReactContext; +import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.bridge.WritableMap; import com.facebook.react.bridge.WritableNativeMap; import com.facebook.react.common.annotations.VisibleForTesting; @@ -97,6 +98,8 @@ protected void onLayout(boolean changed, int l, int t, int r, int b) { @Override public void addView(View child, int index) { + UiThreadUtil.assertOnUiThread(); + mHostView.addView(child, index); } @@ -112,11 +115,15 @@ public View getChildAt(int index) { @Override public void removeView(View child) { + UiThreadUtil.assertOnUiThread(); + mHostView.removeView(child); } @Override public void removeViewAt(int index) { + UiThreadUtil.assertOnUiThread(); + View child = getChildAt(index); mHostView.removeView(child); } @@ -140,6 +147,8 @@ public void onDropInstance() { } private void dismiss() { + UiThreadUtil.assertOnUiThread(); + if (mDialog != null) { if (mDialog.isShowing()) { Activity dialogContext = @@ -217,6 +226,8 @@ public void onHostDestroy() { * new Dialog. */ protected void showOrUpdate() { + UiThreadUtil.assertOnUiThread(); + // If the existing Dialog is currently up, we may need to redraw it or we may be able to update // the property without having to recreate the dialog if (mDialog != null) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.java index 88793f1290a3d2..8f5a436320f169 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactClippingViewManager.java @@ -8,6 +8,7 @@ package com.facebook.react.views.view; import android.view.View; +import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.uimanager.ViewGroupManager; import com.facebook.react.uimanager.annotations.ReactProp; @@ -21,11 +22,15 @@ public abstract class ReactClippingViewManager @ReactProp( name = com.facebook.react.uimanager.ReactClippingViewGroupHelper.PROP_REMOVE_CLIPPED_SUBVIEWS) public void setRemoveClippedSubviews(T view, boolean removeClippedSubviews) { + UiThreadUtil.assertOnUiThread(); + view.setRemoveClippedSubviews(removeClippedSubviews); } @Override public void addView(T parent, View child, int index) { + UiThreadUtil.assertOnUiThread(); + boolean removeClippedSubviews = parent.getRemoveClippedSubviews(); if (removeClippedSubviews) { parent.addViewWithSubviewClippingEnabled(child, index); @@ -56,6 +61,8 @@ public View getChildAt(T parent, int index) { @Override public void removeViewAt(T parent, int index) { + UiThreadUtil.assertOnUiThread(); + boolean removeClippedSubviews = parent.getRemoveClippedSubviews(); if (removeClippedSubviews) { View child = getChildAt(parent, index); @@ -70,6 +77,8 @@ public void removeViewAt(T parent, int index) { @Override public void removeAllViews(T parent) { + UiThreadUtil.assertOnUiThread(); + boolean removeClippedSubviews = parent.getRemoveClippedSubviews(); if (removeClippedSubviews) { parent.removeAllViewsWithSubviewClippingEnabled(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java index 4dcd94a6e5f478..bba7db72de3884 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java @@ -29,6 +29,7 @@ import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.ReactContext; +import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.common.annotations.VisibleForTesting; import com.facebook.react.modules.i18nmanager.I18nUtil; import com.facebook.react.touch.OnInterceptTouchEventListener; @@ -358,6 +359,8 @@ private void updateClippingToRect(Rect clippingRect) { } private void updateSubviewClipStatus(Rect clippingRect, int idx, int clippedSoFar) { + UiThreadUtil.assertOnUiThread(); + View child = Assertions.assertNotNull(mAllChildren)[idx]; sHelperRect.set(child.getLeft(), child.getTop(), child.getRight(), child.getBottom()); boolean intersects = @@ -462,6 +465,8 @@ public void addView(View child, int index, ViewGroup.LayoutParams params) { @Override public void removeView(View view) { + UiThreadUtil.assertOnUiThread(); + mDrawingOrderHelper.handleRemoveView(view); setChildrenDrawingOrderEnabled(mDrawingOrderHelper.shouldEnableCustomDrawingOrder()); @@ -470,6 +475,8 @@ public void removeView(View view) { @Override public void removeViewAt(int index) { + UiThreadUtil.assertOnUiThread(); + mDrawingOrderHelper.handleRemoveView(getChildAt(index)); setChildrenDrawingOrderEnabled(mDrawingOrderHelper.shouldEnableCustomDrawingOrder()); @@ -543,6 +550,8 @@ protected void dispatchSetPressed(boolean pressed) { } /*package*/ void removeViewWithSubviewClippingEnabled(View view) { + UiThreadUtil.assertOnUiThread(); + Assertions.assertCondition(mRemoveClippedSubviews); Assertions.assertNotNull(mClippingRect); Assertions.assertNotNull(mAllChildren);