From c43049d881aa8045aa934c2feb6339ec2864ff9e Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 9 Feb 2021 04:12:09 -0800 Subject: [PATCH] Preserve device state after disconnect Summary: This diff stack introduces support for keeping devices and clients around after they have disconnected. This is a pretty important debugging improvement, that will allow inspecting a device / app after it crashed for example. This feature existed partially before, but only supported Android, and only support plugins with persisted state; as it replace the current device with an archived version of the same device. In practice this didn't work really well, as most plugins would not be available, and all non-persisted state would be lost. This diff makes sure we can keep devices around after disconnecting, the next one will keep the clients around as well. And explain some code choices in more detail. Note that `Device.isArchived` was an overloaded term before, and even more now (both representing imported and disconnected devices), will address that in a later diff. https://github.com/facebook/flipper/issues/1460 https://github.com/facebook/flipper/issues/812 https://github.com/facebook/flipper/issues/1487 Changelog: iOS and Android devices will preserve their state after being disconnected Reviewed By: nikoant Differential Revision: D26224310 fbshipit-source-id: 7dfc93c2a109a51c2880ec212a00463bc8d32041 --- desktop/app/src/__tests__/disconnect.node.tsx | 108 ++++++++++++++++++ desktop/app/src/devices/AndroidDevice.tsx | 10 -- desktop/app/src/devices/ArchivedDevice.tsx | 2 +- desktop/app/src/devices/BaseDevice.tsx | 26 ++++- desktop/app/src/dispatcher/androidDevice.tsx | 27 +---- desktop/app/src/dispatcher/iOSDevice.tsx | 15 +-- desktop/app/src/dispatcher/metroDevice.tsx | 16 --- desktop/app/src/reducers/connections.tsx | 1 + .../sandy-chrome/appinspect/AppInspect.tsx | 4 +- .../sandy-chrome/appinspect/AppSelector.tsx | 3 +- .../src/utils/__tests__/exportData.node.tsx | 24 ++-- .../flipper-plugin/src/plugin/PluginBase.tsx | 4 +- desktop/flipper-plugin/src/state/atom.tsx | 11 +- 13 files changed, 169 insertions(+), 82 deletions(-) create mode 100644 desktop/app/src/__tests__/disconnect.node.tsx diff --git a/desktop/app/src/__tests__/disconnect.node.tsx b/desktop/app/src/__tests__/disconnect.node.tsx new file mode 100644 index 000000000..b3587beab --- /dev/null +++ b/desktop/app/src/__tests__/disconnect.node.tsx @@ -0,0 +1,108 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {default as BaseDevice} from '../devices/BaseDevice'; +import {createMockFlipperWithPlugin} from '../test-utils/createMockFlipperWithPlugin'; +import { + TestUtils, + _SandyPluginDefinition, + createState, + DevicePluginClient, +} from 'flipper-plugin'; + +test('Devices can disconnect', async () => { + const deviceplugin = new _SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + devicePlugin(client: DevicePluginClient) { + const destroy = jest.fn(); + client.onDestroy(destroy); + const counter = createState(0); + return { + counter, + destroy, + }; + }, + supportsDevice() { + return true; + }, + Component() { + return null; + }, + }, + ); + const {device} = await createMockFlipperWithPlugin(deviceplugin); + + device.sandyPluginStates.get(deviceplugin.id)!.instanceApi.counter.set(1); + + expect(device.isArchived).toBe(false); + + device.markDisconnected(); + + expect(device.isArchived).toBe(true); + const instance = device.sandyPluginStates.get(deviceplugin.id)!; + expect(instance).toBeTruthy(); + expect(instance.instanceApi.counter.get(1)).toBe(1); // state preserved + expect(instance.instanceApi.destroy).toBeCalledTimes(0); + + device.destroy(); + expect(device.isArchived).toBe(true); + expect(instance.instanceApi.destroy).toBeCalledTimes(1); + + expect(device.sandyPluginStates.get(deviceplugin.id)).toBeUndefined(); +}); + +test('New device with same serial removes & cleans the old one', async () => { + const deviceplugin = new _SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + devicePlugin(client: DevicePluginClient) { + const destroy = jest.fn(); + client.onDestroy(destroy); + return { + destroy, + }; + }, + supportsDevice() { + return true; + }, + Component() { + return null; + }, + }, + ); + const {device, store} = await createMockFlipperWithPlugin(deviceplugin); + + const instance = device.sandyPluginStates.get(deviceplugin.id)!; + + expect(device.isArchived).toBe(false); + expect(instance.instanceApi.destroy).toBeCalledTimes(0); + expect(store.getState().connections.devices).toEqual([device]); + + // submit a new device with same serial + const device2 = new BaseDevice( + device.serial, + 'physical', + 'MockAndroidDevice', + 'Android', + ); + store.dispatch({ + type: 'REGISTER_DEVICE', + payload: device2, + }); + device2.loadDevicePlugins(store.getState().plugins.devicePlugins); + + expect(device.isArchived).toBe(true); + expect(instance.instanceApi.destroy).toBeCalledTimes(1); + expect( + device2.sandyPluginStates.get(deviceplugin.id)!.instanceApi.destroy, + ).toBeCalledTimes(0); + expect(store.getState().connections.devices.length).toBe(1); + expect(store.getState().connections.devices[0]).toBe(device2); +}); diff --git a/desktop/app/src/devices/AndroidDevice.tsx b/desktop/app/src/devices/AndroidDevice.tsx index 8be72ddf3..227eb913d 100644 --- a/desktop/app/src/devices/AndroidDevice.tsx +++ b/desktop/app/src/devices/AndroidDevice.tsx @@ -89,16 +89,6 @@ export default class AndroidDevice extends BaseDevice { return this.executeShell(['logcat', '-c']); } - archive(): ArchivedDevice { - return new ArchivedDevice({ - serial: this.serial, - deviceType: this.deviceType, - title: this.title, - os: this.os, - screenshotHandle: null, - }); - } - navigateToLocation(location: string) { const shellCommand = `am start ${encodeURI(location)}`; this.adb.shell(this.serial, shellCommand); diff --git a/desktop/app/src/devices/ArchivedDevice.tsx b/desktop/app/src/devices/ArchivedDevice.tsx index 82b8b5a06..ef1e13fba 100644 --- a/desktop/app/src/devices/ArchivedDevice.tsx +++ b/desktop/app/src/devices/ArchivedDevice.tsx @@ -23,13 +23,13 @@ export default class ArchivedDevice extends BaseDevice { supportRequestDetails?: SupportFormRequestDetailsState; }) { super(options.serial, options.deviceType, options.title, options.os); + this.archivedState.set(true); this.source = options.source || ''; this.supportRequestDetails = options.supportRequestDetails; this.archivedScreenshotHandle = options.screenshotHandle; } archivedScreenshotHandle: string | null; - isArchived = true; displayTitle(): string { return `${this.title} ${this.source ? '(Imported)' : '(Offline)'}`; diff --git a/desktop/app/src/devices/BaseDevice.tsx b/desktop/app/src/devices/BaseDevice.tsx index 0d14ddfda..4695c462f 100644 --- a/desktop/app/src/devices/BaseDevice.tsx +++ b/desktop/app/src/devices/BaseDevice.tsx @@ -15,6 +15,7 @@ import { DeviceType, DeviceLogListener, Idler, + createState, } from 'flipper-plugin'; import type {DevicePluginDefinition, DevicePluginMap} from '../plugin'; import {getFlipperLibImplementation} from '../utils/flipperLibImplementation'; @@ -70,7 +71,12 @@ export default class BaseDevice { icon: string | null | undefined; logListeners: Map = new Map(); - isArchived: boolean = false; + + archivedState = createState(false); + get isArchived() { + return this.archivedState.get(); + } + // if imported, stores the original source location source = ''; @@ -87,7 +93,7 @@ export default class BaseDevice { } displayTitle(): string { - return this.title; + return this.isArchived ? `${this.title} (Offline)` : this.title; } async exportState( @@ -154,10 +160,6 @@ export default class BaseDevice { throw new Error('unimplemented'); } - archive(): any | null | undefined { - return null; - } - screenshot(): Promise { return Promise.reject( new Error('No screenshot support for current device'), @@ -218,4 +220,16 @@ export default class BaseDevice { } this.devicePlugins.splice(this.devicePlugins.indexOf(pluginId), 1); } + + markDisconnected() { + this.archivedState.set(true); + } + + destroy() { + this.markDisconnected(); + this.sandyPluginStates.forEach((instance) => { + instance.destroy(); + }); + this.sandyPluginStates.clear(); + } } diff --git a/desktop/app/src/dispatcher/androidDevice.tsx b/desktop/app/src/dispatcher/androidDevice.tsx index d61a2ff3e..834b0f4d9 100644 --- a/desktop/app/src/dispatcher/androidDevice.tsx +++ b/desktop/app/src/dispatcher/androidDevice.tsx @@ -262,28 +262,11 @@ export default (store: Store, logger: Logger) => { }), ); - const archivedDevices = deviceIds - .map((id) => { - const device = store - .getState() - .connections.devices.find((device) => device.serial === id); - if (device && !device.isArchived) { - return device.archive(); - } - }) - .filter(Boolean); - - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: new Set(deviceIds), - }); - - archivedDevices.forEach((device: BaseDevice) => { - device.loadDevicePlugins(store.getState().plugins.devicePlugins); - store.dispatch({ - type: 'REGISTER_DEVICE', - payload: device, - }); + deviceIds.forEach((id) => { + const device = store + .getState() + .connections.devices.find((device) => device.serial === id); + device?.markDisconnected(); }); } diff --git a/desktop/app/src/dispatcher/iOSDevice.tsx b/desktop/app/src/dispatcher/iOSDevice.tsx index 1dc35a27e..972d337ac 100644 --- a/desktop/app/src/dispatcher/iOSDevice.tsx +++ b/desktop/app/src/dispatcher/iOSDevice.tsx @@ -148,15 +148,12 @@ function processDevices( } } - if (currentDeviceIDs.size > 0) { - currentDeviceIDs.forEach((id) => - logger.track('usage', 'unregister-device', {os: 'iOS', serial: id}), - ); - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: currentDeviceIDs, - }); - } + currentDeviceIDs.forEach((id) => { + const device = store + .getState() + .connections.devices.find((device) => device.serial === id); + device?.markDisconnected(); + }); } function getDeviceSetPath() { diff --git a/desktop/app/src/dispatcher/metroDevice.tsx b/desktop/app/src/dispatcher/metroDevice.tsx index b19ce9cdd..c77d5644a 100644 --- a/desktop/app/src/dispatcher/metroDevice.tsx +++ b/desktop/app/src/dispatcher/metroDevice.tsx @@ -79,26 +79,10 @@ async function unregisterDevices(store: Store, logger: Logger) { serial: METRO_URL, }); - let archivedDevice: ArchivedDevice | undefined = undefined; - const device = store - .getState() - .connections.devices.find((device) => device.serial === METRO_URL); - if (device && !device.isArchived) { - archivedDevice = device.archive(); - } - store.dispatch({ type: 'UNREGISTER_DEVICES', payload: new Set([METRO_URL]), }); - - if (archivedDevice) { - archivedDevice.loadDevicePlugins(store.getState().plugins.devicePlugins); - store.dispatch({ - type: 'REGISTER_DEVICE', - payload: archivedDevice, - }); - } } export default (store: Store, logger: Logger) => { diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 626654f3a..88f180d5f 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -197,6 +197,7 @@ export default (state: State = INITAL_STATE, action: Actions): State => { console.debug( `Got a new device instance for already existing serial ${payload.serial}`, ); + state.devices[existing].destroy(); newDevices[existing] = payload; } else { newDevices.push(payload); diff --git a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx index 268d893f5..e46cc2903 100644 --- a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx +++ b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx @@ -11,7 +11,7 @@ import React from 'react'; import {Alert} from 'antd'; import {LeftSidebar, SidebarTitle, InfoIcon} from '../LeftSidebar'; import {Layout, Link, styled} from '../../ui'; -import {theme} from 'flipper-plugin'; +import {theme, useValue} from 'flipper-plugin'; import {AppSelector} from './AppSelector'; import {useStore} from '../../utils/useStore'; import {PluginList} from './PluginList'; @@ -52,7 +52,7 @@ export function AppInspect() { metroDevice, connections.userPreferredDevice, ]); - const isArchived = !!activeDevice?.isArchived; + const isArchived = useValue(activeDevice?.archivedState, false); return ( diff --git a/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx b/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx index a2dedc5c9..ff5085992 100644 --- a/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx +++ b/desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx @@ -17,7 +17,7 @@ import { RocketOutlined, } from '@ant-design/icons'; import {Glyph, Layout, styled} from '../../ui'; -import {theme, useTrackedCallback} from 'flipper-plugin'; +import {theme, useTrackedCallback, useValue} from 'flipper-plugin'; import {batch} from 'react-redux'; import {useDispatch, useStore} from '../../utils/useStore'; import { @@ -56,6 +56,7 @@ export function AppSelector() { uninitializedClients, selectedApp, } = useStore((state) => state.connections); + useValue(selectedDevice?.archivedState, false); // subscribe to future archived state changes const onSelectDevice = useTrackedCallback( 'select-device', diff --git a/desktop/app/src/utils/__tests__/exportData.node.tsx b/desktop/app/src/utils/__tests__/exportData.node.tsx index bc0c3d793..9bf0c6ed2 100644 --- a/desktop/app/src/utils/__tests__/exportData.node.tsx +++ b/desktop/app/src/utils/__tests__/exportData.node.tsx @@ -21,7 +21,7 @@ import {FlipperPlugin, FlipperDevicePlugin} from '../../plugin'; import {Notification} from '../../plugin'; import {default as Client, ClientExport} from '../../Client'; import {State as PluginsState} from '../../reducers/plugins'; -import {renderMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin'; +import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin'; import { TestUtils, _SandyPluginDefinition, @@ -1116,7 +1116,7 @@ const sandyTestPlugin = new _SandyPluginDefinition( ); test('Sandy plugins are exported properly', async () => { - const {client, sendMessage, store} = await renderMockFlipperWithPlugin( + const {client, sendMessage, store} = await createMockFlipperWithPlugin( sandyTestPlugin, ); @@ -1145,7 +1145,7 @@ test('Sandy plugins are exported properly', async () => { }); test('Sandy plugins with custom export are exported properly', async () => { - const {client, sendMessage, store} = await renderMockFlipperWithPlugin( + const {client, sendMessage, store} = await createMockFlipperWithPlugin( sandyTestPlugin, ); @@ -1218,7 +1218,7 @@ test('Sandy plugins are imported properly', async () => { }, }; - const {client, store} = await renderMockFlipperWithPlugin(sandyTestPlugin); + const {client, store} = await createMockFlipperWithPlugin(sandyTestPlugin); await importDataToStore('unittest.json', JSON.stringify(data), store); @@ -1312,7 +1312,7 @@ test('Sandy device plugins are exported / imported properly', async () => { }, }; - const {device, store} = await renderMockFlipperWithPlugin( + const {device, store} = await createMockFlipperWithPlugin( sandyDeviceTestPlugin, ); @@ -1350,7 +1350,7 @@ test('Sandy device plugins are exported / imported properly', async () => { }); test('Sandy device plugins with custom export are export properly', async () => { - const {device, store} = await renderMockFlipperWithPlugin( + const {device, store} = await createMockFlipperWithPlugin( sandyDeviceTestPlugin, ); @@ -1384,7 +1384,7 @@ test('Sandy plugin with custom import', async () => { }, ); - const {store} = await renderMockFlipperWithPlugin(plugin); + const {store} = await createMockFlipperWithPlugin(plugin); const data = { clients: [ @@ -1484,7 +1484,7 @@ test('Sandy device plugin with custom import', async () => { }, }; - const {store} = await renderMockFlipperWithPlugin(plugin); + const {store} = await createMockFlipperWithPlugin(plugin); await importDataToStore('unittest.json', JSON.stringify(data), store); @@ -1503,7 +1503,7 @@ test('Sandy device plugin with custom import', async () => { }); test('Sandy plugins with complex data are imported / exported correctly', async () => { - const deviceplugin = new _SandyPluginDefinition( + const plugin = new _SandyPluginDefinition( TestUtils.createMockPluginDetails(), { plugin() { @@ -1522,7 +1522,7 @@ test('Sandy plugins with complex data are imported / exported correctly', async }, ); - const {store} = await renderMockFlipperWithPlugin(deviceplugin); + const {store} = await createMockFlipperWithPlugin(plugin); const data = await exportStore(store); expect(Object.values(data.exportStoreData.pluginStates2)).toMatchObject([ @@ -1551,7 +1551,7 @@ test('Sandy plugins with complex data are imported / exported correctly', async await importDataToStore('unittest.json', data.serializedString, store); const api = store .getState() - .connections.clients[1].sandyPluginStates.get(deviceplugin.id)?.instanceApi; + .connections.clients[1].sandyPluginStates.get(plugin.id)?.instanceApi; expect(api.m.get()).toMatchInlineSnapshot(` Map { "a" => 1, @@ -1590,7 +1590,7 @@ test('Sandy device plugins with complex data are imported / exported correctly' }, ); - const {store} = await renderMockFlipperWithPlugin(deviceplugin); + const {store} = await createMockFlipperWithPlugin(deviceplugin); const data = await exportStore(store); expect(data.exportStoreData.device?.pluginStates).toMatchObject({ diff --git a/desktop/flipper-plugin/src/plugin/PluginBase.tsx b/desktop/flipper-plugin/src/plugin/PluginBase.tsx index e18741ff6..60bc4494c 100644 --- a/desktop/flipper-plugin/src/plugin/PluginBase.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginBase.tsx @@ -132,7 +132,9 @@ export abstract class BasePluginInstance { realDevice, // TODO: temporarily, clean up T70688226 // N.B. we model OS as string, not as enum, to make custom device types possible in the future os: realDevice.os, - isArchived: realDevice.isArchived, + get isArchived() { + return realDevice.isArchived; + }, deviceType: realDevice.deviceType, onLogEntry(cb) { diff --git a/desktop/flipper-plugin/src/state/atom.tsx b/desktop/flipper-plugin/src/state/atom.tsx index 7c6d86525..f66b17cfc 100644 --- a/desktop/flipper-plugin/src/state/atom.tsx +++ b/desktop/flipper-plugin/src/state/atom.tsx @@ -84,9 +84,16 @@ export function createState( return atom; } -export function useValue(atom: Atom): T { - const [localValue, setLocalValue] = useState(atom.get()); +export function useValue(atom: Atom): T; +export function useValue(atom: Atom | undefined, defaultValue: T): T; +export function useValue(atom: Atom | undefined, defaultValue?: T): T { + const [localValue, setLocalValue] = useState( + atom ? atom.get() : defaultValue!, + ); useEffect(() => { + if (!atom) { + return; + } // atom might have changed between mounting and effect setup // in that case, this will cause a re-render, otherwise not setLocalValue(atom.get());