From 260704f2d1d92b8d33c5956669dce1cfd9786bfd Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Mon, 10 Jan 2022 14:10:23 +0000 Subject: [PATCH] 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 --- .../debugger/src/actions/event-listeners.js | 2 +- .../browser_dbg-event-breakpoints.js | 106 +++++++++++++++--- .../actors/targets/target-actor-mixin.js | 4 +- devtools/server/actors/thread.js | 28 +++++ 4 files changed, 123 insertions(+), 17 deletions(-) diff --git a/devtools/client/debugger/src/actions/event-listeners.js b/devtools/client/debugger/src/actions/event-listeners.js index bbdc0a3e946d..9c59e930a7a2 100644 --- a/devtools/client/debugger/src/actions/event-listeners.js +++ b/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) { diff --git a/devtools/client/debugger/test/mochitest/browser_dbg-event-breakpoints.js b/devtools/client/debugger/test/mochitest/browser_dbg-event-breakpoints.js index 1197198ffc0b..17e5582eb1ac 100644 --- a/devtools/client/debugger/test/mochitest/browser_dbg-event-breakpoints.js +++ b/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(); diff --git a/devtools/server/actors/targets/target-actor-mixin.js b/devtools/server/actors/targets/target-actor-mixin.js index b1bb0c44c3d7..671d3b674bcd 100644 --- a/devtools/server/actors/targets/target-actor-mixin.js +++ b/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(); diff --git a/devtools/server/actors/thread.js b/devtools/server/actors/thread.js index 41deba421622..d51b1a610368 100644 --- a/devtools/server/actors/thread.js +++ b/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} 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} 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} ids: events to add breakpoint for (e.g. ["event.mouse.click","event.mouse.mousedown"]) + */ setActiveEventBreakpoints(ids) { this._activeEventBreakpoints = new Set(ids);