Skip to content

Commit

Permalink
Rename "hasActiveCatalystInstance" to "hasActiveReactInstance" for cl…
Browse files Browse the repository at this point in the history
…arification

Summary:
Sometimes ```hasActiveCatalystInstance()``` is used to check if it's safe to access the CatalystInstance, which will still crash in Venice.

Previously we mitigate this by changing ```reactContext.hasActiveCatalystInstance()``` to ```reactContext.hasActiveCatalystInstance() || reactContext.isBridgeless()```.

To solve this for all and good the plan is:
1, Rename ```hasActiveCatalystInstance()``` to ```hasActiveReactInstance()``` so it won't sounds like CatalystInstance-only.
2, Implement hasActiveReactInstance() for Venice. D27343867
3, Remove previous mitigation. D27343952

This diff is the first step, by xbgs there are **58** non-generated callsites of  ```hasActiveCatalystInstance()``` in code base which are all renamed in this diff.

Changelog:
[Android][Changed] - Rename "hasActiveCatalystInstance" to "hasActiveReactInstance"

Reviewed By: mdvacca

Differential Revision: D27335055

fbshipit-source-id: 5b8ff5e09b79a492e910bb8f197e70fa1360bcef
  • Loading branch information
luluwu2032 authored and facebook-github-bot committed Mar 31, 2021
1 parent b86e52a commit dfa8eb0
Show file tree
Hide file tree
Showing 19 changed files with 27 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public void doFrame(long frameTimeNanos) {
}

