Bug 1485676 - Tweak RDM manage to support new async forTab. r=yulia

Summary:
Fetching any target is now asynchronous. But RDM setup/destroy codepath is very fragile
and introduce many low level exception when trying to restore the original browser element
if any timing changes.
So this patch prevents trying to fetch the target object if a toolbox isn't already opened.
The target object is being used only for Telemetry purpose for now.

Depends On D4538

Reviewers: yulia!

Tags: #secure-revision

Bug #: 1485676

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

MozReview-Commit-ID: 2QDUNqentMP

devtools/client/responsive.html/manager.js

@@ -87,36 +87,18 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
    */
   async openIfNeeded(window, tab, options = {}) {
     if (!tab.linkedBrowser.isRemoteBrowser) {
-      this.showRemoteOnlyNotification(window, tab, options);
+      await this.showRemoteOnlyNotification(window, tab, options);
       return promise.reject(new Error("RDM only available for remote tabs."));
     }
     if (!this.isActiveForTab(tab)) {
       this.initMenuCheckListenerFor(window);
 
-      // Track whether a toolbox was opened before RDM was opened.
-      const toolbox = gDevTools.getToolbox(TargetFactory.forTab(tab));
-      const hostType = toolbox ? toolbox.hostType : "none";
-      const hasToolbox = !!toolbox;
-      const tel = this._telemetry;
-      if (hasToolbox) {
-        tel.scalarAdd("devtools.responsive.toolbox_opened_first", 1);
-      }
-
-      tel.recordEvent("devtools.main", "activate", "responsive_design", null, {
-        "host": hostType,
-        "width": Math.ceil(window.outerWidth / 50) * 50,
-        "session_id": toolbox ? toolbox.sessionId : -1
-      });
-
-      // Track opens keyed by the UI entry point used.
-      let { trigger } = options;
-      if (!trigger) {
-        trigger = "unknown";
-      }
-      tel.keyedScalarAdd("devtools.responsive.open_trigger", trigger, 1);
-
       const ui = new ResponsiveUI(window, tab);
       this.activeTabs.set(tab, ui);
+
+      // Explicitly not await on telemetry to avoid delaying RDM opening
+      this.recordTelemetryOpen(window, tab, options);
+
       await this.setMenuCheckFor(tab, window);
       await ui.inited;
       this.emit("on", { tab });
@@ -125,6 +107,38 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
     return this.getResponsiveUIForTab(tab);
   },
 
+  /**
+   * Record all telemetry probes related to RDM opening.
+   */
+  async recordTelemetryOpen(window, tab, options) {
+    // Track whether a toolbox was opened before RDM was opened.
+    const isKnownTab = TargetFactory.isKnownTab(tab);
+    let toolbox;
+    if (isKnownTab) {
+      const target = await TargetFactory.forTab(tab);
+      toolbox = gDevTools.getToolbox(target);
+    }
+    const hostType = toolbox ? toolbox.hostType : "none";
+    const hasToolbox = !!toolbox;
+    const tel = this._telemetry;
+    if (hasToolbox) {
+      tel.scalarAdd("devtools.responsive.toolbox_opened_first", 1);
+    }
+
+    tel.recordEvent("devtools.main", "activate", "responsive_design", null, {
+      "host": hostType,
+      "width": Math.ceil(window.outerWidth / 50) * 50,
+      "session_id": toolbox ? toolbox.sessionId : -1
+    });
+
+    // Track opens keyed by the UI entry point used.
+    let { trigger } = options;
+    if (!trigger) {
+      trigger = "unknown";
+    }
+    tel.keyedScalarAdd("devtools.responsive.open_trigger", trigger, 1);
+  },
+
   /**
    * Closes the responsive UI, if not already closed.
    *
@@ -144,15 +158,6 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
    */
   async closeIfNeeded(window, tab, options = {}) {
     if (this.isActiveForTab(tab)) {
-      const isKnownTab = TargetFactory.isKnownTab(tab);
-      const target = TargetFactory.forTab(tab);
-      const toolbox = gDevTools.getToolbox(target);
-
-      if (!toolbox && !isKnownTab) {
-        // Destroy the tabTarget to avoid a memory leak.
-        target.destroy();
-      }
-
       const ui = this.activeTabs.get(tab);
       const destroyed = await ui.destroy(options);
       if (!destroyed) {
@@ -160,14 +165,6 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
         return;
       }
 
-      const hostType = toolbox ? toolbox.hostType : "none";
-      const t = this._telemetry;
-      t.recordEvent("devtools.main", "deactivate", "responsive_design", null, {
-        "host": hostType,
-        "width": Math.ceil(window.outerWidth / 50) * 50,
-        "session_id": toolbox ? toolbox.sessionId : -1
-      });
-
       this.activeTabs.delete(tab);
 
       if (!this.isActiveForWindow(window)) {
@@ -175,9 +172,29 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
       }
       this.emit("off", { tab });
       await this.setMenuCheckFor(tab, window);
+
+      // Explicitly not await on telemetry to avoid delaying RDM closing
+      this.recordTelemetryClose(window, tab);
     }
   },
 
+  async recordTelemetryClose(window, tab) {
+    const isKnownTab = TargetFactory.isKnownTab(tab);
+    let toolbox;
+    if (isKnownTab) {
+      const target = await TargetFactory.forTab(tab);
+      toolbox = gDevTools.getToolbox(target);
+    }
+
+    const hostType = toolbox ? toolbox.hostType : "none";
+    const t = this._telemetry;
+    t.recordEvent("devtools.main", "deactivate", "responsive_design", null, {
+      "host": hostType,
+      "width": Math.ceil(window.outerWidth / 50) * 50,
+      "session_id": toolbox ? toolbox.sessionId : -1
+    });
+  },
+
   /**
    * Returns true if responsive UI is active for a given tab.
    *
@@ -238,7 +255,7 @@ const ResponsiveUIManager = exports.ResponsiveUIManager = {
   },
 
   showRemoteOnlyNotification(window, tab, { trigger } = {}) {
-    showNotification(window, tab, {
+    return showNotification(window, tab, {
       toolboxButton: trigger == "toolbox",
       msg: l10n.getStr("responsive.remoteOnly"),
       priority: PriorityLevels.PRIORITY_CRITICAL_MEDIUM,
@@ -374,7 +391,8 @@ ResponsiveUI.prototype = {
     // If our tab is about to be closed, there's not enough time to exit
     // 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 isWindowClosing = options && options.reason === "unload";
+    const isTabDestroyed = !this.tab.linkedBrowser;
+    const isWindowClosing = (options && options.reason === "unload") || isTabDestroyed;
     const isTabContentDestroying =
       isWindowClosing || (options && (options.reason === "TabClose" ||
                                       options.reason === "BeforeTabRemotenessChange"));

devtools/client/responsive.html/utils/notification.js

@@ -22,7 +22,7 @@ loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools"
  *        - `priority`: Priority level for the notification, which affects the icon and
  *                      overall appearance.
  */
-function showNotification(window, tab, { toolboxButton, msg, priority } = {}) {
+async function showNotification(window, tab, { toolboxButton, msg, priority } = {}) {
   // Default to using the browser's per-tab notification box
   let nbox = window.gBrowser.getNotificationBox(tab.linkedBrowser);
 
@@ -30,7 +30,7 @@ function showNotification(window, tab, { toolboxButton, msg, priority } = {}) {
   // toolbox for the tab.  If one exists, use the toolbox's notification box so that the
   // message is placed closer to the action taken by the user.
   if (toolboxButton) {
-    const target = TargetFactory.forTab(tab);
+    const target = await TargetFactory.forTab(tab);
     const toolbox = gDevTools.getToolbox(target);
     if (toolbox) {
       nbox = toolbox.notificationBox;