Re-land: Pass in EventEmitter during View CREATE and Preallocation

Summary:
This is a re-land of D28810022 (https://github.com/facebook/react-native/commit/7e9c741146e1b3aa542ecfe138765e95ddddac3f), which was reverted due to T92179998. The fix is in D28938530. This issue could also be resolved by preventing more view preallocations (D28811419 (https://github.com/facebook/react-native/commit/8ca18f0b602a08cd9a5c9f6681bb5dc74e0d34f7)).

 ---

EventEmitter is not transmitted from C++ to Java until an UPDATE operation is enqueued.

Practically this usually happens "right away", but in the case of an Image component, especially, the EventEmitter could be missing while events are being fired from the native side (for example, loading events).

The fix is just to pass EventEmitter in sooner, in both Create and Preallocate. There should be no ill effect since EventEmitter is nullable anyway.

One potential side-effect: since Views can be PreAllocated and potentially never deallocated until StopSurface is called, this could result in more EventEmitter objects being leaked and retained from Java. I believe the fix is to remove PreAllocated Views more aggressively.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D28938637

fbshipit-source-id: c9e290a24ed15c28881e3eead4a5f580f66b288f
This commit is contained in:
Joshua Gross
2021-06-07 12:14:22 -07:00
committed by Facebook GitHub Bot
parent 792acff149
commit ac6d1982f4
5 changed files with 37 additions and 6 deletions
@@ -625,6 +625,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
final String componentName,
@Nullable ReadableMap props,
@Nullable Object stateWrapper,
@Nullable Object eventEmitterWrapper,
boolean isLayoutable) {
mMountItemDispatcher.addPreAllocateMountItem(
@@ -634,6 +635,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
getFabricComponentName(componentName),
props,
(StateWrapper) stateWrapper,
(EventEmitterWrapper) eventEmitterWrapper,
isLayoutable));
}
@@ -169,7 +169,8 @@ static inline void computeBufferSizes(
batchMountItemIntsSize += getIntBufferSizeForType(mountItemType);
if (mountItemType == CppMountItem::Type::Create) {
batchMountItemObjectsSize += 3; // component name, props, state
batchMountItemObjectsSize +=
4; // component name, props, state, event emitter
}
}
@@ -884,6 +885,13 @@ void Binding::schedulerDidFinishTransaction(
cStateWrapper->state_ = mountItem.newChildShadowView.state;
}
// Do not hold a reference to javaEventEmitter from the C++ side.
SharedEventEmitter eventEmitter =
mountItem.newChildShadowView.eventEmitter;
auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
cEventEmitter->eventEmitter = eventEmitter;
temp[0] = mountItem.newChildShadowView.tag;
temp[1] = isLayoutable;
env->SetIntArrayRegion(intBufferArray, intBufferPosition, 2, temp);
@@ -893,6 +901,7 @@ void Binding::schedulerDidFinishTransaction(
(*objBufferArray)[objBufferPosition++] = props.get();
(*objBufferArray)[objBufferPosition++] =
javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr;
(*objBufferArray)[objBufferPosition++] = javaEventEmitter.get();
} else if (mountItemType == CppMountItem::Type::Insert) {
temp[0] = mountItem.newChildShadowView.tag;
temp[1] = mountItem.parentShadowView.tag;
@@ -1143,8 +1152,13 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
static auto preallocateView =
jni::findClassStatic(Binding::UIManagerJavaDescriptor)
->getMethod<void(
jint, jint, jstring, ReadableMap::javaobject, jobject, jboolean)>(
"preallocateView");
jint,
jint,
jstring,
ReadableMap::javaobject,
jobject,
jobject,
jboolean)>("preallocateView");
// Do not hold onto Java object from C
// We DO want to hold onto C object from Java, since we don't know the
@@ -1156,6 +1170,12 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
cStateWrapper->state_ = shadowView.state;
}
// Do not hold a reference to javaEventEmitter from the C++ side.
SharedEventEmitter eventEmitter = shadowView.eventEmitter;
auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
cEventEmitter->eventEmitter = eventEmitter;
local_ref<ReadableMap::javaobject> props = castReadableMap(
ReadableNativeMap::newObjectCxxArgs(shadowView.props->rawProps));
auto component = getPlatformComponentName(shadowView);
@@ -1167,6 +1187,7 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
component.get(),
props.get(),
(javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr),
javaEventEmitter.get(),
isLayoutableShadowNode);
}
@@ -533,6 +533,7 @@ public class SurfaceMountingManager {
int reactTag,
@Nullable ReadableMap props,
@Nullable StateWrapper stateWrapper,
@Nullable EventEmitterWrapper eventEmitterWrapper,
boolean isLayoutable) {
if (isStopped()) {
return;
@@ -560,6 +561,7 @@ public class SurfaceMountingManager {
ViewState viewState = new ViewState(reactTag, view, viewManager);
viewState.mCurrentProps = propsDiffMap;
viewState.mStateWrapper = stateWrapper;
viewState.mEventEmitter = eventEmitterWrapper;
mTagToViewState.put(reactTag, viewState);
}
@@ -849,6 +851,7 @@ public class SurfaceMountingManager {
int reactTag,
@Nullable ReadableMap props,
@Nullable StateWrapper stateWrapper,
@Nullable EventEmitterWrapper eventEmitterWrapper,
boolean isLayoutable) {
UiThreadUtil.assertOnUiThread();
if (isStopped()) {
@@ -860,7 +863,7 @@ public class SurfaceMountingManager {
"View for component " + componentName + " with tag " + reactTag + " already exists.");
}
createView(componentName, reactTag, props, stateWrapper, isLayoutable);
createView(componentName, reactTag, props, stateWrapper, eventEmitterWrapper, isLayoutable);
}
@AnyThread
@@ -142,6 +142,7 @@ public class IntBufferBatchMountItem implements MountItem {
mIntBuffer[i++],
castToProps(mObjBuffer[j++]),
castToState(mObjBuffer[j++]),
castToEventEmitter(mObjBuffer[j++]),
mIntBuffer[i++] == 1);
} else if (type == INSTRUCTION_DELETE) {
surfaceMountingManager.deleteView(mIntBuffer[i++]);
@@ -202,7 +203,7 @@ public class IntBufferBatchMountItem implements MountItem {
for (int k = 0; k < numInstructions; k++) {
if (type == INSTRUCTION_CREATE) {
String componentName = getFabricComponentName((String) mObjBuffer[j++]);
j += 2;
j += 3;
s.append(
String.format(
"CREATE [%d] - layoutable:%d - %s\n",
@@ -14,6 +14,7 @@ import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.facebook.common.logging.FLog;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.fabric.events.EventEmitterWrapper;
import com.facebook.react.fabric.mounting.MountingManager;
import com.facebook.react.fabric.mounting.SurfaceMountingManager;
import com.facebook.react.uimanager.StateWrapper;
@@ -26,6 +27,7 @@ public class PreAllocateViewMountItem implements MountItem {
private final int mReactTag;
private final @Nullable ReadableMap mProps;
private final @Nullable StateWrapper mStateWrapper;
private final @Nullable EventEmitterWrapper mEventEmitterWrapper;
private final boolean mIsLayoutable;
public PreAllocateViewMountItem(
@@ -34,11 +36,13 @@ public class PreAllocateViewMountItem implements MountItem {
@NonNull String component,
@Nullable ReadableMap props,
@NonNull StateWrapper stateWrapper,
@Nullable EventEmitterWrapper eventEmitterWrapper,
boolean isLayoutable) {
mComponent = component;
mSurfaceId = surfaceId;
mProps = props;
mStateWrapper = stateWrapper;
mEventEmitterWrapper = eventEmitterWrapper;
mReactTag = reactTag;
mIsLayoutable = isLayoutable;
}
@@ -58,7 +62,7 @@ public class PreAllocateViewMountItem implements MountItem {
return;
}
surfaceMountingManager.preallocateView(
mComponent, mReactTag, mProps, mStateWrapper, mIsLayoutable);
mComponent, mReactTag, mProps, mStateWrapper, mEventEmitterWrapper, mIsLayoutable);
}
@Override