Summary:
For over a month I've been searching for a crash that occurs during Android's native `dispatchDraw` method on View. The stack traces didn't show anything useful - everything in the stack trace was native Android code,
not React Native code.

This also seems to only repro on certain vendors, and only on a very few React Native screens - I'm still not sure why either of those are the case, but from my repro, a *very* specific set of interactions needs to happen
to trigger this crash. See comments inline and an example stack trace.

Luckily, the fix is trivial. Since this code is used for animations, accessibility, and a number of other important interactions, I'm gating this change for now.

In general we must be careful to only mutate the View hierarchy only when we /know/ for certain it is safe to do so. Indirectly mutating the View hierarchy during measure, onMeasure, layout, dispatchDraw, etc, can all be
very dangerous. This is one of the last "escape hatches" that can cause view hierarchy modifications unexpectedly, so I think it's a very good idea to "secure" this further, and only update props synchronously here - and
ensure that other MountItems like `Delete` are definitely /not/ executed here.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D24639793

fbshipit-source-id: b6219ce882e8d2204b4d10bf99f6a1120a33cb5a
This commit is contained in:
Joshua Gross 2020-10-30 20:38:07 -07:00 коммит произвёл Facebook GitHub Bot
Родитель 2cd89d040b
Коммит dd9fd2acac
3 изменённых файлов: 75 добавлений и 27 удалений

Просмотреть файл

@ -74,4 +74,7 @@ public class ReactFeatureFlags {
/** Disable customDrawOrder in ReactViewGroup under Fabric only. */
public static boolean disableCustomDrawOrderFabric = false;
/** Potential bugfix for crashes caused by mutating the view hierarchy during onDraw. */
public static boolean enableDrawMutationFix = true;
}

Просмотреть файл

@ -566,18 +566,65 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
@Override
@UiThread
@ThreadConfined(UI)
public void synchronouslyUpdateViewOnUIThread(int reactTag, @NonNull ReadableMap props) {
public void synchronouslyUpdateViewOnUIThread(
final int reactTag, @NonNull final ReadableMap props) {
UiThreadUtil.assertOnUiThread();
int commitNumber = mCurrentSynchronousCommitNumber++;
// We are on the UI thread so this is safe to call. We try to flush any existing
// mount instructions that are queued.
// We are on the UI thread so this would otherwise be safe to call, *BUT* we don't know
// where we are on the callstack. Why isn't this safe, and why do we have additional safeguards
// here?
//
// A tangible example where this will cause a crash:
// 1. There are queued "delete" mutations
// 2. We're called by this stack trace:
// FabricUIManager.synchronouslyUpdateViewOnUIThread(FabricUIManager.java:574)
// PropsAnimatedNode.updateView(PropsAnimatedNode.java:114)
// NativeAnimatedNodesManager.updateNodes(NativeAnimatedNodesManager.java:655)
// NativeAnimatedNodesManager.handleEvent(NativeAnimatedNodesManager.java:521)
// NativeAnimatedNodesManager.onEventDispatch(NativeAnimatedNodesManager.java:483)
// EventDispatcherImpl.dispatchEvent(EventDispatcherImpl.java:116)
// ReactScrollViewHelper.emitScrollEvent(ReactScrollViewHelper.java:85)
// ReactScrollViewHelper.emitScrollEvent(ReactScrollViewHelper.java:46)
// ReactScrollView.onScrollChanged(ReactScrollView.java:285)
// ReactScrollView.onOverScrolled(ReactScrollView.java:808)
// android.view.View.overScrollBy(View.java:26052)
// android.widget.ScrollView.overScrollBy(ScrollView.java:2040)
// android.widget.ScrollView.computeScroll(ScrollView.java:1481)
// android.view.View.updateDisplayListIfDirty(View.java:20466)
if (!ReactFeatureFlags.enableDrawMutationFix) {
tryDispatchMountItems();
}
MountItem synchronousMountItem =
new MountItem() {
@Override
public void execute(@NonNull MountingManager mountingManager) {
try {
updatePropsMountItem(reactTag, props).execute(mountingManager);
} catch (Exception ex) {
// TODO T42943890: Fix animations in Fabric and remove this try/catch
ReactSoftException.logSoftException(
TAG,
new ReactNoCrashSoftException(
"Caught exception in synchronouslyUpdateViewOnUIThread", ex));
}
}
};
// If the reactTag exists, we assume that it might at the end of the next
// batch of MountItems. Otherwise, we try to execute immediately.
if (!mMountingManager.getViewExists(reactTag)) {
synchronized (mMountItemsLock) {
mMountItems.add(synchronousMountItem);
}
return;
}
ReactMarker.logFabricMarker(
ReactMarkerConstants.FABRIC_UPDATE_UI_MAIN_THREAD_START, null, commitNumber);
if (ENABLE_FABRIC_LOGS) {
FLog.d(
TAG,
@ -586,18 +633,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
(IS_DEVELOPMENT_ENVIRONMENT ? props.toHashMap().toString() : "<hidden>"));
}
updatePropsMountItem(reactTag, props).execute(mMountingManager);
} catch (Exception ex) {
// TODO T42943890: Fix animations in Fabric and remove this try/catch
ReactSoftException.logSoftException(
TAG,
new ReactNoCrashSoftException(
"Caught exception in synchronouslyUpdateViewOnUIThread", ex));
} finally {
synchronousMountItem.execute(mMountingManager);
ReactMarker.logFabricMarker(
ReactMarkerConstants.FABRIC_UPDATE_UI_MAIN_THREAD_END, null, commitNumber);
}
}
public void addUIManagerEventListener(UIManagerListener listener) {
mListeners.add(listener);

Просмотреть файл

@ -262,7 +262,8 @@ public class MountingManager {
}
}
private @NonNull ViewState getViewState(int tag) {
@NonNull
private ViewState getViewState(int tag) {
ViewState viewState = mTagToViewState.get(tag);
if (viewState == null) {
throw new RetryableMountingLayerException("Unable to find viewState view for tag " + tag);
@ -270,6 +271,10 @@ public class MountingManager {
return viewState;
}
public boolean getViewExists(int tag) {
return mTagToViewState.get(tag) != null;
}
private @Nullable ViewState getNullableViewState(int tag) {
return mTagToViewState.get(tag);
}