Fixed crash related to ReactContext#getJSModule()

[ifsnow]

I faced the crash as below.

Fatal Exception: java.lang.RuntimeException: Tried to access a JS module before the React instance was fully set up. Calls to ReactContext#getJSModule should only happen once initialize() has been called on your native module.
       at com.facebook.react.bridge.ReactContext.getJSModule(ReactContext.java:120)
       at com.facebook.reactnative.androidsdk.FBAccessTokenModule$1.onCurrentAccessTokenChanged(FBAccessTokenModule.java:51)
       at com.facebook.AccessTokenTracker$CurrentAccessTokenBroadcastReceiver.onReceive(AccessTokenTracker.java:110)
       at androidx.localbroadcastmanager.content.LocalBroadcastManager.executePendingBroadcasts(LocalBroadcastManager.java:313)
       at androidx.localbroadcastmanager.content.LocalBroadcastManager$1.handleMessage(LocalBroadcastManager.java:121)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loop(Looper.java:214)
       at android.app.ActivityThread.main(ActivityThread.java:7094)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:975)

To solve this problem, we need to check if ReactContext is initialized.

[mikehardy]

@matt-oakes I think this is related to your PR #666 merged by @janicduplessis - what do you guys think? I'm going to integrate this locally via patch-package to see if it mitigates the crashes I see (as described in #675)

[mikehardy]

In case anyone else wants to try it, this is relatively easy to paste into a file for patch-package consumption:

mike@isabela:~/work/Kullki/ksocialscore/packages/public-app (onboarding) % cat patches/react-native-fbsdk+1.1.1.patch 
diff --git a/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java b/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
index ee66e1d..bfdf5d7 100644
--- a/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
+++ b/node_modules/react-native-fbsdk/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java
@@ -47,9 +47,11 @@ public class FBAccessTokenModule extends ReactContextBaseJavaModule {
     private final AccessTokenTracker accessTokenTracker = new AccessTokenTracker() {
         @Override
         protected void onCurrentAccessTokenChanged(AccessToken oldAccessToken, AccessToken currentAccessToken) {
-            mReactContext
-                    .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
-                    .emit(CHANGE_EVENT_NAME, currentAccessToken == null ? null : Utility.accessTokenToReactMap(currentAccessToken));
+            if (mReactContext.hasActiveCatalystInstance()) {
+                mReactContext
+                        .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
+                        .emit(CHANGE_EVENT_NAME, currentAccessToken == null ? null : Utility.accessTokenToReactMap(currentAccessToken));
+            }
         }
     };
 

[vforvasile]

@ifsnow is your solution safe for production?

[mikehardy]

@vforvasile for what it's worth, after reviewing it myself it seemed correct, so I'm using it in production now via that patch I posted above (in combination with patch-package module to apply it for all devs + environments automagically) and it's working fine. No more crash reports.

[mikehardy]

this is working for me in production

[ifsnow]

@vforvasile Sure. As @mikehardy said, it works fine without any problem in production.

[janicduplessis]

Looks like the callback gets called outside the lifecycle of the native module. Instead can we create the AccessTokenTracker instance in the native module initialize method and call stopTracking in onCatalystInstanceDestroy.

[chakrihacker]

Can I send a PR with the changes you requested @janicduplessis ??

[chakrihacker]

@ifsnow are you gonna update this PR??

[ifsnow]

@chakrihacker What update are you talking about? My app with this patch has been working well for months without any crashes.

[chakrihacker]

Your PR works well and most of the native modules are following this way of checking, but @janicduplessis wanted a refactor with initializing and destroy methods so it would be nice

[mikehardy]

Yeah - @ifsnow mine is working also, but if it's not upstream or planned to go upstream, it's a dead PR and/or extra patch weight you have to carry. The idea is to upstream it so we don't crash and we don't have to carry a patch :-). Seems like @chakrihacker is up for it ?, in my world that mean "go for it @chakrihacker" :-)

[luancurti]

@janicduplessis Can we merge this PR and @chakrihacker made another PR to do this refactor and release another version of this library? This PR is opened for two months and is very critical, I have a lot of crashes in production :(

[ifsnow]

I agree with @luancurti's opinion. This PR has proven to work well in other projects. But, it's not easy to apply the new idea to production service without verification. I think it's a good idea to approach them in stages.

[janicduplessis]

Merged @chakrihacker version of this PR, thanks!