Ignore PreAllocateViewMountItem if it's associated with a stopped surface

Summary:
Because of the previous diffs there's an increased chance of race conditions between JS executing and queuing up PreAllocateViewMountItems for surfaces that are stopped. Make sure those are ignored if they're queued up and a surface has been stopped.

Currently stopSurface only happens on the UI thread; PreAllocateViewMountItems can be queued from any thread, but are only executed on the UI thread. So once a batch of items starts executing, there's no race between teardown and execution: we just need to make sure we check that the surface is still running initially.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D21965839

fbshipit-source-id: 0241dc171022cc923b7e38dcd110d664096dde79
This commit is contained in:
Joshua Gross
2020-06-09 23:23:11 -07:00
committed by Facebook GitHub Bot
parent 03174f1cad
commit 912dac24fc
2 changed files with 59 additions and 16 deletions
@@ -134,13 +134,19 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
@GuardedBy("mPreMountItemsLock")
@NonNull
private ArrayDeque<MountItem> mPreMountItems =
private ArrayDeque<PreAllocateViewMountItem> mPreMountItems =
new ArrayDeque<>(PRE_MOUNT_ITEMS_INITIAL_SIZE_ARRAY);
@ThreadConfined(UI)
@NonNull
private final DispatchUIFrameCallback mDispatchUIFrameCallback;
@ThreadConfined(UI)
private int mLastExecutedMountItemSurfaceId = -1;
@ThreadConfined(UI)
private boolean mLastExecutedMountItemSurfaceIdActive = false;
/**
* This is used to keep track of whether or not the FabricUIManager has been destroyed. Once the
* Catalyst instance is being destroyed, we should cease all operation here.
@@ -674,9 +680,9 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
}
}
private ArrayDeque<MountItem> getAndResetPreMountItems() {
private ArrayDeque<PreAllocateViewMountItem> getAndResetPreMountItems() {
synchronized (mPreMountItemsLock) {
ArrayDeque<MountItem> result = mPreMountItems;
ArrayDeque<PreAllocateViewMountItem> result = mPreMountItems;
if (result.isEmpty()) {
return null;
}
@@ -685,6 +691,38 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
}
}
/**
* Check if a surfaceId is active and ready for MountItems to be executed against it. It is safe
* and cheap to call this repeatedly because we expect many operations to be batched with the same
* surfaceId in a row and we memoize the parameters and results.
*
* @param surfaceId
* @param context
* @return
*/
@UiThread
@ThreadConfined(UI)
private boolean surfaceActiveForExecution(int surfaceId, String context) {
if (mLastExecutedMountItemSurfaceId != surfaceId) {
mLastExecutedMountItemSurfaceId = surfaceId;
mLastExecutedMountItemSurfaceIdActive = mReactContextForRootTag.get(surfaceId) != null;
// If there are many MountItems with the same SurfaceId, we only
// log a warning for the first one that is skipped.
if (!mLastExecutedMountItemSurfaceIdActive) {
ReactSoftException.logSoftException(
TAG,
new ReactNoCrashSoftException(
"dispatchMountItems: skipping "
+ context
+ ", because surface not available: "
+ surfaceId));
}
}
return mLastExecutedMountItemSurfaceIdActive;
}
@UiThread
@ThreadConfined(UI)
/** Nothing should call this directly except for `tryDispatchMountItems`. */
@@ -756,7 +794,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
// If there are MountItems to dispatch, we make sure all the "pre mount items" are executed
// first
ArrayDeque<MountItem> mPreMountItemsToDispatch = getAndResetPreMountItems();
ArrayDeque<PreAllocateViewMountItem> mPreMountItemsToDispatch = getAndResetPreMountItems();
if (mPreMountItemsToDispatch != null) {
Systrace.beginSection(
@@ -765,7 +803,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
+ mPreMountItemsToDispatch.size());
while (!mPreMountItemsToDispatch.isEmpty()) {
mPreMountItemsToDispatch.pollFirst().execute(mMountingManager);
PreAllocateViewMountItem mountItem = mPreMountItemsToDispatch.pollFirst();
if (surfaceActiveForExecution(
mountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) {
mountItem.execute(mMountingManager);
}
}
Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);
@@ -792,14 +834,8 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
// TODO T68118357: clean up this logic and simplify this method overall
if (mountItem instanceof BatchMountItem) {
BatchMountItem batchMountItem = (BatchMountItem) mountItem;
int rootTag = batchMountItem.getRootTag();
if (mReactContextForRootTag.get(rootTag) == null) {
ReactSoftException.logSoftException(
TAG,
new ReactNoCrashSoftException(
"dispatchMountItems: skipping batched item: surface not available ["
+ rootTag
+ "]"));
if (!surfaceActiveForExecution(
batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) {
continue;
}
}
@@ -855,15 +891,18 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
break;
}
MountItem preMountItemsToDispatch;
PreAllocateViewMountItem preMountItemToDispatch;
synchronized (mPreMountItemsLock) {
if (mPreMountItems.isEmpty()) {
break;
}
preMountItemsToDispatch = mPreMountItems.pollFirst();
preMountItemToDispatch = mPreMountItems.pollFirst();
}
preMountItemsToDispatch.execute(mMountingManager);
if (surfaceActiveForExecution(
preMountItemToDispatch.getRootTag(), "dispatchPreMountItems")) {
preMountItemToDispatch.execute(mMountingManager);
}
}
} finally {
mInDispatch = false;
@@ -47,6 +47,10 @@ public class PreAllocateViewMountItem implements MountItem {
mIsLayoutable = isLayoutable;
}
public int getRootTag() {
return mRootTag;
}
@Override
public void execute(@NonNull MountingManager mountingManager) {
if (ENABLE_FABRIC_LOGS) {