private static void waitForJSIdle(ReactContext reactContext) {
if (!reactContext.hasActiveCatalystInstance()) {
if (!reactContext.hasActiveReactInstance()) {
return;
}
final CountDownLatch latch = new CountDownLatch(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ public void onNewIntent(Intent intent) {

private void toggleElementInspector() {
ReactContext currentContext = getCurrentReactContext();
if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
if (currentContext != null && currentContext.hasActiveReactInstance()) {
currentContext
.getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
.emit("toggleElementInspector", null);
Expand Down Expand Up @@ -859,7 +859,7 @@ public void detachRootView(ReactRoot reactRoot) {
if (mAttachedReactRoots.contains(reactRoot)) {
ReactContext currentContext = getCurrentReactContext();
mAttachedReactRoots.remove(reactRoot);
if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
if (currentContext != null && currentContext.hasActiveReactInstance()) {
detachViewFromInstance(reactRoot, currentContext.getCatalystInstance());
}
}
Expand Down Expand Up @@ -894,7 +894,7 @@ public List<ViewManager> getOrCreateViewManagers(
ReactApplicationContext context;
synchronized (mReactContextLock) {
context = (ReactApplicationContext) getCurrentReactContext();
if (context == null || !context.hasActiveCatalystInstance()) {
if (context == null || !context.hasActiveReactInstance()) {
return null;
}
}
Expand All @@ -920,7 +920,7 @@ public List<ViewManager> getOrCreateViewManagers(
ReactApplicationContext context;
synchronized (mReactContextLock) {
context = (ReactApplicationContext) getCurrentReactContext();
if (context == null || !context.hasActiveCatalystInstance()) {
if (context == null || !context.hasActiveReactInstance()) {
return null;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ public CatalystInstance getCatalystInstance() {
return Assertions.assertNotNull(mCatalystInstance);
}

public boolean hasActiveCatalystInstance() {
/** @return true if there is an non-null, alive react native instance */
public boolean hasActiveReactInstance() {
return mCatalystInstance != null && !mCatalystInstance.isDestroyed();
}

Expand All @@ -184,7 +185,7 @@ public LifecycleState getLifecycleState() {

public void addLifecycleEventListener(final LifecycleEventListener listener) {
mLifecycleEventListeners.add(listener);
if (hasActiveCatalystInstance() || isBridgeless()) {
if (hasActiveReactInstance() || isBridgeless()) {
switch (mLifecycleState) {
case BEFORE_CREATE:
case BEFORE_RESUME:
Expand Down Expand Up @@ -452,7 +453,7 @@ public JavaScriptContextHolder getJavaScriptContextHolder() {
}

public @Nullable JSIModule getJSIModule(JSIModuleType moduleType) {
if (!hasActiveCatalystInstance()) {
if (!hasActiveReactInstance()) {
throw new IllegalStateException(
"Unable to retrieve a JSIModule if CatalystInstance is not active.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ protected final ReactApplicationContext getReactApplicationContext() {
*/
@ThreadConfined(ANY)
protected @Nullable final ReactApplicationContext getReactApplicationContextIfActiveOrWarn() {
if (mReactApplicationContext.hasActiveCatalystInstance()
if (mReactApplicationContext.hasActiveReactInstance()
|| mReactApplicationContext.isBridgeless()) {
return mReactApplicationContext;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ public void run() {

@Nullable ReactContext context = mCurrentContext;
if (context == null
|| (!context.isBridgeless() && !context.hasActiveCatalystInstance())) {
|| (!context.isBridgeless() && !context.hasActiveReactInstance())) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private synchronized void startTask(final HeadlessJsTaskConfig taskConfig, int t
}
mActiveTasks.add(taskId);
mActiveTaskConfigs.put(taskId, new HeadlessJsTaskConfig(taskConfig));
if (reactContext.hasActiveCatalystInstance()) {
if (reactContext.hasActiveReactInstance()) {
reactContext
.getJSModule(AppRegistry.class)
.startHeadlessTask(taskId, taskConfig.getTaskKey(), taskConfig.getData());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void onChange(boolean selfChange) {

@Override
public void onChange(boolean selfChange, Uri uri) {
if (getReactApplicationContext().hasActiveCatalystInstance()) {
if (getReactApplicationContext().hasActiveReactInstance()) {
AccessibilityInfoModule.this.updateAndSendReduceMotionChangeEvent();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ private void sendEvent(String eventName, @Nullable Object data) {
// We don't gain anything interesting from logging here, and it's an extremely common
// race condition for an AppState event to be triggered as the Catalyst instance is being
// set up or torn down. So, just fail silently here.
if (!reactApplicationContext.hasActiveCatalystInstance()) {
if (!reactApplicationContext.hasActiveReactInstance()) {
return;
}
reactApplicationContext.getJSModule(RCTDeviceEventEmitter.class).emit(eventName, data);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public DatePickerDialogListener(final Promise promise) {

@Override
public void onDateSet(DatePicker view, int year, int month, int day) {
if (!mPromiseResolved && getReactApplicationContext().hasActiveCatalystInstance()) {
if (!mPromiseResolved && getReactApplicationContext().hasActiveReactInstance()) {
WritableMap result = new WritableNativeMap();
result.putString("action", ACTION_DATE_SET);
result.putInt("year", year);
Expand All @@ -74,7 +74,7 @@ public void onDateSet(DatePicker view, int year, int month, int day) {

@Override
public void onDismiss(DialogInterface dialog) {
if (!mPromiseResolved && getReactApplicationContext().hasActiveCatalystInstance()) {
if (!mPromiseResolved && getReactApplicationContext().hasActiveReactInstance()) {
WritableMap result = new WritableNativeMap();
result.putString("action", ACTION_DISMISSED);
mPromise.resolve(result);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void emitUpdateDimensionsEvent() {
return;
}

if (mReactApplicationContext.hasActiveCatalystInstance()) {
if (mReactApplicationContext.hasActiveReactInstance()) {
// Don't emit an event to JS if the dimensions haven't changed
WritableNativeMap displayMetrics =
DisplayMetricsHolder.getDisplayMetricsNativeMap(mFontScale);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public AlertFragmentListener(Callback callback) {
public void onClick(DialogInterface dialog, int which) {
if (!mCallbackConsumed) {
if (getReactApplicationContext().isBridgeless()
|| getReactApplicationContext().hasActiveCatalystInstance()) {
|| getReactApplicationContext().hasActiveReactInstance()) {
mCallback.invoke(ACTION_BUTTON_CLICKED, which);
mCallbackConsumed = true;
}
Expand All @@ -141,7 +141,7 @@ public void onClick(DialogInterface dialog, int which) {
public void onDismiss(DialogInterface dialog) {
if (!mCallbackConsumed) {
if (getReactApplicationContext().isBridgeless()
|| getReactApplicationContext().hasActiveCatalystInstance()) {
|| getReactApplicationContext().hasActiveReactInstance()) {
mCallback.invoke(ACTION_DISMISSED);
mCallbackConsumed = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ public boolean performAccessibilityAction(View host, int action, Bundle args) {
final WritableMap event = Arguments.createMap();
event.putString("actionName", mAccessibilityActionsMap.get(action));
ReactContext reactContext = (ReactContext) host.getContext();
if (reactContext.hasActiveCatalystInstance()) {
if (reactContext.hasActiveReactInstance()) {
final int reactTag = host.getId();
final int surfaceId = UIManagerHelper.getSurfaceId(reactContext);
UIManager uiManager = UIManagerHelper.getUIManager(reactContext, reactTag);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private static UIManager getUIManager(
}
// TODO T60461551: add tests to verify emission of events when the ReactContext is being turn
// down.
if (!context.hasActiveCatalystInstance()) {
if (!context.hasActiveReactInstance()) {
ReactSoftException.logSoftException(
"UIManagerHelper",
new ReactNoCrashSoftException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private RCTEventEmitter getEventEmitter(int reactTag) {
int type = ViewUtil.getUIManagerType(reactTag);
assert type == UIManagerType.DEFAULT;
if (mRCTEventEmitter == null) {
if (mReactContext.hasActiveCatalystInstance()) {
if (mReactContext.hasActiveReactInstance()) {
mRCTEventEmitter = mReactContext.getJSModule(RCTEventEmitter.class);
} else {
ReactSoftException.logSoftException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public long measure(
text, layout, sTextPaintInstance, themedReactContext);
WritableMap event = Arguments.createMap();
event.putArray("lines", lines);
if (themedReactContext.hasActiveCatalystInstance()) {
if (themedReactContext.hasActiveReactInstance()) {
themedReactContext
.getJSModule(RCTEventEmitter.class)
.receiveEvent(getReactTag(), "topTextLayout", event);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
mFrameTimeNanos = INITIAL_FRAME_TIME_NANOS;

mReactApplicationContextMock = mock(ReactApplicationContext.class);
PowerMockito.when(mReactApplicationContextMock.hasActiveCatalystInstance())
PowerMockito.when(mReactApplicationContextMock.hasActiveReactInstance())
.thenAnswer(
new Answer<Boolean>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void setUp() throws Exception {
mActivity = mActivityController.create().start().resume().get();

final ReactApplicationContext context = PowerMockito.mock(ReactApplicationContext.class);
PowerMockito.when(context.hasActiveCatalystInstance()).thenReturn(true);
PowerMockito.when(context.hasActiveReactInstance()).thenReturn(true);
PowerMockito.when(context, "getCurrentActivity").thenReturn(mActivity);

mDialogModule = new DialogModule(context);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
CatalystInstance reactInstance = mock(CatalystInstance.class);
ReactApplicationContext reactContext = mock(ReactApplicationContext.class);
when(reactContext.getCatalystInstance()).thenReturn(reactInstance);
when(reactContext.hasActiveCatalystInstance()).thenReturn(true);
when(reactContext.hasActiveReactInstance()).thenReturn(true);
when(reactContext.getJSModule(any(Class.class))).thenReturn(mEmitter);
mNetworkingModule = new NetworkingModule(reactContext, "", mHttpClient);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
CatalystInstance reactInstance = mock(CatalystInstance.class);
ReactApplicationContext reactContext = mock(ReactApplicationContext.class);
when(reactContext.getCatalystInstance()).thenReturn(reactInstance);
when(reactContext.hasActiveCatalystInstance()).thenReturn(true);
when(reactContext.hasActiveReactInstance()).thenReturn(true);

mCurrentTimeNs = 0;
mPostFrameCallbackHandler = new PostFrameCallbackHandler();
Expand Down

0 comments on commit dfa8eb0

Please sign in to comment.