From ac6d1982f4df55eca4da1cfc588dadc35bc005d0 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 7 Jun 2021 12:14:22 -0700 Subject: [PATCH] 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 --- .../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 d56799e8409..d0092602efa 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 51dc02b199e..73e43c2a18b 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 } } @@ -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( - "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 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); } 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 b2f5af0968f..004af2f6471 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 @@ -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 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