Actually close packager websocket connection when destroying instance

Summary:
Ugh. We never actually closed the websocket connection. Furthermore, upon calling `closeQuietly()`, `onClose()` is called, which does `reconnect()`. Beautiful. This results in `ReactInstanceManager` leaking after calling `destroy()` and nullifying all references to it.

To fix this I made sure `closeQuietly()` actually closes the connection for good, **and** made sure we actually call it when destroying an instance.

Reviewed By: AaaChiuuu

Differential Revision: D3835023

fbshipit-source-id: 31811805dd97b725ea5887cffed9bed49addda83

ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java

@@ -79,7 +79,7 @@ public class DevServerHelper {
   }
 
   public interface PackagerCommandListener {
-    void onReload();
+    void onPackagerReloadCommand();
   }
 
   public interface PackagerStatusCallback {
@@ -88,15 +88,15 @@ public class DevServerHelper {
 
   private final DevInternalSettings mSettings;
   private final OkHttpClient mClient;
-  private final JSPackagerWebSocketClient mPackagerConnection;
   private final Handler mRestartOnChangePollingHandler;
 
   private boolean mOnChangePollingEnabled;
+  private @Nullable JSPackagerWebSocketClient mPackagerConnection;
   private @Nullable OkHttpClient mOnChangePollingClient;
   private @Nullable OnServerContentChangeListener mOnServerContentChangeListener;
   private @Nullable Call mDownloadBundleFromURLCall;
 
-  public DevServerHelper(DevInternalSettings settings, final PackagerCommandListener commandListener) {
+  public DevServerHelper(DevInternalSettings settings) {
     mSettings = settings;
     mClient = new OkHttpClient.Builder()
       .connectTimeout(HTTP_CONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS)
@@ -105,18 +105,32 @@ public class DevServerHelper {
       .build();
 
     mRestartOnChangePollingHandler = new Handler();
+  }
+
+  public void openPackagerConnection(final PackagerCommandListener commandListener) {
+    if (mPackagerConnection != null) {
+      FLog.w(ReactConstants.TAG, "Packager connection already open, nooping.");
+      return;
+    }
     mPackagerConnection = new JSPackagerWebSocketClient(getPackagerConnectionURL(),
         new JSPackagerWebSocketClient.JSPackagerCallback() {
           @Override
           public void onMessage(String target, String action) {
             if (commandListener != null && "bridge".equals(target) && "reload".equals(action)) {
-              commandListener.onReload();
+              commandListener.onPackagerReloadCommand();
             }
           }
         });
     mPackagerConnection.connect();
   }
 
+  public void closePackagerConnection() {
+    if (mPackagerConnection != null) {
+      mPackagerConnection.closeQuietly();
+      mPackagerConnection = null;
+    }
+  }
+
   /** Intent action for reloading the JS */
   public static String getReloadAppAction(Context context) {
     return context.getPackageName() + RELOAD_APP_ACTION_SUFFIX;

ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java

@@ -48,6 +48,7 @@ import com.facebook.react.bridge.UiThreadUtil;
 import com.facebook.react.common.ReactConstants;
 import com.facebook.react.common.ShakeDetector;
 import com.facebook.react.common.futures.SimpleSettableFuture;
+import com.facebook.react.devsupport.DevServerHelper.PackagerCommandListener;
 import com.facebook.react.devsupport.StackTraceHelper.StackFrame;
 import com.facebook.react.modules.debug.DeveloperSettings;
 
@@ -82,7 +83,7 @@ import okhttp3.RequestBody;
  * {@code <activity android:name="com.facebook.react.devsupport.DevSettingsActivity"/>}
  * {@code <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>}
  */
-public class DevSupportManagerImpl implements DevSupportManager {
+public class DevSupportManagerImpl implements DevSupportManager, PackagerCommandListener {
 
   private static final int JAVA_ERROR_COOKIE = -1;
   private static final int JSEXCEPTION_ERROR_COOKIE = -1;
@@ -120,7 +121,6 @@ public class DevSupportManagerImpl implements DevSupportManager {
   private int mLastErrorCookie = 0;
   private @Nullable ErrorType mLastErrorType;
 
-
   private static class JscProfileTask extends AsyncTask<String, Void, Void> {
     private static final MediaType JSON =
       MediaType.parse("application/json; charset=utf-8");
@@ -178,19 +178,7 @@ public class DevSupportManagerImpl implements DevSupportManager {
     mApplicationContext = applicationContext;
     mJSAppBundleName = packagerPathForJSBundleName;
     mDevSettings = new DevInternalSettings(applicationContext, this);
-    mDevServerHelper = new DevServerHelper(
-        mDevSettings,
-        new DevServerHelper.PackagerCommandListener() {
-          @Override
-          public void onReload() {
-            UiThreadUtil.runOnUiThread(new Runnable() {
-              @Override
-              public void run() {
-                handleReloadJS();
-              }
-            });
-          }
-        });
+    mDevServerHelper = new DevServerHelper(mDevSettings);
 
     // Prepare shake gesture detector (will be started/stopped from #reload)
     mShakeDetector = new ShakeDetector(new ShakeDetector.ShakeListener() {
@@ -237,8 +225,11 @@ public class DevSupportManagerImpl implements DevSupportManager {
       if (e instanceof JSException) {
         FLog.e(ReactConstants.TAG, "Exception in native call from JS", e);
         // TODO #11638796: convert the stack into something useful
-        showNewError(e.getMessage() + "\n\n" + ((JSException) e).getStack(), new StackFrame[] {},
-                     JSEXCEPTION_ERROR_COOKIE, ErrorType.JS);
+        showNewError(
+            e.getMessage() + "\n\n" + ((JSException) e).getStack(),
+            new StackFrame[] {},
+            JSEXCEPTION_ERROR_COOKIE,
+            ErrorType.JS);
       } else {
         showNewJavaError(e.getMessage(), e);
       }
@@ -388,7 +379,7 @@ public class DevSupportManagerImpl implements DevSupportManager {
               }
             });
     options.put(
-      mApplicationContext.getString(R.string.catalyst_element_inspector),
+        mApplicationContext.getString(R.string.catalyst_element_inspector),
         new DevOptionHandler() {
           @Override
           public void onOptionSelected() {
@@ -674,6 +665,16 @@ public class DevSupportManagerImpl implements DevSupportManager {
     return mLastErrorStack;
   }
 
+  @Override
+  public void onPackagerReloadCommand() {
+    UiThreadUtil.runOnUiThread(new Runnable() {
+      @Override
+      public void run() {
+        handleReloadJS();
+      }
+    });
+  }
+
   private void updateLastErrorInfo(
       final String message,
       final StackFrame[] stack,
@@ -802,6 +803,7 @@ public class DevSupportManagerImpl implements DevSupportManager {
         mIsReceiverRegistered = true;
       }
 
+      mDevServerHelper.openPackagerConnection(this);
       if (mDevSettings.isReloadOnJSChangeEnabled()) {
         mDevServerHelper.startPollingOnChangeEndpoint(
             new DevServerHelper.OnServerContentChangeListener() {
@@ -841,6 +843,7 @@ public class DevSupportManagerImpl implements DevSupportManager {
         mDevOptionsDialog.dismiss();
       }
 
+      mDevServerHelper.closePackagerConnection();
       mDevServerHelper.stopPollingOnChangeEndpoint();
     }
   }

ReactAndroid/src/main/java/com/facebook/react/devsupport/JSPackagerWebSocketClient.java

@@ -40,6 +40,7 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
 
   private final String mUrl;
   private final Handler mHandler;
+  private boolean mClosed = false;
   private boolean mSuppressConnectionErrors;
 
   public interface JSPackagerCallback {
@@ -57,6 +58,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
   }
 
   public void connect() {
+    if (mClosed) {
+      throw new IllegalStateException("Can't connect closed client");
+    }
     OkHttpClient  httpClient = new OkHttpClient.Builder()
         .connectTimeout(10, TimeUnit.SECONDS)
         .writeTimeout(10, TimeUnit.SECONDS)
@@ -69,6 +73,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
   }
 
   private void reconnect() {
+    if (mClosed) {
+      throw new IllegalStateException("Can't reconnect closed client");
+    }
     if (!mSuppressConnectionErrors) {
       FLog.w(TAG, "Couldn't connect to packager, will silently retry");
       mSuppressConnectionErrors = true;
@@ -77,12 +84,21 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
       new Runnable() {
         @Override
         public void run() {
-          connect();
+          // check that we haven't been closed in the meantime
+          if (!mClosed) {
+            connect();
+          }
         }
-      }, RECONNECT_DELAY_MS);
+      },
+      RECONNECT_DELAY_MS);
   }
 
   public void closeQuietly() {
+    mClosed = true;
+    closeWebSocketQuietly();
+  }
+
+  private void closeWebSocketQuietly() {
     if (mWebSocket != null) {
       try {
         mWebSocket.close(1000, "End of session");
@@ -151,7 +167,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
     if (mWebSocket != null) {
       abort("Websocket exception", e);
     }
-    reconnect();
+    if (!mClosed) {
+      reconnect();
+    }
   }
 
   @Override
@@ -163,7 +181,9 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
   @Override
   public void onClose(int code, String reason) {
     mWebSocket = null;
-    reconnect();
+    if (!mClosed) {
+      reconnect();
+    }
   }
 
   @Override
@@ -173,6 +193,6 @@ public class JSPackagerWebSocketClient implements WebSocketListener {
 
   private void abort(String message, Throwable cause) {
     FLog.e(TAG, "Error occurred, shutting down websocket connection: " + message, cause);
-    closeQuietly();
+    closeWebSocketQuietly();
   }
 }