Bug 1842252 - use correct destruction callback and improve handling of hiding the sidebar, r=jhirsch

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

browser/base/content/browser.js

@@ -9970,6 +9970,9 @@ var ShoppingSidebarManager = {
   },
 
   _updateVisibility() {
+    if (window.closed) {
+      return;
+    }
     let optedOut = this.optedInPref === 2;
     let isPBM = PrivateBrowsingUtils.isWindowPrivate(window);
 

browser/components/shopping/ShoppingSidebarChild.sys.mjs

@@ -33,8 +33,9 @@ export class ShoppingSidebarChild extends RemotePageChild {
     gAllActors.add(this);
   }
 
-  actorDestroyed() {
-    super.actorDestroyed();
+  didDestroy() {
+    this._destroyed = true;
+    super.didDestroy?.();
     gAllActors.delete(this);
     this.#product?.uninit();
   }
@@ -46,12 +47,17 @@ export class ShoppingSidebarChild extends RemotePageChild {
     switch (message.name) {
       case "ShoppingSidebar:UpdateProductURL":
         let { url } = message.data;
-        let uri = Services.io.newURI(url);
-        if (this.#productURI?.equalsExceptRef(uri)) {
+        let uri = url ? Services.io.newURI(url) : null;
+        // If we're going from null to null, bail out:
+        if (!this.#productURI && !uri) {
+          return;
+        }
+        // Otherwise, check if we now have a product:
+        if (uri && this.#productURI?.equalsExceptRef(uri)) {
           return;
         }
         this.#productURI = uri;
-        this.updateContent();
+        this.updateContent({ haveUpdatedURI: true });
         break;
     }
   }
@@ -79,7 +85,37 @@ export class ShoppingSidebarChild extends RemotePageChild {
     return this.#productURI;
   }
 
-  async updateContent() {
+  /**
+   * This callback is invoked whenever something changes that requires
+   * re-rendering content. The expected cases for this are:
+   * - page navigations (both to new products and away from a product once
+   *   the sidebar has been created)
+   * - opt in state changes.
+   *
+   * @param {object?} options
+   *        Optional parameter object.
+   * @param {bool} options.haveUpdatedURI = false
+   *        Whether we've got an up-to-date URI already. If true, we avoid
+   *        fetching the URI from the parent, and assume `this.#productURI`
+   *        is current. Defaults to false.
+   *
+   */
+  async updateContent({ haveUpdatedURI = false } = {}) {
+    // updateContent is an async function, and when we're off making requests or doing
+    // other things asynchronously, the actor can be destroyed, the user
+    // might navigate to a new page, the user might disable the feature ... -
+    // all kinds of things can change. So we need to repeatedly check
+    // whether we can keep going with our async processes. This helper takes
+    // care of these checks.
+    let canContinue = (currentURI, checkURI = true) => {
+      if (this._destroyed || !this.canFetchAndShowData) {
+        return false;
+      }
+      if (!checkURI) {
+        return true;
+      }
+      return currentURI && currentURI == this.#productURI;
+    };
     this.#product?.uninit();
     // We are called either because the URL has changed or because the opt-in
     // state has changed. In both cases, we want to clear out content
@@ -91,10 +127,14 @@ export class ShoppingSidebarChild extends RemotePageChild {
     });
     if (this.canFetchAndShowData) {
       if (!this.#productURI) {
+        // If we already have a URI and it's just null, bail immediately.
+        if (haveUpdatedURI) {
+          return;
+        }
         let url = await this.sendQuery("GetProductURL");
 
         // Bail out if we opted out in the meantime, or don't have a URI.
-        if (lazy.optedIn !== 1 || !url) {
+        if (!canContinue(null, false)) {
           return;
         }
 
@@ -107,8 +147,8 @@ export class ShoppingSidebarChild extends RemotePageChild {
         console.error("Failed to fetch product analysis data", err);
         return { error: err };
       });
-      // Check if the product URI or opt in changed while we waited.
-      if (uri != this.#productURI || !this.canFetchAndShowData) {
+      // Check if we got nuked from orbit, or the product URI or opt in changed while we waited.
+      if (!canContinue(uri)) {
         return;
       }
       this.sendToContent("Update", {
@@ -120,6 +160,9 @@ export class ShoppingSidebarChild extends RemotePageChild {
   }
 
   sendToContent(eventName, detail) {
+    if (this._destroyed) {
+      return;
+    }
     let win = this.contentWindow;
     let evt = new win.CustomEvent(eventName, {
       bubbles: true,

browser/components/shopping/tests/browser/browser_shopping_urlbar.js

@@ -74,7 +74,7 @@ add_task(async function test_button_inactive() {
 });
 
 // Switching Tabs shows and hides the button
-add_task(async function test_button_changes_with_location() {
+add_task(async function test_button_changes_with_tabswitch() {
   Services.prefs.setBoolPref("browser.shopping.experience2023.active", true);
 
   let shoppingButton = document.getElementById("shopping-sidebar-button");
@@ -107,41 +107,45 @@ add_task(async function test_button_changes_with_location() {
 add_task(async function test_button_toggles_sidebars() {
   Services.prefs.setBoolPref("browser.shopping.experience2023.active", false);
 
-  let shoppingButton = document.getElementById("shopping-sidebar-button");
-  let browser = gBrowser.selectedBrowser;
-  let browserPanel = gBrowser.getPanel(browser);
+  await BrowserTestUtils.withNewTab(CONTENT_PAGE, async function (browser) {
+    let shoppingButton = document.getElementById("shopping-sidebar-button");
+    let browserPanel = gBrowser.getPanel(browser);
 
-  BrowserTestUtils.loadURIString(browser, PRODUCT_PAGE);
-  await BrowserTestUtils.browserLoaded(browser);
+    BrowserTestUtils.loadURIString(browser, PRODUCT_PAGE);
+    await BrowserTestUtils.browserLoaded(browser);
 
-  let sidebar = browserPanel.querySelector("shopping-sidebar");
+    let sidebar = browserPanel.querySelector("shopping-sidebar");
 
-  is(sidebar, null, "Shopping sidebar should be closed");
+    is(sidebar, null, "Shopping sidebar should be closed");
 
-  // open
-  shoppingButton.click();
-  await BrowserTestUtils.waitForMutationCondition(
-    shoppingButton,
-    {
-      attributeFilter: ["shoppingsidebaropen"],
-    },
-    () => shoppingButton.getAttribute("shoppingsidebaropen") == "true"
-  );
+    // open
+    shoppingButton.click();
+    await BrowserTestUtils.waitForMutationCondition(
+      shoppingButton,
+      {
+        attributeFilter: ["shoppingsidebaropen"],
+      },
+      () => shoppingButton.getAttribute("shoppingsidebaropen") == "true"
+    );
 
-  sidebar = browserPanel.querySelector("shopping-sidebar");
-  ok(BrowserTestUtils.is_visible(sidebar), "Shopping sidebar should be open");
+    sidebar = browserPanel.querySelector("shopping-sidebar");
+    ok(BrowserTestUtils.is_visible(sidebar), "Shopping sidebar should be open");
 
-  // close
-  shoppingButton.click();
-  await BrowserTestUtils.waitForMutationCondition(
-    shoppingButton,
-    {
-      attributeFilter: ["shoppingsidebaropen"],
-    },
-    () => shoppingButton.getAttribute("shoppingsidebaropen") == "false"
-  );
+    // close
+    shoppingButton.click();
+    await BrowserTestUtils.waitForMutationCondition(
+      shoppingButton,
+      {
+        attributeFilter: ["shoppingsidebaropen"],
+      },
+      () => shoppingButton.getAttribute("shoppingsidebaropen") == "false"
+    );
 
-  ok(BrowserTestUtils.is_hidden(sidebar), "Shopping sidebar should be closed");
+    ok(
+      BrowserTestUtils.is_hidden(sidebar),
+      "Shopping sidebar should be closed"
+    );
+  });
 });
 
 // Button changes all Windows
@@ -150,8 +154,7 @@ add_task(async function test_button_toggles_all_windows() {
 
   let shoppingButton = document.getElementById("shopping-sidebar-button");
 
-  BrowserTestUtils.loadURIString(gBrowser.selectedBrowser, PRODUCT_PAGE);
-  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PRODUCT_PAGE);
 
   let browserPanelA = gBrowser.getPanel(gBrowser.selectedBrowser);
   let sidebarA = browserPanelA.querySelector("shopping-sidebar");
@@ -164,12 +167,15 @@ add_task(async function test_button_toggles_all_windows() {
   );
   await BrowserTestUtils.browserLoaded(newWindow.gBrowser.selectedBrowser);
 
-  let browserPanelB = gBrowser.getPanel(newWindow.gBrowser.selectedBrowser);
+  let browserPanelB = newWindow.gBrowser.getPanel(
+    newWindow.gBrowser.selectedBrowser
+  );
   let sidebarB = browserPanelB.querySelector("shopping-sidebar");
 
-  ok(
-    BrowserTestUtils.is_hidden(sidebarA),
-    "Shopping sidebar should be closed in current window"
+  is(
+    sidebarA,
+    null,
+    "Shopping sidebar should not exist yet for new tab in current window"
   );
   is(sidebarB, null, "Shopping sidebar closed in new window");
 
@@ -212,5 +218,6 @@ add_task(async function test_button_toggles_all_windows() {
     "Shopping sidebar should be closed in new window"
   );
 
+  BrowserTestUtils.removeTab(tab);
   await BrowserTestUtils.closeWindow(newWindow);
 });