Remove premature optimization in FabricUIManager; could be causing stopSurface inconsistencies

Summary:
There is an optimization in FabricUIManager.surfaceActiveForExecution that ensures it returns the same value (true or false)
for a given SurfaceId for a single frame (the value is cached until the next frame).

It seems like this can be causing a few different crashes, in a couple different ways:

1) If StopSurface is called off the UI thread, in the middle of a batch of operations (probably less likely to cause problems)
2) If StopSurface is called on the UI thread, in between different operations; the latter operations will still execute because the `true` value of `surfaceActiveForExecution` was cached.

I don't think that this optimization was providing much for us, and could be causing crashes. Remove it for now and we'll analyze impact on crashes and perf.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D25379970

fbshipit-source-id: 2c15c971bd0c828e1d38a34662d93293271041b2
This commit is contained in:
Joshua Gross
2020-12-07 17:10:00 -08:00
committed by Facebook GitHub Bot
parent 72d29ee7d7
commit 3cdef265ae
@@ -168,12 +168,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
@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.
@@ -755,7 +749,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
didDispatchItems = dispatchMountItems();
} catch (Throwable e) {
mReDispatchCounter = 0;
mLastExecutedMountItemSurfaceId = -1;
throw e;
} finally {
// Clean up after running dispatchMountItems - even if an exception was thrown
@@ -782,7 +775,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
tryDispatchMountItems();
}
mReDispatchCounter = 0;
mLastExecutedMountItemSurfaceId = -1;
}
@Nullable
@@ -860,25 +852,22 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
*/
@UiThread
@ThreadConfined(UI)
private boolean surfaceActiveForExecution(int surfaceId, String context) {
if (mLastExecutedMountItemSurfaceId != surfaceId) {
mLastExecutedMountItemSurfaceId = surfaceId;
mLastExecutedMountItemSurfaceIdActive = mReactContextForRootTag.get(surfaceId) != null;
private boolean isSurfaceActiveForExecution(int surfaceId, String context) {
boolean surfaceActive = 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));
}
// If there are many MountItems with the same SurfaceId, we only
// log a warning for the first one that is skipped.
if (!surfaceActive) {
ReactSoftException.logSoftException(
TAG,
new ReactNoCrashSoftException(
"dispatchMountItems: skipping "
+ context
+ ", because surface not available: "
+ surfaceId));
}
return mLastExecutedMountItemSurfaceIdActive;
return surfaceActive;
}
private static void printMountItem(MountItem mountItem, String prefix) {
@@ -970,7 +959,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
+ preMountItemsToDispatch.size());
for (PreAllocateViewMountItem preMountItem : preMountItemsToDispatch) {
if (surfaceActiveForExecution(
if (isSurfaceActiveForExecution(
preMountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) {
preMountItem.execute(mMountingManager);
}
@@ -996,7 +985,7 @@ 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;
if (!surfaceActiveForExecution(
if (!isSurfaceActiveForExecution(
batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) {
continue;
}
@@ -1006,7 +995,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
// TODO T68118357: clean up this logic and simplify this method overall
if (mountItem instanceof IntBufferBatchMountItem) {
IntBufferBatchMountItem batchMountItem = (IntBufferBatchMountItem) mountItem;
if (!surfaceActiveForExecution(
if (!isSurfaceActiveForExecution(
batchMountItem.getRootTag(), "dispatchMountItems IntBufferBatchMountItem")) {
continue;
}
@@ -1065,14 +1054,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
break;
}
if (surfaceActiveForExecution(
if (isSurfaceActiveForExecution(
preMountItemToDispatch.getRootTag(), "dispatchPreMountItems")) {
preMountItemToDispatch.execute(mMountingManager);
}
}
} finally {
mInDispatch = false;
mLastExecutedMountItemSurfaceId = -1;
}
Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);