Bug 1811049 - [cdp] Implement filter option for Target.setDiscoverTargets r=webdriver-reviewers,whimboo

Implement the filter option for Target.setDiscoverTargets, using
internal function created previously.

Also rewrote tests for Target.setDiscoverTargets, and some internal test
helpers.

Tweaked Puppeteer to accommodate for these changes, now using a
filter for Target.setDiscoverTargets like Chrome.
Upstream: https://github.com/puppeteer/puppeteer/pull/9693

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

remote/cdp/domains/parent/Target.sys.mjs

@@ -25,6 +25,7 @@ const defaultFilter = [
 
 export class Target extends Domain {
   #browserContextIds;
+  #discoverTargetFilter;
 
   constructor(session) {
     super(session);
@@ -77,8 +78,24 @@ export class Target extends Domain {
   }
 
   setDiscoverTargets(options = {}) {
-    const { discover } = options;
+    const { discover, filter } = options;
     const { targetList } = this.session.target;
+
+    if (typeof discover !== "boolean") {
+      throw new TypeError("discover: boolean value expected");
+    }
+
+    if (discover === false && filter !== undefined) {
+      throw new Error("filter: should not be present when discover is false");
+    }
+
+    // null filter should not be defaulted
+    const targetFilter = filter === undefined ? defaultFilter : filter;
+    this._validateTargetFilter(targetFilter);
+
+    // Store active filter for filtering in event listeners (targetCreated, targetDestroyed, targetInfoChanged)
+    this.#discoverTargetFilter = targetFilter;
+
     if (discover) {
       targetList.on("target-created", this._onTargetCreated);
       targetList.on("target-destroyed", this._onTargetDestroyed);
@@ -86,6 +103,7 @@ export class Target extends Domain {
       targetList.off("target-created", this._onTargetCreated);
       targetList.off("target-destroyed", this._onTargetDestroyed);
     }
+
     for (const target of targetList) {
       this._onTargetCreated("target-created", target);
     }
@@ -248,11 +266,19 @@ export class Target extends Domain {
   }
 
   _onTargetCreated(eventName, target) {
+    if (!this._filterIncludesTarget(target, this.#discoverTargetFilter)) {
+      return;
+    }
+
     const targetInfo = this._getTargetInfo(target);
     this.emit("Target.targetCreated", { targetInfo });
   }
 
   _onTargetDestroyed(eventName, target) {
+    if (!this._filterIncludesTarget(target, this.#discoverTargetFilter)) {
+      return;
+    }
+
     this.emit("Target.targetDestroyed", {
       targetId: target.id,
     });

remote/cdp/test/browser/head.js

@@ -199,23 +199,21 @@ function getTargets(CDP) {
   });
 }
 
-// Wait for all Target.targetCreated events. One for each tab, plus the one
-// for the main process target.
-async function getDiscoveredTargets(Target) {
-  return new Promise(resolve => {
-    const targets = [];
+// Wait for all Target.targetCreated events. One for each tab.
+async function getDiscoveredTargets(Target, options = {}) {
+  const { discover = true, filter } = options;
 
-    const unsubscribe = Target.targetCreated(target => {
-      targets.push(target);
-
-      if (targets.length >= gBrowser.tabs.length + 1) {
-        unsubscribe();
-        resolve(targets);
-      }
-    });
-
-    Target.setDiscoverTargets({ discover: true });
+  const targets = [];
+  const unsubscribe = Target.targetCreated(target => {
+    targets.push(target.targetInfo);
   });
+
+  await Target.setDiscoverTargets({
+    discover,
+    filter,
+  }).finally(() => unsubscribe());
+
+  return targets;
 }
 
 async function openTab(Target, options = {}) {

remote/cdp/test/browser/target/browser_activateTarget.js

@@ -31,7 +31,7 @@ add_task(async function selectTabInOtherWindow({ client, tab }) {
   const currentTargetId = target.id;
   const targets = await getDiscoveredTargets(Target);
   const filtered_targets = targets.filter(target => {
-    return target.targetInfo.targetId == currentTargetId;
+    return target.targetId == currentTargetId;
   });
   is(filtered_targets.length, 1, "The current target has been found");
   const initialTarget = filtered_targets[0];
@@ -60,7 +60,7 @@ add_task(async function selectTabInOtherWindow({ client, tab }) {
   try {
     is(newWindow, getFocusedNavigator(), "The new window is focused");
     await Target.activateTarget({
-      targetId: initialTarget.targetInfo.targetId,
+      targetId: initialTarget.targetId,
     });
     is(
       tab.ownerGlobal,

remote/cdp/test/browser/target/browser_setDiscoverTargets.js

@@ -3,65 +3,252 @@
 
 "use strict";
 
+// These tests are a near copy of the tests for Target.getTargets, but using
+// the `setDiscoverTargets` method and `targetCreated` events instead.
+// Calling `setDiscoverTargets` with `discover: true` will dispatch a
+// `targetCreated` event for all already opened tabs and NOT the browser target
+// with the default filter.
+
+const PAGE_TEST =
+  "https://example.com/browser/remote/cdp/test/browser/target/doc_test.html";
+
 add_task(
-  async function mainTarget({ client }) {
+  async function discoverInvalidTypes({ client }) {
     const { Target } = client;
-    const { targetInfo } = await new Promise(resolve => {
-      const unsubscribe = Target.targetCreated(target => {
-        if (target.targetInfo.type == "browser") {
-          unsubscribe();
-          resolve(target);
-        }
-      });
 
-      // Calling `setDiscoverTargets` will dispatch `targetCreated` event for all
-      // already opened tabs and the browser target.
-      Target.setDiscoverTargets({ discover: true });
-    });
+    for (const discover of [null, undefined, 1, "foo", [], {}]) {
+      info(`Checking discover with invalid value: ${discover}`);
 
-    ok(!!targetInfo, "Target info for main target has been found");
-    ok(!!targetInfo.targetId, "Main target has a non-empty target id");
-    is(targetInfo.type, "browser", "Type of target is browser");
+      let errorThrown = "";
+      try {
+        await Target.setDiscoverTargets({ discover });
+      } catch (e) {
+        errorThrown = e.message;
+      }
+
+      ok(
+        errorThrown.match(/discover: boolean value expected/),
+        `Discover fails for invalid type: ${discover}`
+      );
+    }
   },
   { createTab: false }
 );
 
-add_task(async function pageTargets({ client, tab }) {
-  const { Target, target } = client;
+add_task(
+  async function filterInvalid({ client }) {
+    const { Target } = client;
 
-  const currentTargetId = target.id;
-  const url = toDataURL("pageTargets");
-  await loadURL(url);
+    for (const filter of [null, true, 1, "foo", {}]) {
+      info(`Checking filter with invalid value: ${filter}`);
 
-  const targets = await new Promise(resolve => {
-    const targets = [];
-
-    const unsubscribe = Target.targetCreated(target => {
-      if (target.targetInfo.type == "page") {
-        targets.push(target);
-
-        // Return once all page targets have been discovered
-        if (targets.length == gBrowser.tabs.length) {
-          unsubscribe();
-          resolve(targets);
-        }
+      let errorThrown = "";
+      try {
+        await Target.setDiscoverTargets({ discover: true, filter });
+      } catch (e) {
+        errorThrown = e.message;
       }
-    });
 
-    // Calling `setDiscoverTargets` will dispatch `targetCreated` event for all
-    // already opened tabs and the browser target.
-    Target.setDiscoverTargets({ discover: true });
-  });
+      ok(
+        errorThrown.match(/filter: array value expected/),
+        `Filter fails for invalid type: ${filter}`
+      );
+    }
 
-  // Get the current target
-  const filtered_targets = targets.filter(target => {
-    return target.targetInfo.targetId == currentTargetId;
-  });
-  is(filtered_targets.length, 1, "The current target has been found");
-  const { targetInfo } = filtered_targets[0];
+    for (const filterEntry of [null, undefined, true, 1, "foo", []]) {
+      info(`Checking filter entry with invalid value: ${filterEntry}`);
 
-  ok(!!targetInfo, "Target info for current tab has been found");
-  ok(!!targetInfo.targetId, "Page target has a non-empty target id");
-  is(targetInfo.type, "page", "Type of current target is 'page'");
-  is(targetInfo.url, url, "Page target has a non-empty target id");
-});
+      let errorThrown = "";
+      try {
+        await Target.setDiscoverTargets({
+          discover: true,
+          filter: [filterEntry],
+        });
+      } catch (e) {
+        errorThrown = e.message;
+      }
+
+      ok(
+        errorThrown.match(/filter: object values expected in array/),
+        `Filter entry fails for invalid type: ${filterEntry}`
+      );
+    }
+
+    for (const type of [null, true, 1, [], {}]) {
+      info(`Checking filter entry with type as invalid value: ${type}`);
+
+      let errorThrown = "";
+      try {
+        await Target.setDiscoverTargets({
+          discover: true,
+          filter: [{ type }],
+        });
+      } catch (e) {
+        errorThrown = e.message;
+      }
+
+      ok(
+        errorThrown.match(/filter: type: string value expected/),
+        `Filter entry type fails for invalid type: ${type}`
+      );
+    }
+
+    for (const exclude of [null, 1, "foo", [], {}]) {
+      info(`Checking filter entry with exclude as invalid value: ${exclude}`);
+
+      let errorThrown = "";
+      try {
+        await Target.setDiscoverTargets({
+          discover: true,
+          filter: [{ exclude }],
+        });
+      } catch (e) {
+        errorThrown = e.message;
+      }
+
+      ok(
+        errorThrown.match(/filter: exclude: boolean value expected/),
+        `Filter entry exclude for invalid type: ${exclude}`
+      );
+    }
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function noFilterWithDiscoverFalse({ client }) {
+    const { Target } = client;
+
+    // Check filter cannot be given with discover: false
+    let errorThrown = "";
+    try {
+      await Target.setDiscoverTargets({
+        discover: false,
+        filter: [{}],
+      });
+    } catch (e) {
+      errorThrown = e.message;
+    }
+
+    ok(
+      errorThrown.match(/filter: should not be present when discover is false/),
+      `Error throw when given filter with discover false`
+    );
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function targetInfoValues({ client }) {
+    const { Target, target } = client;
+
+    await loadURL(PAGE_TEST);
+
+    const targets = await getDiscoveredTargets(Target);
+
+    Assert.equal(targets.length, 1, "Got expected amount of targets");
+
+    const targetInfo = targets[0];
+    is(targetInfo.id, target.id, "Got expected target id");
+    is(targetInfo.type, "page", "Got expected target type");
+    is(targetInfo.title, "Test Page", "Got expected target title");
+    is(targetInfo.url, PAGE_TEST, "Got expected target URL");
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function discoverEnabledAndMultipleTabs({ client }) {
+    const { Target, target } = client;
+    const { targetInfo: newTabTargetInfo } = await openTab(Target);
+
+    await loadURL(PAGE_TEST);
+
+    const targets = await getDiscoveredTargets(Target);
+
+    Assert.equal(targets.length, 2, "Got expected amount of targets");
+    const targetIds = targets.map(info => info.id);
+    ok(targetIds.includes(target.id), "Got expected original target id");
+    ok(targetIds.includes(newTabTargetInfo.id), "Got expected new target id");
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function allFilters({ client }) {
+    const { Target } = client;
+
+    await loadURL(PAGE_TEST);
+
+    for (const filter of [[{}], [{ type: "browser" }, { type: "page" }]]) {
+      // Blank/all filter so all targets are returned, including main process
+      const targets = await getDiscoveredTargets(Target, { filter });
+
+      is(targets.length, 2, "Got expected amount of targets with all filter");
+
+      const pageTarget = targets.find(info => info.type === "page");
+      ok(!!pageTarget, "Found page target in targets with all filter");
+
+      const mainProcessTarget = targets.find(info => info.type === "browser");
+      ok(
+        !!mainProcessTarget,
+        "Found main process target in targets with all filter"
+      );
+    }
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function pageOnlyFilters({ client }) {
+    const { Target } = client;
+
+    await loadURL(PAGE_TEST);
+
+    for (const filter of [
+      [{ type: "page" }],
+      [{ type: "browser", exclude: true }, { type: "page" }],
+    ]) {
+      // Filter so only page targets are returned
+      // This returns same as default but pass our own custom filter to ensure
+      // these filters still return what they should
+      const targets = await getDiscoveredTargets(Target, { filter });
+
+      is(targets.length, 1, "Got expected amount of targets with page filter");
+      is(
+        targets[0].type,
+        "page",
+        "Got expected type 'page' of target from page filter"
+      );
+    }
+  },
+  { createTab: false }
+);
+
+add_task(
+  async function browserOnlyFilters({ client }) {
+    const { Target } = client;
+
+    await loadURL(PAGE_TEST);
+
+    for (const filter of [
+      [{ type: "browser" }],
+      [{ type: "page", exclude: true }, {}],
+    ]) {
+      // Filter so only main process target is returned
+      const targets = await getDiscoveredTargets(Target, { filter });
+
+      is(
+        targets.length,
+        1,
+        "Got expected amount of targets with browser only filter"
+      );
+      is(
+        targets[0].type,
+        "browser",
+        "Got expected type 'browser' of target from browser only filter"
+      );
+    }
+  },
+  { createTab: false }
+);

remote/test/puppeteer/packages/puppeteer-core/src/common/FirefoxTargetManager.ts

@@ -166,7 +166,10 @@ export class FirefoxTargetManager
   }
 
   async initialize(): Promise<void> {
-    await this.#connection.send('Target.setDiscoverTargets', {discover: true});
+    await this.#connection.send('Target.setDiscoverTargets', {
+      discover: true,
+      filter: [{}],
+    });
     this.#targetsIdsForInit = new Set(this.#discoveredTargetsByTargetId.keys());
     await this.#initializePromise;
   }