Bug 1515612 - Disconnect all the invalid clients when updating remote clients;r=ladybenko
☠☠ backed out by 5d5c62405e28 ☠ ☠
authorJulian Descottes <jdescottes@mozilla.com>
Wed, 09 Jan 2019 17:14:50 +0000
changeset 510218 552427e973d5556a4666f86974f4793af42c97d8
parent 510217 97b28c1b8a8595a7e47dc671432ef3d7b3ea8770
child 510219 5345e8472f29cd844c8218b50051c01f8aa12aee
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersladybenko
bugs1515612
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1515612 - Disconnect all the invalid clients when updating remote clients;r=ladybenko Depends on D15415 Differential Revision: https://phabricator.services.mozilla.com/D15416
devtools/client/aboutdebugging-new/src/actions/runtimes.js
devtools/client/aboutdebugging-new/src/components/App.js
devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
devtools/client/aboutdebugging-new/test/browser/mocks/helper-client-wrapper-mock.js
--- a/devtools/client/aboutdebugging-new/src/actions/runtimes.js
+++ b/devtools/client/aboutdebugging-new/src/actions/runtimes.js
@@ -21,16 +21,17 @@ const { remoteClientManager } =
 const {
   CONNECT_RUNTIME_FAILURE,
   CONNECT_RUNTIME_START,
   CONNECT_RUNTIME_SUCCESS,
   DEBUG_TARGETS,
   DISCONNECT_RUNTIME_FAILURE,
   DISCONNECT_RUNTIME_START,
   DISCONNECT_RUNTIME_SUCCESS,
+  PAGE_TYPES,
   REMOTE_RUNTIMES_UPDATED,
   RUNTIME_PREFERENCE,
   RUNTIMES,
   UNWATCH_RUNTIME_FAILURE,
   UNWATCH_RUNTIME_START,
   UNWATCH_RUNTIME_SUCCESS,
   UPDATE_CONNECTION_PROMPT_SETTING_FAILURE,
   UPDATE_CONNECTION_PROMPT_SETTING_START,
@@ -264,63 +265,77 @@ function updateUSBRuntimes(adbRuntimes) 
       isUnknown: adbRuntime.isUnknown(),
       name: adbRuntime.shortName,
       type: RUNTIMES.USB,
     };
   });
   return updateRemoteRuntimes(runtimes, RUNTIMES.USB);
 }
 
