Fix device cleanup

Summary:
Fixes https://github.com/facebook/flipper/issues/1989

We had some self healing side effect that would destroy devices when registering a new device with the same serial, if they weren't yet. Redux isn't too happy about that, causing the attached crash.

Instead introduced a utility to destroy devices, and log an error if the device life cycle isn't respected by the device implementations, rather than crashing we will now just waste some memory.

Changelog: Fix a crash when disconnecting metro devices

Reviewed By: passy

Differential Revision: D26749214

fbshipit-source-id: 4c185ac521d44c1337fac8a9145440123b8b784c

desktop/app/src/__tests__/disconnect.node.tsx

@@ -17,6 +17,7 @@ import {
   PluginClient,
 } from 'flipper-plugin';
 import {registerNewClient} from '../dispatcher/server';
+import {destroyDevice} from '../reducers/connections';
 
 test('Devices can disconnect', async () => {
   const deviceplugin = new _SandyPluginDefinition(
@@ -89,7 +90,9 @@ test('New device with same serial removes & cleans the old one', async () => {
       },
     },
   );
-  const {device, store} = await createMockFlipperWithPlugin(deviceplugin);
+  const {device, store, logger} = await createMockFlipperWithPlugin(
+    deviceplugin,
+  );
 
   const instance = device.sandyPluginStates.get(deviceplugin.id)!;
 
@@ -98,6 +101,10 @@ test('New device with same serial removes & cleans the old one', async () => {
   expect(instance.instanceApi.destroy).toBeCalledTimes(0);
   expect(store.getState().connections.devices).toEqual([device]);
 
+  // calling destroy explicitly defeats the point of this test a bit,
+  // but we now print an error rather than proactively destroying the device,
+  // see https://github.com/facebook/flipper/issues/1989
+  destroyDevice(store, logger, device.serial);
   // submit a new device with same serial
   const device2 = new BaseDevice(
     device.serial,

desktop/app/src/dispatcher/androidDevice.tsx

@@ -20,6 +20,7 @@ import {promisify} from 'util';
 import {ServerPorts} from '../reducers/application';
 import {Client as ADBClient} from 'adbkit';
 import {addErrorNotification} from '../reducers/notifications';
+import {destroyDevice} from '../reducers/connections';
 
 function createDevice(
   adbClient: ADBClient,
@@ -235,9 +236,8 @@ export default (store: Store, logger: Logger) => {
       )
       .map((device) => device.serial);
 
-    store.dispatch({
+    reconnectedDevices.forEach((serial) => {
-      type: 'UNREGISTER_DEVICES',
+      destroyDevice(store, logger, serial);
-      payload: new Set(reconnectedDevices),
     });
 
     androidDevice.loadDevicePlugins(

desktop/app/src/dispatcher/iOSDevice.tsx

@@ -21,6 +21,7 @@ import IOSDevice from '../devices/IOSDevice';
 import {registerDeviceCallbackOnPlugins} from '../utils/onRegisterDevice';
 import {addErrorNotification} from '../reducers/notifications';
 import {getStaticPath} from '../utils/pathUtils';
+import {destroyDevice} from '../reducers/connections';
 
 type iOSSimulatorDevice = {
   state: 'Booted' | 'Shutdown' | 'Shutting Down';
@@ -127,6 +128,8 @@ function processDevices(
     if (currentDeviceIDs.has(udid)) {
       currentDeviceIDs.delete(udid);
     } else {
+      // clean up offline device
+      destroyDevice(store, logger, udid);
       logger.track('usage', 'register-device', {
         os: 'iOS',
         type: type,

desktop/app/src/dispatcher/metroDevice.tsx

@@ -13,6 +13,7 @@ import {registerDeviceCallbackOnPlugins} from '../utils/onRegisterDevice';
 import MetroDevice from '../devices/MetroDevice';
 import http from 'http';
 import {addErrorNotification} from '../reducers/notifications';
+import {destroyDevice} from '../reducers/connections';
 
 const METRO_PORT = 8081;
 const METRO_HOST = 'localhost';
@@ -75,18 +76,6 @@ export async function registerMetroDevice(
   );
 }
 
-async function unregisterDevices(store: Store, logger: Logger) {
-  logger.track('usage', 'unregister-device', {
-    os: 'Metro',
-    serial: METRO_URL,
-  });
-
-  store.dispatch({
-    type: 'UNREGISTER_DEVICES',
-    payload: new Set([METRO_URL]),
-  });
-}
-
 export default (store: Store, logger: Logger) => {
   let timeoutHandle: NodeJS.Timeout;
   let ws: WebSocket | undefined;
@@ -111,7 +100,7 @@ export default (store: Store, logger: Logger) => {
           unregistered = true;
           clearTimeout(guard);
           ws = undefined;
-          unregisterDevices(store, logger);
+          destroyDevice(store, logger, METRO_URL);
           scheduleNext();
         }
       };

desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx

@@ -9,7 +9,7 @@
 
 import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin';
 import {Store} from '../../';
-import {selectPlugin} from '../../reducers/connections';
+import {destroyDevice, selectPlugin} from '../../reducers/connections';
 import {
   _SandyPluginDefinition,
   _SandyDevicePluginInstance,
@@ -87,15 +87,12 @@ test('it should initialize device sandy plugins', async () => {
 });
 
 test('it should cleanup if device is removed', async () => {
-  const {device, store} = await createMockFlipperWithPlugin(TestPlugin);
+  const {device, store, logger} = await createMockFlipperWithPlugin(TestPlugin);
   const pluginInstance = device.sandyPluginStates.get(TestPlugin.id)!;
   expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0);
 
   // close device
-  store.dispatch({
+  destroyDevice(store, logger, device.serial);
-    type: 'UNREGISTER_DEVICES',
-    payload: new Set([device.serial]),
-  });
   expect(
     (pluginInstance.instanceApi as PluginApi).destroyStub,
   ).toHaveBeenCalledTimes(1);

desktop/app/src/reducers/connections.tsx

@@ -16,7 +16,7 @@ import type Client from '../Client';
 import type {UninitializedClient} from '../UninitializedClient';
 import {isEqual} from 'lodash';
 import {performance} from 'perf_hooks';
-import type {Actions} from '.';
+import type {Actions, Store} from '.';
 import {WelcomeScreenStaticView} from '../sandy-chrome/WelcomeScreen';
 import {getPluginKey, isDevicePluginDefinition} from '../utils/pluginUtils';
 import {deconstructClientId} from '../utils/clientUtils';
@@ -231,10 +231,9 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
         (device) => device.serial === payload.serial,
       );
       if (existing !== -1) {
-        console.debug(
+        console.warn(
           `Got a new device instance for already existing serial ${payload.serial}`,
         );
-        state.devices[existing].destroy();
         newDevices[existing] = payload;
       } else {
         newDevices.push(payload);
@@ -255,7 +254,11 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
             if (!deviceSerials.has(device.serial)) {
               return true;
             } else {
-              device.destroy();
+              if (device.connected.get()) {
+                console.warn(
+                  'Tried to unregister a device before it was destroyed',
+                );
+              }
               return false;
             }
           });
@@ -654,3 +657,20 @@ export function isPluginEnabled(
   const enabledAppPlugins = enabledPlugins[appInfo.app];
   return enabledAppPlugins && enabledAppPlugins.indexOf(pluginId) > -1;
 }
+
+export function destroyDevice(store: Store, logger: Logger, serial: string) {
+  const device = store
+    .getState()
+    .connections.devices.find((device) => device.serial === serial);
+  if (device) {
+    device.destroy();
+    logger.track('usage', 'unregister-device', {
+      os: device.os,
+      serial,
+    });
+    store.dispatch({
+      type: 'UNREGISTER_DEVICES',
+      payload: new Set([serial]),
+    });
+  }
+}

desktop/app/src/server.tsx

@@ -40,6 +40,7 @@ import {initSelfInpector} from './utils/self-inspection/selfInspectionUtils';
 import ClientDevice from './devices/ClientDevice';
 import BaseDevice from './devices/BaseDevice';
 import {sideEffect} from './utils/sideEffect';
+import {destroyDevice} from './reducers/connections';
 
 type ClientInfo = {
   connection: FlipperClientConnection<any, any> | null | undefined;
@@ -206,10 +207,7 @@ class Server extends EventEmitter {
         Object.values(clients).map((p) =>
           p.then((c) => this.removeConnection(c.id)),
         );
-        this.store.dispatch({
+        destroyDevice(this.store, this.logger, deviceId);
-          type: 'UNREGISTER_DEVICES',
-          payload: new Set([deviceId]),
-        });
       };
 
       ws.on('message', (rawMessage: any) => {

desktop/app/src/utils/js-client-server-utils/serverUtils.tsx

@@ -18,6 +18,7 @@ import {Payload, ConnectionStatus, ISubscriber} from 'rsocket-types';
 import {Flowable, Single} from 'rsocket-flowable';
 import Server from '../../server';
 import {buildClientId} from '../clientUtils';
+import {destroyDevice} from '../../reducers/connections';
 
 const connections: Map<number, JSClientFlipperConnection<any>> = new Map();
 
@@ -83,12 +84,7 @@ export function initJsEmulatorIPC(
           if (payload.kind == 'ERROR' || payload.kind == 'CLOSED') {
             console.debug(`Device disconnected ${client.id}`, 'server');
             flipperServer.removeConnection(client.id);
-            const toUnregister = new Set<string>();
+            destroyDevice(store, logger, jsDeviceId(windowId));
-            toUnregister.add(jsDeviceId(windowId));
-            store.dispatch({
-              type: 'UNREGISTER_DEVICES',
-              payload: toUnregister,
-            });
             connections.delete(windowId);
             availablePlugins.delete(windowId);
           }

desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx

@@ -16,6 +16,7 @@ import Server from '../../server';
 import {buildClientId} from '../clientUtils';
 import {selfInspectionClient} from './selfInspectionClient';
 import {flipperMessagesClientPlugin} from './plugins/FlipperMessagesClientPlugin';
+import {destroyDevice} from '../../reducers/connections';
 
 export function initSelfInpector(
   store: Store,
@@ -69,11 +70,7 @@ export function initSelfInpector(
       if (payload.kind == 'ERROR' || payload.kind == 'CLOSED') {
         console.debug(`Device disconnected ${client.id}`, 'server');
         flipperServer.removeConnection(client.id);
-        const toUnregister = new Set<string>();
+        destroyDevice(store, logger, client.id);
-        store.dispatch({
-          type: 'UNREGISTER_DEVICES',
-          payload: toUnregister,
-        });
       }
     },
     onSubscribe(subscription) {