From 7e9c741146e1b3aa542ecfe138765e95ddddac3f Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 1 Jun 2021 15:14:29 -0700 Subject: [PATCH] Pass in EventEmitter during View CREATE and Preallocation Summary: 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: mdvacca Differential Revision: D28810022 fbshipit-source-id: ae4c8b4eefe619d9a99fa5f90f612d6dd4880da5 --- .../react/fabric/FabricUIManager.java | 2 ++ .../com/facebook/react/fabric/jni/Binding.cpp | 27 ++++++++++++++++--- .../mounting/SurfaceMountingManager.java | 5 +++- .../mountitems/IntBufferBatchMountItem.java | 3 ++- .../mountitems/PreAllocateViewMountItem.java | 6 ++++- 5 files changed, 37 insertions(+), 6 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index 69ce09fb4a0..b7fcd2e0f8e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -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)); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index a141f5343e9..8cb2ebc858c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -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 } } @@ -899,6 +900,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); @@ -908,6 +916,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; @@ -1157,8 +1166,13 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation( static auto preallocateView = jni::findClassStatic(Binding::UIManagerJavaDescriptor) ->getMethod( - "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 @@ -1170,6 +1184,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 props = castReadableMap( ReadableNativeMap::newObjectCxxArgs(shadowView.props->rawProps)); auto component = getPlatformComponentName(shadowView); @@ -1181,6 +1201,7 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation( component.get(), props.get(), (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr), + javaEventEmitter.get(), isLayoutableShadowNode); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index e61f26a2dd6..4e1b3e31f08 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -529,6 +529,7 @@ public class SurfaceMountingManager { int reactTag, @Nullable ReadableMap props, @Nullable StateWrapper stateWrapper, + @Nullable EventEmitterWrapper eventEmitterWrapper, boolean isLayoutable) { if (isStopped()) { return; @@ -556,6 +557,7 @@ public class SurfaceMountingManager { ViewState viewState = new ViewState(reactTag, view, viewManager); viewState.mCurrentProps = propsDiffMap; viewState.mStateWrapper = stateWrapper; + viewState.mEventEmitter = eventEmitterWrapper; mTagToViewState.put(reactTag, viewState); } @@ -845,6 +847,7 @@ public class SurfaceMountingManager { int reactTag, @Nullable ReadableMap props, @Nullable StateWrapper stateWrapper, + @Nullable EventEmitterWrapper eventEmitterWrapper, boolean isLayoutable) { UiThreadUtil.assertOnUiThread(); if (isStopped()) { @@ -856,7 +859,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 diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 9d2aa1d63c8..cd4bb8c5f61 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -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", diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index 4516d60dd85..5a0f5241bc8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -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