+/**
+ * Check that a given runtime can still be found in the provided array of runtimes, and
+ * that the connection of the associated DebuggerClient is still valid.
+ * Note that this check is only valid for runtimes which match the type of the runtimes
+ * in the array.
+ */
+function _isRuntimeValid(runtime, runtimes) {
+  const isRuntimeAvailable = runtimes.some(r => r.id === runtime.id);
+  const isConnectionValid = runtime.runtimeDetails &&
+    !runtime.runtimeDetails.clientWrapper.isClosed();
+  return isRuntimeAvailable && isConnectionValid;
+}
+
 function updateRemoteRuntimes(runtimes, type) {
   return async (dispatch, getState) => {
     const currentRuntime = getCurrentRuntime(getState().runtimes);
 
-    if (currentRuntime &&
-        currentRuntime.type === type &&
-        !runtimes.find(runtime => currentRuntime.id === runtime.id)) {
-      // Since current USB runtime was invalid, move to this firefox page.
+    // Check if the updated remote runtimes should trigger a navigation out of the current
+    // runtime page.
+    if (currentRuntime && currentRuntime.type === type &&
+      !_isRuntimeValid(currentRuntime, runtimes)) {
+      // Since current remote runtime is invalid, move to this firefox page.
       // This case is considered as followings and so on:
       // * Remove ADB addon
       // * (Physically) Disconnect USB runtime
       //
-      // The reason why we call selectPage before USB_RUNTIMES_UPDATED was fired is below.
-      // Current runtime can not be retrieved after USB_RUNTIMES_UPDATED action, since
-      // that updates runtime state. So, before that we fire selectPage action so that to
-      // transact unwatchRuntime correctly.
-
-      await dispatch(Actions.selectPage(RUNTIMES.THIS_FIREFOX, RUNTIMES.THIS_FIREFOX));
+      // The reason we call selectPage before REMOTE_RUNTIMES_UPDATED is fired is below.
+      // Current runtime can not be retrieved after REMOTE_RUNTIMES_UPDATED action, since
+      // that updates runtime state. So, before that we fire selectPage action to execute
+      // `unwatchRuntime` correctly.
+      await dispatch(Actions.selectPage(PAGE_TYPES.RUNTIME, RUNTIMES.THIS_FIREFOX));
     }
 
     // Retrieve runtimeDetails from existing runtimes.
     runtimes.forEach(runtime => {
       const existingRuntime = findRuntimeById(runtime.id, getState().runtimes);
-      runtime.runtimeDetails = existingRuntime ? existingRuntime.runtimeDetails : null;
+      const isConnectionValid = existingRuntime && existingRuntime.runtimeDetails &&
+        !existingRuntime.runtimeDetails.clientWrapper.isClosed();
+      runtime.runtimeDetails = isConnectionValid ? existingRuntime.runtimeDetails : null;
     });
 
-    // Disconnect runtimes that were no longer valid
-    const validIds = runtimes.map(r => r.id);
     const existingRuntimes = getAllRuntimes(getState().runtimes);
-    const invalidRuntimes = existingRuntimes.filter(r => {
-      return r.type === type && !validIds.includes(r.id);
-    });
-
-    for (const invalidRuntime of invalidRuntimes) {
-      const isConnected = !!invalidRuntime.runtimeDetails;
-      if (isConnected) {
-        await dispatch(disconnectRuntime(invalidRuntime.id));
+    for (const runtime of existingRuntimes) {
+      // Runtime was connected before.
+      const isConnected = runtime.runtimeDetails;
+      // Runtime is of the same type as the updated runtimes array, so we should check it.
+      const isSameType = runtime.type === type;
+      if (isConnected && isSameType && !_isRuntimeValid(runtime, runtimes)) {
+        // Disconnect runtimes that were no longer valid.
+        await dispatch(disconnectRuntime(runtime.id));
       }
     }
 
     dispatch({ type: REMOTE_RUNTIMES_UPDATED, runtimes, runtimeType: type });
 
     for (const runtime of getAllRuntimes(getState().runtimes)) {
       if (runtime.type !== type) {
         continue;
       }
 
+      // Reconnect clients already available in the RemoteClientManager.
       const isConnected = !!runtime.runtimeDetails;
       const hasConnectedClient = remoteClientManager.hasClient(runtime.id, runtime.type);
       if (!isConnected && hasConnectedClient) {
         await dispatch(connectRuntime(runtime.id));
       }
     }
   };
 }
--- a/devtools/client/aboutdebugging-new/src/components/App.js
+++ b/devtools/client/aboutdebugging-new/src/components/App.js
@@ -66,30 +66,31 @@ class App extends PureComponent {
   // We are using it to read the route path.
   // See react-router docs:
   // https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/api/match.md
   renderRuntime({ match }) {
     // Redirect to This Firefox in these cases:
     // - If the runtimepage for a device is the first page shown (since we can't
     //   keep connections open between page reloads).
     // - If no runtimeId is given.
-    // - If runtime is not found in the runtimes list (this is handled later)
+    // - If runtime is not in the runtimes list or disconnected (this is handled later)
     const isDeviceFirstPage =
       !this.props.selectedPage &&
       match.params.runtimeId !== RUNTIMES.THIS_FIREFOX;
     if (!match.params.runtimeId || isDeviceFirstPage) {
       return Redirect({ to: `/runtime/${RUNTIMES.THIS_FIREFOX}` });
     }
 
     const isRuntimeAvailable = id => {
       const runtimes = [
         ...this.props.networkRuntimes,
         ...this.props.usbRuntimes,
       ];
-      return !!runtimes.find(x => x.id === id);
+      const runtime = runtimes.find(x => x.id === id);
+      return runtime && runtime.runtimeDetails;
     };
 
     const { dispatch } = this.props;
 
     let runtimeId = match.params.runtimeId || RUNTIMES.THIS_FIREFOX;
     if (match.params.runtimeId !== RUNTIMES.THIS_FIREFOX) {
       const rawId = decodeURIComponent(match.params.runtimeId);
       if (isRuntimeAvailable(rawId)) {
--- a/devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
+++ b/devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
@@ -133,11 +133,15 @@ class ClientWrapper {
 
   async request(options) {
     return this.client.request(options);
   }
 
   async close() {
     return this.client.close();
   }
+
+  isClosed() {
+    return this.client._closed;
+  }
 }
 
 exports.ClientWrapper = ClientWrapper;
--- a/devtools/client/aboutdebugging-new/test/browser/mocks/helper-client-wrapper-mock.js
+++ b/devtools/client/aboutdebugging-new/test/browser/mocks/helper-client-wrapper-mock.js
@@ -43,19 +43,20 @@ function createClientMock() {
       },
       addListener: (evt, listener) => {
         eventEmitter.on(evt, listener);
       },
       removeListener: (evt, listener) => {
         eventEmitter.off(evt, listener);
       },
     },
-
     // no-op
     close: () => {},
+    // client is not closed
+    isClosed: () => false,
     // no-op
     connect: () => {},
     // no-op
     getDeviceDescription: () => {},
     // Return default preference value or null if no match.
     getPreference: function(prefName) {
       if (prefName in this._preferences) {
         return this._preferences[prefName];