From e19225aa6464ec71d15c5da942477d6584e7545c Mon Sep 17 00:00:00 2001 From: Chris Hopman Date: Thu, 30 Jun 2016 15:09:43 -0700 Subject: [PATCH] assert that ShadowNodeRegistry is only accessed from one thread Reviewed By: astreet Differential Revision: D3461859 fbshipit-source-id: 790e831d2ca239110e00a5723be40e870ceab020 --- .../react/common/SingleThreadAsserter.java | 29 +++++++++++++++++++ .../react/uimanager/ShadowNodeRegistry.java | 14 +++++++++ .../react/uimanager/UIImplementation.java | 1 + .../react/uimanager/UIManagerModule.java | 4 +-- 4 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/common/SingleThreadAsserter.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/common/SingleThreadAsserter.java b/ReactAndroid/src/main/java/com/facebook/react/common/SingleThreadAsserter.java new file mode 100644 index 000000000..ad45e838e --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/common/SingleThreadAsserter.java @@ -0,0 +1,29 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package com.facebook.react.common; + +import javax.annotation.Nullable; + +import com.facebook.infer.annotation.Assertions; + +/** + * Simple class for asserting that operations only run on a single thread. + */ +public class SingleThreadAsserter { + private @Nullable Thread mThread = null; + + public void assertNow() { + Thread current = Thread.currentThread(); + if (mThread == null) { + mThread = current; + } + Assertions.assertCondition(mThread == current); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ShadowNodeRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ShadowNodeRegistry.java index 9ffdcd7a4..b8df6fd21 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ShadowNodeRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ShadowNodeRegistry.java @@ -12,6 +12,8 @@ package com.facebook.react.uimanager; import android.util.SparseArray; import android.util.SparseBooleanArray; +import com.facebook.react.common.SingleThreadAsserter; + /** * Simple container class to keep track of {@link ReactShadowNode}s associated with a particular * UIManagerModule instance. @@ -20,19 +22,25 @@ import android.util.SparseBooleanArray; private final SparseArray mTagsToCSSNodes; private final SparseBooleanArray mRootTags; + private final SingleThreadAsserter mThreadAsserter; public ShadowNodeRegistry() { mTagsToCSSNodes = new SparseArray<>(); mRootTags = new SparseBooleanArray(); + mThreadAsserter = new SingleThreadAsserter(); } public void addRootNode(ReactShadowNode node) { + // TODO(6242243): This should be asserted... but UIManagerModule is + // thread-unsafe and calls this on the wrong thread. + //mThreadAsserter.assertNow(); int tag = node.getReactTag(); mTagsToCSSNodes.put(tag, node); mRootTags.put(tag, true); } public void removeRootNode(int tag) { + mThreadAsserter.assertNow(); if (!mRootTags.get(tag)) { throw new IllegalViewOperationException( "View with tag " + tag + " is not registered as a root view"); @@ -43,10 +51,12 @@ import android.util.SparseBooleanArray; } public void addNode(ReactShadowNode node) { + mThreadAsserter.assertNow(); mTagsToCSSNodes.put(node.getReactTag(), node); } public void removeNode(int tag) { + mThreadAsserter.assertNow(); if (mRootTags.get(tag)) { throw new IllegalViewOperationException( "Trying to remove root node " + tag + " without using removeRootNode!"); @@ -55,18 +65,22 @@ import android.util.SparseBooleanArray; } public ReactShadowNode getNode(int tag) { + mThreadAsserter.assertNow(); return mTagsToCSSNodes.get(tag); } public boolean isRootNode(int tag) { + mThreadAsserter.assertNow(); return mRootTags.get(tag); } public int getRootNodeCount() { + mThreadAsserter.assertNow(); return mRootTags.size(); } public int getRootTag(int index) { + mThreadAsserter.assertNow(); return mRootTags.keyAt(index); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index 3aa56e2f2..b1d52cf0f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -99,6 +99,7 @@ public class UIImplementation { rootCSSNode.setThemedContext(context); rootCSSNode.setStyleWidth(width); rootCSSNode.setStyleHeight(height); + mShadowNodeRegistry.addRootNode(rootCSSNode); // register it within NativeViewHierarchyManager diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java index abfed9d68..12bbc1e3a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java @@ -141,9 +141,7 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements * CatalystApplicationFragment as an example. * * TODO(6242243): Make addMeasuredRootView thread safe - * NB: this method is horribly not-thread-safe, the only reason it works right now is because - * it's called exactly once and is called before any JS calls are made. As soon as that fact no - * longer holds, this method will need to be fixed. + * NB: this method is horribly not-thread-safe. */ public int addMeasuredRootView(final SizeMonitoringFrameLayout rootView) { final int tag = mNextRootViewTag;