Bug 1867483 - [devtools] Stop async redux/react code after RDM is closed. r=devtools-reviewers,nchevobbe

Unmounting the top level React component will help avoid trigerring React updates
when async thunk actions keep dispatching new actions after the tool is closed.
This was leading to various exceptions in many React components.

Also avoid a race condition where `responsiveFront` could be undefined when closing
RDM while it is still initializing.

Finally, ensure registering the `ResponsiveUI` immediately in `activeTabs` to prevent
race condition in openIfNeeded.

Adding some test coverage for such race condition, but can't enable this on verify.
In chaos mode, there is too many exceptions at the test teardown.
Ideally, RDM should:
* only close the RDP client and let the server restore the default value on its own.
  i.e. we shouldn't do any request on closing.
* RDM close requests should wait for previous initialization request to be done.
  This would avoid zillion of possible exceptions because initialization is very async
  and each step may throw because of things are already being destroyed.
  But this wouldn't address race condition when the tab is closed.
  So RDM should probably try to have a more resilient initialization sequence as well.

Differential Revision: https://phabricator.services.mozilla.com/D197581

devtools/client/responsive/components/App.js

@@ -78,7 +78,6 @@ class App extends PureComponent {
     super(props);
 
     this.onAddCustomDevice = this.onAddCustomDevice.bind(this);
-    this.onBrowserContextMenu = this.onBrowserContextMenu.bind(this);
     this.onChangeDevice = this.onChangeDevice.bind(this);
     this.onChangeNetworkThrottling = this.onChangeNetworkThrottling.bind(this);
     this.onChangePixelRatio = this.onChangePixelRatio.bind(this);
@@ -103,21 +102,10 @@ class App extends PureComponent {
     this.onUpdateDeviceModal = this.onUpdateDeviceModal.bind(this);
   }
 
-  componentWillUnmount() {
-    this.browser.removeEventListener("contextmenu", this.onContextMenu);
-    this.browser = null;
-  }
-
   onAddCustomDevice(device) {
     this.props.dispatch(addCustomDevice(device));
   }
 
-  onBrowserContextMenu() {
-    // Update the position of remote browser so that makes the context menu to show at
-    // proper position before showing.
-    this.browser.frameLoader.requestUpdatePosition();
-  }
-
   onChangeDevice(id, device, deviceType) {
     // Resize the viewport first.
     this.doResizeViewport(id, device.width, device.height);

devtools/client/responsive/index.js

@@ -73,6 +73,9 @@ const bootstrap = {
   destroy() {
     window.removeEventListener("unload", this.destroy, { once: true });
 
+    // unmount to stop async action and renders after destroy
+    ReactDOM.unmountComponentAtNode(this._root);
+
     this.store = null;
 
     this.telemetry.toolClosed("responsive", this);

devtools/client/responsive/manager.js

@@ -124,8 +124,6 @@ class ResponsiveUIManager {
    */
   async openIfNeeded(window, tab, options = {}) {
     if (!this.isActiveForTab(tab)) {
-      await gDevToolsBrowser.loadBrowserStyleSheet(window);
-
       this.initMenuCheckListenerFor(window);
 
       const ui = new ResponsiveUI(this, window, tab);
@@ -134,6 +132,7 @@ class ResponsiveUIManager {
       // Explicitly not await on telemetry to avoid delaying RDM opening
       this.recordTelemetryOpen(window, tab, options);
 
+      await gDevToolsBrowser.loadBrowserStyleSheet(window);
       await this.setMenuCheckFor(tab, window);
       await ui.inited;
       this.emit("on", { tab });

devtools/client/responsive/test/browser/browser.toml

@@ -73,6 +73,9 @@ tags = "devtools webextensions"
 
 ["browser_in_rdm_pane.js"]
 
+["browser_many_toggles.js"]
+skip-if = ["verify"] # Too many exceptions happening on test teardown
+
 ["browser_max_touchpoints.js"]
 
 ["browser_menu_item_01.js"]

devtools/client/responsive/test/browser/browser_many_toggles.js

@@ -0,0 +1,51 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Verify RDM can be toggled many times in a raw
+
+const TEST_URL = "https://example.com/";
+
+addRDMTask(
+  null,
+  async function () {
+    const tab = await addTab(TEST_URL);
+
+    // Record all currently active RDMs, there may not be one created on each loop
+    let opened = 0;
+    ResponsiveUIManager.on("on", () => opened++);
+    ResponsiveUIManager.on("off", () => opened--);
+
+    for (let i = 0; i < 10; i++) {
+      info(`Toggling RDM #${i + 1}`);
+      // This may throw when we were just closing is still ongoing,
+      // ignore any exception.
+      openRDM(tab).catch(e => {});
+      // Sometime pause in order to cover both full synchronous opening and close
+      // but also the same but with some pause between each operation.
+      if (i % 2 == 0) {
+        info("Wait a bit after open");
+        await wait(250);
+      }
+      closeRDM(tab);
+      if (i % 3 == 0) {
+        info("Wait a bit after close");
+        await wait(250);
+      }
+    }
+
+    // Wait a bit so that we can receive the very last ResponsiveUIManager `on` event,
+    // and properly wait for its closing.
+    await wait(1000);
+
+    // This is important to wait for all destruction as closing the tab while RDM
+    // is still closing may lead to exception because of pending cleanup RDP requests.
+    info("Wait for all opened RDM to be closed before closing the tab");
+    await waitFor(() => opened == 0);
+    info("All RDM are closed");
+
+    await removeTab(tab);
+  },
+  { onlyPrefAndTask: true }
+);

devtools/client/responsive/ui.js

@@ -266,7 +266,7 @@ class ResponsiveUI {
     // gracefully, but that shouldn't be a problem since the tab will go away.
     // So, skip any waiting when we're about to close the tab.
     const isTabDestroyed =
-      !this.tab.linkedBrowser || this.responsiveFront.isDestroyed();
+      !this.tab.linkedBrowser || this.responsiveFront?.isDestroyed();
     const isWindowClosing = options?.reason === "unload" || isTabDestroyed;
     const isTabContentDestroying =
       isWindowClosing || options?.reason === "TabClose";