refactor[devtools/extension]: more stable element updates polling to avoid timed out errors (#27357)

Some context:
- When user selects an element in tree inspector, we display current
state of the component. In order to display really current state, we
start polling the backend to get available updates for the element.

Previously:
- Straight-forward sending an event to get element updates each second.
Potential race condition is not handled in any form.
- If user navigates from the page, timeout wouldn't be cleared and we
would potentially throw "Timed out ..." error.
- Bridge disconnection is not handled in any form, if it was shut down,
we could spam with "Timed out ..." errors.

With these changes:
- Requests are now chained, so there can be a single request at a time.
- Handling both navigation and shut down events.

This should reduce the number of "Timed out ..." errors that we see in
our logs for the extension. Other surfaces will also benefit from it,
but not to the full extent, as long as they utilize
"resumeElementPolling" and "pauseElementPolling" events.

Tested this on Chrome, running React DevTools on multiple tabs,
explicitly checked the case when service worker is in idle state and we
return back to the tab.

Author: hoxyq

packages/react-devtools-extensions/src/background/index.js

@@ -83,7 +83,7 @@ chrome.runtime.onConnect.addListener(port => {
   }
 
   if (isNumeric(port.name)) {
-    // Extension port doesn't have tab id specified, because its sender is the extension.
+    // DevTools page port doesn't have tab id specified, because its sender is the extension.
     const tabId = +port.name;
 
     registerTab(tabId);
@@ -231,3 +231,20 @@ chrome.runtime.onMessage.addListener((message, sender) => {
     }
   }
 });
+
+chrome.tabs.onActivated.addListener(({tabId: activeTabId}) => {
+  for (const registeredTabId in ports) {
+    if (
+      ports[registeredTabId].proxy != null &&
+      ports[registeredTabId].extension != null
+    ) {
+      const numericRegisteredTabId = +registeredTabId;
+      const event =
+        activeTabId === numericRegisteredTabId
+          ? 'resumeElementPolling'
+          : 'pauseElementPolling';
+
+      ports[registeredTabId].extension.postMessage({event});
+    }
+  }
+});

packages/react-devtools-shared/src/__tests__/inspectedElement-test.js

@@ -461,10 +461,6 @@ describe('InspectedElement', () => {
     // This test causes an intermediate error to be logged but we can ignore it.
     jest.spyOn(console, 'error').mockImplementation(() => {});
 
-    // Wait for our check-for-updates poll to get the new data.
-    jest.runOnlyPendingTimers();
-    await Promise.resolve();
-
     // Clear the frontend cache to simulate DevTools being closed and re-opened.
     // The backend still thinks the most recently-inspected element is still cached,
     // so the frontend needs to tell it to resend a full value.
@@ -1072,7 +1068,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1227,7 +1222,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1309,7 +1303,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1470,9 +1463,8 @@ describe('InspectedElement', () => {
 
     async function loadPath(path) {
       await TestUtilsAct(async () => {
-        await TestRendererAct(async () => {
+        await TestRendererAct(() => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1597,9 +1589,8 @@ describe('InspectedElement', () => {
 
     async function loadPath(path) {
       await TestUtilsAct(async () => {
-        await TestRendererAct(async () => {
+        await TestRendererAct(() => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1640,9 +1631,11 @@ describe('InspectedElement', () => {
     expect(inspectedElement.props).toMatchInlineSnapshot(`
       {
         "nestedObject": {
-          "a": Dehydrated {
-            "preview_short": {…},
-            "preview_long": {b: {…}, value: 2},
+          "a": {
+            "b": {
+              "value": 2,
+            },
+            "value": 2,
           },
           "value": 2,
         },

packages/react-devtools-shared/src/backendAPI.js

@@ -11,6 +11,7 @@ import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration';
 import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils';
 import Store from 'react-devtools-shared/src/devtools/store';
 import TimeoutError from 'react-devtools-shared/src/errors/TimeoutError';
+import ElementPollingCancellationError from 'react-devtools-shared/src/errors/ElementPollingCancellationError';
 
 import type {
   InspectedElement as InspectedElementBackend,
@@ -138,7 +139,7 @@ export function storeAsGlobal({
   });
 }
 
-const TIMEOUT_DELAY = 5000;
+const TIMEOUT_DELAY = 10_000;
 
 let requestCounter = 0;
 
@@ -151,10 +152,17 @@ function getPromiseForRequestID<T>(
   return new Promise((resolve, reject) => {
     const cleanup = () => {
       bridge.removeListener(eventType, onInspectedElement);
+      bridge.removeListener('shutdown', onDisconnect);
+      bridge.removeListener('pauseElementPolling', onDisconnect);
 
       clearTimeout(timeoutID);
     };
 
+    const onDisconnect = () => {
+      cleanup();
+      reject(new ElementPollingCancellationError());
+    };
+
     const onInspectedElement = (data: any) => {
       if (data.responseID === requestID) {
         cleanup();
@@ -168,6 +176,8 @@ function getPromiseForRequestID<T>(
     };
 
     bridge.addListener(eventType, onInspectedElement);
+    bridge.addListener('shutdown', onDisconnect);
+    bridge.addListener('pauseElementPolling', onDisconnect);
 
     const timeoutID = setTimeout(onTimeout, TIMEOUT_DELAY);
   });

packages/react-devtools-shared/src/bridge.js

@@ -263,6 +263,9 @@ type FrontendEvents = {
   overrideHookState: [OverrideHookState],
   overrideProps: [OverrideValue],
   overrideState: [OverrideValue],
+
+  resumeElementPolling: [],
+  pauseElementPolling: [],
 };
 
 class Bridge<

packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js

@@ -24,8 +24,8 @@ import {
 import {TreeStateContext} from './TreeContext';
 import {BridgeContext, StoreContext} from '../context';
 import {
-  checkForUpdate,
   inspectElement,
+  startElementUpdatesPolling,
 } from 'react-devtools-shared/src/inspectedElementCache';
 import {
   clearHookNamesCache,
@@ -59,8 +59,6 @@ type Context = {
 export const InspectedElementContext: ReactContext<Context> =
   createContext<Context>(((null: any): Context));
 
-const POLL_INTERVAL = 1000;
-
 export type Props = {
   children: ReactNodeList,
 };
@@ -228,14 +226,21 @@ export function InspectedElementContextController({
   // Periodically poll the selected element for updates.
   useEffect(() => {
     if (element !== null && bridgeIsAlive) {
-      const checkForUpdateWrapper = () => {
-        checkForUpdate({bridge, element, refresh, store});
-        timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
-      };
-      let timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
+      const {abort, pause, resume} = startElementUpdatesPolling({
+        bridge,
+        element,
+        refresh,
+        store,
+      });
+
+      bridge.addListener('resumeElementPolling', resume);
+      bridge.addListener('pauseElementPolling', pause);
 
       return () => {
-        clearTimeout(timeoutID);
+        bridge.removeListener('resumeElementPolling', resume);
+        bridge.removeListener('pauseElementPolling', pause);
+
+        abort();
       };
     }
   }, [

packages/react-devtools-shared/src/errors/ElementPollingCancellationError.js

@@ -0,0 +1,21 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+export default class ElementPollingCancellationError extends Error {
+  constructor() {
+    super();
+
+    // Maintains proper stack trace for where our error was thrown (only available on V8)
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, ElementPollingCancellationError);
+    }
+
+    this.name = 'ElementPollingCancellationError';
+  }
+}

packages/react-devtools-shared/src/inspectedElementCache.js

@@ -11,8 +11,9 @@ import {
   unstable_getCacheForType as getCacheForType,
   startTransition,
 } from 'react';
-import Store from './devtools/store';
-import {inspectElement as inspectElementMutableSource} from './inspectedElementMutableSource';
+import Store from 'react-devtools-shared/src/devtools/store';
+import {inspectElement as inspectElementMutableSource} from 'react-devtools-shared/src/inspectedElementMutableSource';
+import ElementPollingCancellationError from 'react-devtools-shared/src//errors/ElementPollingCancellationError';
 
 import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
 import type {Wakeable} from 'shared/ReactTypes';
@@ -177,15 +178,15 @@ export function checkForUpdate({
   element: Element,
   refresh: RefreshFunction,
   store: Store,
-}): void {
+}): void | Promise<void> {
   const {id} = element;
   const rendererID = store.getRendererIDForElement(id);
 
   if (rendererID == null) {
     return;
   }
 
-  inspectElementMutableSource({
+  return inspectElementMutableSource({
     bridge,
     element,
     path: null,
@@ -202,15 +203,88 @@ export function checkForUpdate({
         });
       }
     },
-
-    // There isn't much to do about errors in this case,
-    // but we should at least log them so they aren't silent.
-    error => {
-      console.error(error);
-    },
   );
 }
 
+function createPromiseWhichResolvesInOneSecond() {
+  return new Promise(resolve => setTimeout(resolve, 1000));
+}
+
+type PollingStatus = 'idle' | 'running' | 'paused' | 'aborted';
+
+export function startElementUpdatesPolling({
+  bridge,
+  element,
+  refresh,
+  store,
+}: {
+  bridge: FrontendBridge,
+  element: Element,
+  refresh: RefreshFunction,
+  store: Store,
+}): {abort: () => void, pause: () => void, resume: () => void} {
+  let status: PollingStatus = 'idle';
+
+  function abort() {
+    status = 'aborted';
+  }
+
+  function resume() {
+    if (status === 'running' || status === 'aborted') {
+      return;
+    }
+
+    status = 'idle';
+    poll();
+  }
+
+  function pause() {
+    if (status === 'paused' || status === 'aborted') {
+      return;
+    }
+
+    status = 'paused';
+  }
+
+  function poll(): Promise<void> {
+    status = 'running';
+
+    return Promise.allSettled([
+      checkForUpdate({bridge, element, refresh, store}),
+      createPromiseWhichResolvesInOneSecond(),
+    ])
+      .then(([{status: updateStatus, reason}]) => {
+        // There isn't much to do about errors in this case,
+        // but we should at least log them, so they aren't silent.
+        // Log only if polling is still active, we can't handle the case when
+        // request was sent, and then bridge was remounted (for example, when user did navigate to a new page),
+        // but at least we can mark that polling was aborted
+        if (updateStatus === 'rejected' && status !== 'aborted') {
+          // This is expected Promise rejection, no need to log it
+          if (reason instanceof ElementPollingCancellationError) {
+            return;
+          }
+
+          console.error(reason);
+        }
+      })
+      .finally(() => {
+        const shouldContinuePolling =
+          status !== 'aborted' && status !== 'paused';
+
+        status = 'idle';
+
+        if (shouldContinuePolling) {
+          return poll();
+        }
+      });
+  }
+
+  poll();
+
+  return {abort, resume, pause};
+}
+
 export function clearCacheBecauseOfError(refresh: RefreshFunction): void {
   startTransition(() => {
     const map = createMap();