Bug 1742999 - Make DownloadSpamProtection per-window. r=Gijs,mak

This patch modifies DownloadSpamProtection and DownloadIntegration so
that each window will track blocked spam downloads separately. (Which
shouldn't affect permissions.) When a download is blocked, the helper
app service dispatches a notification, passing the relevant browsing
context and URL to DownloadIntegration. Then it passes the window and
URL to the singleton DownloadSpamProtection. That maps all the windows
to objects that carry the spam download objects. This allows us to only
show blocked spam downloads in the downloads panel of the window from
which they were triggered.

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

browser/components/downloads/DownloadSpamProtection.jsm

@@ -14,7 +14,7 @@ const { Download, DownloadError } = ChromeUtils.import(
   "resource://gre/modules/DownloadCore.jsm"
 );
 
-var { XPCOMUtils } = ChromeUtils.importESModule(
+const { XPCOMUtils } = ChromeUtils.importESModule(
   "resource://gre/modules/XPCOMUtils.sys.mjs"
 );
 
@@ -28,56 +28,256 @@ XPCOMUtils.defineLazyModuleGetters(lazy, {
 });
 
 /**
- * Responsible for detecting events related to downloads spam and updating the
- * downloads UI with this information.
+ * Each window tracks download spam independently, so one of these objects is
+ * constructed for each window. This is responsible for tracking the spam and
+ * updating the window's downloads UI accordingly.
  */
-class DownloadSpamProtection {
-  constructor() {
-    /**
-     * Tracks URLs we have detected download spam for.
-     * @type {Map<string, DownloadSpam>}
-     */
-    this._blockedURLToDownloadSpam = new Map();
-    this._browserWin = lazy.BrowserWindowTracker.getTopWindow();
-    this._indicator = lazy.DownloadsCommon.getIndicatorData(this._browserWin);
-    this.list = new lazy.DownloadList();
-  }
-
-  get spamList() {
-    return this.list;
-  }
-
-  update(url) {
-    if (this._blockedURLToDownloadSpam.has(url)) {
-      let downloadSpam = this._blockedURLToDownloadSpam.get(url);
-      this.spamList.remove(downloadSpam);
-      downloadSpam.blockedDownloadsCount += 1;
-      this.spamList.add(downloadSpam);
-      this._indicator.onDownloadStateChanged(downloadSpam);
-      return;
-    }
-
-    let downloadSpam = new DownloadSpam(url);
-    this.spamList.add(downloadSpam);
-    this._blockedURLToDownloadSpam.set(url, downloadSpam);
-    let hasActiveDownloads = lazy.DownloadsCommon.summarizeDownloads(
-      this._indicator._activeDownloads()
-    ).numDownloading;
-    if (!hasActiveDownloads) {
-      this._browserWin.DownloadsPanel.showPanel();
-    }
-    this._indicator.onDownloadAdded(downloadSpam);
+class WindowSpamProtection {
+  constructor(window) {
+    this._window = window;
   }
 
   /**
-   * Removes the download spam data for the current url.
+   * This map stores blocked spam downloads for the window, keyed by the
+   * download's source URL. This is done so we can track the number of times a
+   * given download has been blocked.
+   * @type {Map<String, DownloadSpam>}
    */
-  clearDownloadSpam(URL) {
-    if (this._blockedURLToDownloadSpam.has(URL)) {
-      let downloadSpam = this._blockedURLToDownloadSpam.get(URL);
+  _downloadSpamForUrl = new Map();
+
+  /**
+   * This set stores views that are waiting to have download notification
+   * listeners attached. They will be attached when the spamList is created
+   * (i.e. when the first spam download is blocked).
+   * @type {Set<Object>}
+   */
+  _pendingViews = new Set();
+
+  /**
+   * Set to true when we first start _blocking downloads in the window. This is
+   * used to lazily load the spamList. Spam downloads are rare enough that many
+   * sessions will have no blocked downloads. So we don't want to create a
+   * DownloadList unless we actually need it.
+   * @type {Boolean}
+   */
+  _blocking = false;
+
+  /**
+   * A per-window DownloadList for blocked spam downloads. Registered views will
+   * be sent notifications about downloads in this list, so that blocked spam
+   * downloads can be represented in the UI. If spam downloads haven't been
+   * blocked in the window, this will be undefined. See DownloadList.jsm.
+   * @type {DownloadList | undefined}
+   */
+  get spamList() {
+    if (!this._blocking) {
+      return undefined;
+    }
+    if (!this._spamList) {
+      this._spamList = new lazy.DownloadList();
+    }
+    return this._spamList;
+  }
+
+  /**
+   * A per-window downloads indicator whose state depends on notifications from
+   * DownloadLists registered in the window (for example, the visual state of
+   * the downloads toolbar button). See DownloadsCommon.jsm for more details.
+   * @type {DownloadsIndicatorData}
+   */
+  get indicator() {
+    if (!this._indicator) {
+      this._indicator = lazy.DownloadsCommon.getIndicatorData(this._window);
+    }
+    return this._indicator;
+  }
+
+  /**
+   * Add a blocked download to the spamList or increment the count of an
+   * existing blocked download, then notify listeners about this.
+   * @param {String} url
+   */
+  addDownloadSpam(url) {
+    this._blocking = true;
+    // Start listening on registered downloads views, if any exist.
+    this._maybeAddViews();
+    // If this URL is already paired with a DownloadSpam object, increment its
+    // blocked downloads count by 1 and don't open the downloads panel.
+    if (this._downloadSpamForUrl.has(url)) {
+      let downloadSpam = this._downloadSpamForUrl.get(url);
+      downloadSpam.blockedDownloadsCount += 1;
+      this.indicator.onDownloadStateChanged(downloadSpam);
+      return;
+    }
+    // Otherwise, create a new DownloadSpam object for the URL, add it to the
+    // spamList, and open the downloads panel.
+    let downloadSpam = new DownloadSpam(url);
+    this.spamList.add(downloadSpam);
+    this._downloadSpamForUrl.set(url, downloadSpam);
+    this._notifyDownloadSpamAdded(downloadSpam);
+  }
+
+  /**
+   * Notify the downloads panel that a new download has been added to the
+   * spamList. This is invoked when a new DownloadSpam object is created.
+   * @param {DownloadSpam} downloadSpam
+   */
+  _notifyDownloadSpamAdded(downloadSpam) {
+    let hasActiveDownloads = lazy.DownloadsCommon.summarizeDownloads(
+      this.indicator._activeDownloads()
+    ).numDownloading;
+    if (
+      !hasActiveDownloads &&
+      this._window === lazy.BrowserWindowTracker.getTopWindow()
+    ) {
+      // If there are no active downloads, open the downloads panel.
+      this._window.DownloadsPanel.showPanel();
+    } else {
+      // Otherwise, flash a taskbar/dock icon notification if available.
+      this._window.getAttention();
+    }
+    this.indicator.onDownloadAdded(downloadSpam);
+  }
+
+  /**
+   * Remove the download spam data for a given source URL.
+   * @param {String} url
+   */
+  removeDownloadSpamForUrl(url) {
+    if (this._downloadSpamForUrl.has(url)) {
+      let downloadSpam = this._downloadSpamForUrl.get(url);
       this.spamList.remove(downloadSpam);
-      this._indicator.onDownloadRemoved(downloadSpam);
-      this._blockedURLToDownloadSpam.delete(URL);
+      this.indicator.onDownloadRemoved(downloadSpam);
+      this._downloadSpamForUrl.delete(url);
+    }
+  }
+
+  /**
+   * Set up a downloads view (e.g. the downloads panel) to receive notifications
+   * about downloads in the spamList.
+   * @param {Object} view An object that implements handlers for download
+   *                      related notifications, like onDownloadAdded.
+   */
+  registerView(view) {
+    if (!view || this.spamList?._views.has(view)) {
+      return;
+    }
+    this._pendingViews.add(view);
+    this._maybeAddViews();
+  }
+
+  /**
+   * If any downloads have been blocked in the window, add download notification
+   * listeners for each downloads view that has been registered.
+   */
+  _maybeAddViews() {
+    if (this.spamList) {
+      for (let view of this._pendingViews) {
+        if (!this.spamList._views.has(view)) {
+          this.spamList.addView(view);
+        }
+      }
+      this._pendingViews.clear();
+    }
+  }
+
+  /**
+   * Remove download notification listeners for all views. This is invoked when
+   * the window is closed.
+   */
+  removeAllViews() {
+    if (this.spamList) {
+      for (let view of this.spamList._views) {
+        this.spamList.removeView(view);
+      }
+    }
+    this._pendingViews.clear();
+  }
+}
+
+/**
+ * Responsible for detecting events related to downloads spam and notifying the
+ * relevant window's WindowSpamProtection object. This is a singleton object,
+ * constructed by DownloadIntegration.jsm when the first download is blocked.
+ */
+class DownloadSpamProtection {
+  /**
+   * Stores spam protection data per-window.
+   * @type {WeakMap<Window, WindowSpamProtection>}
+   */
+  _forWindowMap = new WeakMap();
+
+  /**
+   * Add download spam data for a given source URL in the window where the
+   * download was blocked. This is invoked when a download is blocked by
+   * nsExternalAppHandler::IsDownloadSpam
+   * @param {String} url
+   * @param {Window} window
+   */
+  update(url, window) {
+    if (window == null) {
+      lazy.DownloadsCommon.log(
+        "Download spam blocked in a non-chrome window. URL: ",
+        url
+      );
+      return;
+    }
+    // Get the spam protection object for a given window or create one if it
+    // does not already exist. Also attach notification listeners to any pending
+    // downloads views.
+    let wsp =
+      this._forWindowMap.get(window) ?? new WindowSpamProtection(window);
+    this._forWindowMap.set(window, wsp);
+    wsp.addDownloadSpam(url);
+  }
+
+  /**
+   * Get the spam list for a given window (provided it exists).
+   * @param {Window} window
+   * @returns {DownloadList}
+   */
+  getSpamListForWindow(window) {
+    return this._forWindowMap.get(window)?.spamList;
+  }
+
+  /**
+   * Remove the download spam data for a given source URL in the passed window,
+   * if any exists.
+   * @param {String} url
+   * @param {Window} window
+   */
+  removeDownloadSpamForWindow(url, window) {
+    let wsp = this._forWindowMap.get(window);
+    wsp?.removeDownloadSpamForUrl(url);
+  }
+
+  /**
+   * Create the spam protection object for a given window (if not already
+   * created) and prepare to start listening for notifications on the passed
+   * downloads view. The bulk of resources won't be expended until a download is
+   * blocked. To add multiple views, call this method multiple times.
+   * @param {Object} view An object that implements handlers for download
+   *                      related notifications, like onDownloadAdded.
+   * @param {Window} window
+   */
+  register(view, window) {
+    let wsp =
+      this._forWindowMap.get(window) ?? new WindowSpamProtection(window);
+    // Try setting up the view now; it will be deferred if there's no spam.
+    wsp.registerView(view);
+    this._forWindowMap.set(window, wsp);
+  }
+
+  /**
+   * Remove the spam protection object for a window when it is closed.
+   * @param {Window} window
+   */
+  unregister(window) {
+    let wsp = this._forWindowMap.get(window);
+    if (wsp) {
+      // Stop listening on the view if it was previously set up.
+      wsp.removeAllViews();
+      this._forWindowMap.delete(window);
     }
   }
 }

browser/components/downloads/content/downloads.js

@@ -107,16 +107,10 @@ var DownloadsPanel = {
       "Attempting to initialize DownloadsPanel for a window."
     );
 
-    // Allow the download spam protection module to notify DownloadsView
-    // if it's been created.
-    if (
-      DownloadIntegration.downloadSpamProtection &&
-      !DownloadIntegration.downloadSpamProtection.spamList._views.has(
-        DownloadsView
-      )
-    ) {
-      DownloadIntegration.downloadSpamProtection.spamList.addView(
-        DownloadsView
+    if (DownloadIntegration.downloadSpamProtection) {
+      DownloadIntegration.downloadSpamProtection.register(
+        DownloadsView,
+        window
       );
     }
 
@@ -179,9 +173,7 @@ var DownloadsPanel = {
     this._unattachEventListeners();
 
     if (DownloadIntegration.downloadSpamProtection) {
-      DownloadIntegration.downloadSpamProtection.spamList.removeView(
-        DownloadsView
-      );
+      DownloadIntegration.downloadSpamProtection.unregister(window);
     }
 
     this._state = this.kStateUninitialized;

browser/components/downloads/test/browser/browser_download_spam_protection.js

@@ -40,10 +40,11 @@ add_setup(async function() {
     Services.perms.UNKNOWN_ACTION
   );
   await SpecialPowers.pushPrefEnv({
-    set: [
-      ["browser.download.improvements_to_download_panel", true],
-      ["browser.download.always_ask_before_handling_new_types", false], // To avoid the saving dialog being shown
-      ["browser.download.enable_spam_prevention", true],
+    set: [["browser.download.enable_spam_prevention", true]],
+    clear: [
+      ["browser.download.improvements_to_download_panel"],
+      ["browser.download.alwaysOpenPanel"],
+      ["browser.download.always_ask_before_handling_new_types"],
     ],
   });
 
@@ -57,12 +58,19 @@ add_setup(async function() {
 add_task(async function check_download_spam_ui() {
   await task_resetState();
 
+  let browserWin = BrowserWindowTracker.getTopWindow();
   registerCleanupFunction(async () => {
-    BrowserWindowTracker.getTopWindow().DownloadsPanel.hidePanel();
-    DownloadIntegration.downloadSpamProtection.clearDownloadSpam(TEST_URI);
+    for (let win of [browserWin, browserWin2]) {
+      win.DownloadsPanel.hidePanel();
+      DownloadIntegration.downloadSpamProtection.removeDownloadSpamForWindow(
+        TEST_URI,
+        win
+      );
+    }
     let publicList = await Downloads.getList(Downloads.PUBLIC);
     await publicList.removeFinished();
     BrowserTestUtils.removeTab(newTab);
+    await BrowserTestUtils.closeWindow(browserWin2);
   });
   let observedBlockedDownloads = 0;
   let gotAllBlockedDownloads = TestUtils.topicObserved(
@@ -73,7 +81,7 @@ add_task(async function check_download_spam_ui() {
   );
 
   let newTab = await BrowserTestUtils.openNewForegroundTab(
-    gBrowser,
+    browserWin.gBrowser,
     TEST_PATH + "test_spammy_page.html"
   );
 
@@ -83,11 +91,11 @@ add_task(async function check_download_spam_ui() {
     newTab.linkedBrowser
   );
 
-  info("Waiting on all blocked downloads.");
+  info("Waiting on all blocked downloads");
   await gotAllBlockedDownloads;
 
-  let spamProtection = DownloadIntegration.downloadSpamProtection;
-  let spamList = spamProtection.spamList;
+  let { downloadSpamProtection } = DownloadIntegration;
+  let spamList = downloadSpamProtection.getSpamListForWindow(browserWin);
   is(
     spamList._downloads[0].blockedDownloadsCount,
     99,
@@ -103,7 +111,7 @@ add_task(async function check_download_spam_ui() {
     "Verdict is DownloadSpam"
   );
 
-  let browserWin = BrowserWindowTracker.getTopWindow();
+  browserWin.focus();
   await BrowserTestUtils.waitForPopupEvent(
     browserWin.DownloadsPanel.panel,
     "shown"
@@ -112,7 +120,7 @@ add_task(async function check_download_spam_ui() {
   ok(browserWin.DownloadsPanel.isPanelShowing, "Download panel should open");
   await Downloads.getList(Downloads.PUBLIC);
 
-  let listbox = document.getElementById("downloadsListBox");
+  let listbox = browserWin.document.getElementById("downloadsListBox");
   ok(listbox, "Download list box present");
 
   await TestUtils.waitForCondition(() => {
@@ -126,4 +134,94 @@ add_task(async function check_download_spam_ui() {
     : listbox.itemChildren[1];
 
   ok(spamElement.classList.contains("temporary-block"), "Download is blocked");
+
+  info("Testing spam protection in a second window");
+
+  browserWin.DownloadsPanel.hidePanel();
+  DownloadIntegration.downloadSpamProtection.removeDownloadSpamForWindow(
+    TEST_URI,
+    browserWin
+  );
+
+  ok(
+    !browserWin.DownloadsPanel.isPanelShowing,
+    "Download panel should be closed in first window"
+  );
+  is(
+    listbox.childElementCount,
+    1,
+    "First window's download list should have one item - the download that wasn't blocked"
+  );
+
+  let browserWin2 = await BrowserTestUtils.openNewBrowserWindow();
+  let observedBlockedDownloads2 = 0;
+  let gotAllBlockedDownloads2 = TestUtils.topicObserved(
+    "blocked-automatic-download",
+    () => {
+      return ++observedBlockedDownloads2 >= 100;
+    }
+  );
+
+  let newTab2 = await BrowserTestUtils.openNewForegroundTab(
+    browserWin2.gBrowser,
+    TEST_PATH + "test_spammy_page.html"
+  );
+  await BrowserTestUtils.synthesizeMouseAtCenter(
+    "body",
+    {},
+    newTab2.linkedBrowser
+  );
+
+  info("Waiting on all blocked downloads in second window");
+  await gotAllBlockedDownloads2;
+
+  let spamList2 = downloadSpamProtection.getSpamListForWindow(browserWin2);
+  is(
+    spamList2._downloads[0].blockedDownloadsCount,
+    100,
+    "100 blocked downloads recorded in second window"
+  );
+  ok(
+    !spamList._downloads[0]?.blockedDownloadsCount,
+    "No blocked downloads in first window"
+  );
+
+  browserWin2.focus();
+  await BrowserTestUtils.waitForPopupEvent(
+    browserWin2.DownloadsPanel.panel,
+    "shown"
+  );
+
+  ok(
+    browserWin2.DownloadsPanel.isPanelShowing,
+    "Download panel should open in second window"
+  );
+
+  ok(
+    !browserWin.DownloadsPanel.isPanelShowing,
+    "Download panel should not open in first window"
+  );
+
+  let listbox2 = browserWin2.document.getElementById("downloadsListBox");
+  ok(listbox2, "Download list box present");
+
+  await TestUtils.waitForCondition(() => {
+    return (
+      listbox2.childElementCount == 2 && !listbox2.getAttribute("disabled")
+    );
+  }, "2 downloads = 1 allowed download from first window, and 1 for 100 downloads blocked in second window");
+
+  is(
+    listbox.childElementCount,
+    1,
+    "First window's download list should still have one item - the download that wasn't blocked"
+  );
+
+  let spamElement2 = listbox2.itemChildren[0].classList.contains(
+    "temporary-block"
+  )
+    ? listbox2.itemChildren[0]
+    : listbox2.itemChildren[1];
+
+  ok(spamElement2.classList.contains("temporary-block"), "Download is blocked");
 });

toolkit/components/downloads/DownloadIntegration.jsm

@@ -932,12 +932,15 @@ var DownloadIntegration = {
   _getDirectory(name) {
     return Services.dirsvc.get(name, Ci.nsIFile).path;
   },
+
   /**
    * Initializes the DownloadSpamProtection instance.
    * This is used to observe and group multiple automatic downloads.
    */
   _initializeDownloadSpamProtection() {
-    this.downloadSpamProtection = new lazy.DownloadSpamProtection();
+    if (!this.downloadSpamProtection) {
+      this.downloadSpamProtection = new lazy.DownloadSpamProtection();
+    }
   },
 
   /**
@@ -1191,13 +1194,13 @@ var DownloadObserver = {
         }
         break;
       case "blocked-automatic-download":
-        if (
-          AppConstants.MOZ_BUILD_APP == "browser" &&
-          !DownloadIntegration.downloadSpamProtection
-        ) {
+        if (AppConstants.MOZ_BUILD_APP == "browser") {
           DownloadIntegration._initializeDownloadSpamProtection();
+          DownloadIntegration.downloadSpamProtection.update(
+            aData,
+            aSubject.topChromeWindow
+          );
         }
-        DownloadIntegration.downloadSpamProtection.update(aData);
         break;
     }
   },

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1954,11 +1954,13 @@ bool nsExternalAppHandler::IsDownloadSpam(nsIChannel* aChannel) {
     if (capability == nsIPermissionManager::PROMPT_ACTION) {
       nsCOMPtr<nsIObserverService> observerService =
           mozilla::services::GetObserverService();
+      RefPtr<BrowsingContext> browsingContext;
+      loadInfo->GetBrowsingContext(getter_AddRefs(browsingContext));
 
       nsAutoCString cStringURI;
       loadInfo->TriggeringPrincipal()->GetPrePath(cStringURI);
       observerService->NotifyObservers(
-          nullptr, "blocked-automatic-download",
+          browsingContext, "blocked-automatic-download",
           NS_ConvertASCIItoUTF16(cStringURI.get()).get());
       // FIXME: In order to escape memory leaks, currently we cancel blocked
       // downloads. This is temporary solution, because download data should be

uriloader/exthandler/tests/mochitest/browser_download_spam_permissions.js

@@ -80,7 +80,10 @@ add_task(async function check_download_spam_permissions() {
     TEST_PATH + "test_spammy_page.html"
   );
   registerCleanupFunction(async () => {
-    DownloadIntegration.downloadSpamProtection.clearDownloadSpam(TEST_URI);
+    DownloadIntegration.downloadSpamProtection.removeDownloadSpamForWindow(
+      TEST_URI,
+      window
+    );
     DownloadsPanel.hidePanel();
     await publicList.removeFinished();
     BrowserTestUtils.removeTab(newTab);