Bug 1748589 - [devtools] Fix event listener breakpoints toggling. r=bomsy.

In target-actor-mixin, we were calling `setActiveEventBreakpoints` only with the
new events we were receiving, which mean if the user was clicking a first event in
the UI, and then a second one, only the second one would have a functioning event breakpoint.
Also, we were not handling removing event breakpoints at all.
We're adding `(add|remove)EventBreakpoints` to the thread actor so it's easier
to update the list of event breakpoints.

The existing event breakpoints test is modified to ensure we don't regress such behaviour.

The call to `setEventListenerBreakpoints` is moved before dispatch the `UPDATE_EVENT_LISTENERS`
action so we can properly wait for the breakpoint to be set in tests.

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

devtools/client/debugger/src/actions/event-listeners.js

@@ -9,8 +9,8 @@ import {
 } from "../selectors";
 
 async function updateBreakpoints(dispatch, client, newEvents) {
-  dispatch({ type: "UPDATE_EVENT_LISTENERS", active: newEvents });
   await client.setEventListenerBreakpoints(newEvents);
+  dispatch({ type: "UPDATE_EVENT_LISTENERS", active: newEvents });
 }
 
 async function updateExpanded(dispatch, newExpanded) {

devtools/client/debugger/test/mochitest/browser_dbg-event-breakpoints.js

@@ -15,15 +15,8 @@ add_task(async function() {
   await selectSource(dbg, "event-breakpoints");
   await waitForSelectedSource(dbg, "event-breakpoints");
 
-  await dbg.actions.addEventListenerBreakpoints([
-    "event.control.focusin",
-    "event.control.focusout",
-    "event.mouse.click",
-    "event.xhr.load",
-    "timer.timeout.set",
-    "timer.timeout.fire",
-    "script.source.firstStatement",
-  ]);
+  // We want to set each breakpoint individually to test adding/removing breakpoints, see Bug 1748589.
+  await toggleEventBreakpoint(dbg, "Mouse", "event.mouse.click");
 
   invokeInTab("clickHandler");
   await waitForPaused(dbg);
@@ -32,15 +25,21 @@ add_task(async function() {
   const whyPaused = await waitFor(
     () => dbg.win.document.querySelector(".why-paused")?.innerText
   );
-  is(whyPaused, `Paused on event breakpoint\nDOM 'click' event`);
-
+  is(
+    whyPaused,
+    `Paused on event breakpoint\nDOM 'click' event`,
+    "whyPaused does state that the debugger is paused as a result of a click event breakpoint"
+  );
   await resume(dbg);
 
+  await toggleEventBreakpoint(dbg, "XHR", "event.xhr.load");
   invokeInTab("xhrHandler");
   await waitForPaused(dbg);
   assertPauseLocation(dbg, 20);
   await resume(dbg);
 
+  await toggleEventBreakpoint(dbg, "Timer", "timer.timeout.set");
+  await toggleEventBreakpoint(dbg, "Timer", "timer.timeout.fire");
   invokeInTab("timerHandler");
   await waitForPaused(dbg);
   assertPauseLocation(dbg, 27);
@@ -50,11 +49,14 @@ add_task(async function() {
   assertPauseLocation(dbg, 28);
   await resume(dbg);
 
+  await toggleEventBreakpoint(dbg, "Script", "script.source.firstStatement");
   invokeInTab("evalHandler");
   await waitForPaused(dbg);
   assertPauseLocation(dbg, 2, "https://example.com/eval-test.js");
   await resume(dbg);
 
+  await toggleEventBreakpoint(dbg, "Control", "event.control.focusin");
+  await toggleEventBreakpoint(dbg, "Control", "event.control.focusout");
   invokeOnElement("#focus-text", "focus");
   await waitForPaused(dbg);
   assertPauseLocation(dbg, 43);
@@ -65,24 +67,98 @@ add_task(async function() {
   assertPauseLocation(dbg, 48);
   await resume(dbg);
 
-  // Test that we don't pause on event breakpoints when source is blackboxed.
+  info("Check that the click event breakpoint is still enabled");
+  invokeInTab("clickHandler");
+  await waitForPaused(dbg);
+  assertPauseLocation(dbg, 12);
+  await resume(dbg);
+
+  info("Check that disabling an event breakpoint works");
+  await toggleEventBreakpoint(dbg, "Mouse", "event.mouse.click");
+  invokeInTab("clickHandler");
+  // wait for a bit to make sure the debugger do not pause
+  await wait(100);
+  assertNotPaused(dbg);
+
+  info("Check that we can re-eanble event breakpoints");
+  await toggleEventBreakpoint(dbg, "Mouse", "event.mouse.click");
+  invokeInTab("clickHandler");
+  await waitForPaused(dbg);
+  assertPauseLocation(dbg, 12);
+  await resume(dbg);
+
+  info(
+    "Test that we don't pause on event breakpoints when source is blackboxed."
+  );
   await clickElement(dbg, "blackbox");
   await waitForDispatch(dbg.store, "BLACKBOX");
 
   invokeInTab("clickHandler");
-  is(isPaused(dbg), false);
+  // wait for a bit to make sure the debugger do not pause
+  await wait(100);
+  assertNotPaused(dbg);
 
   invokeInTab("xhrHandler");
-  is(isPaused(dbg), false);
+  // wait for a bit to make sure the debugger do not pause
+  await wait(100);
+  assertNotPaused(dbg);
 
   invokeInTab("timerHandler");
-  is(isPaused(dbg), false);
+  // wait for a bit to make sure the debugger do not pause
+  await wait(100);
+  assertNotPaused(dbg);
 
   // Cleanup - unblackbox the source
   await clickElement(dbg, "blackbox");
   await waitForDispatch(dbg.store, "BLACKBOX");
 });
 
+function getEventListenersPanel(dbg) {
+  return findElementWithSelector(dbg, ".event-listeners-pane .event-listeners");
+}
+
+async function toggleEventBreakpoint(
+  dbg,
+  eventBreakpointGroup,
+  eventBreakpointName
+) {
+  if (!getEventListenersPanel(dbg)) {
+    // Event listeners panel is collapse, expand it
+    findElementWithSelector(
+      dbg,
+      `.event-listeners-pane ._header .arrow`
+    ).click();
+    await waitFor(() => getEventListenersPanel(dbg));
+  }
+
+  const groupCheckbox = findElementWithSelector(
+    dbg,
+    `input[value="${eventBreakpointGroup}"]`
+  );
+  const groupEl = groupCheckbox.closest(".event-listener-group");
+  let groupEventsUl = groupEl.querySelector("ul");
+  if (!groupEventsUl) {
+    info(
+      `Expand ${eventBreakpointGroup} and wait for the sub list to be displayed`
+    );
+    groupEl.querySelector(".event-listener-expand").click();
+    groupEventsUl = await waitFor(() => groupEl.querySelector("ul"));
+  }
+
+  const eventCheckbox = findElementWithSelector(
+    dbg,
+    `input[value="${eventBreakpointName}"]`
+  );
+  eventCheckbox.scrollIntoView();
+  info(`Toggle ${eventBreakpointName} breakpoint`);
+  const onEventListenersUpdate = waitForDispatch(
+    dbg.store,
+    "UPDATE_EVENT_LISTENERS"
+  );
+  eventCheckbox.click();
+  await onEventListenersUpdate;
+}
+
 function assertPauseLocation(dbg, line, url = "event-breakpoints.js") {
   const { location } = dbg.selectors.getVisibleSelectedFrame();
 

devtools/server/actors/targets/target-actor-mixin.js

@@ -116,7 +116,7 @@ module.exports = function(targetType, targetActorSpec, implementation) {
         ) {
           this.threadActor.attach();
         }
-        await this.threadActor.setActiveEventBreakpoints(entries);
+        this.threadActor.addEventBreakpoints(entries);
       }
     },
 
@@ -133,6 +133,8 @@ module.exports = function(targetType, targetActorSpec, implementation) {
         for (const { path, method } of entries) {
           this.threadActor.removeXHRBreakpoint(path, method);
         }
+      } else if (type == EVENT_BREAKPOINTS) {
+        this.threadActor.removeEventBreakpoints(entries);
       }
 
       return Promise.resolve();

devtools/server/actors/thread.js

@@ -595,6 +595,34 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   getActiveEventBreakpoints() {
     return Array.from(this._activeEventBreakpoints);
   },
+
+  /**
+   * Add event breakpoints to the list of active event breakpoints
+   *
+   * @param {Array<String>} ids: events to add (e.g. ["event.mouse.click","event.mouse.mousedown"])
+   */
+  addEventBreakpoints(ids) {
+    this.setActiveEventBreakpoints(
+      this.getActiveEventBreakpoints().concat(ids)
+    );
+  },
+
+  /**
+   * Remove event breakpoints from the list of active event breakpoints
+   *
+   * @param {Array<String>} ids: events to remove (e.g. ["event.mouse.click","event.mouse.mousedown"])
+   */
+  removeEventBreakpoints(ids) {
+    this.setActiveEventBreakpoints(
+      this.getActiveEventBreakpoints().filter(eventBp => !ids.includes(eventBp))
+    );
+  },
+
+  /**
+   * Set the the list of active event breakpoints
+   *
+   * @param {Array<String>} ids: events to add breakpoint for (e.g. ["event.mouse.click","event.mouse.mousedown"])
+   */
   setActiveEventBreakpoints(ids) {
     this._activeEventBreakpoints = new Set(ids);