From 66e0c52cd59aa99c36d1edd77d32fce2e9f60447 Mon Sep 17 00:00:00 2001
From: Alexandre Poirot <poirot.alex@gmail.com>
Date: Thu, 27 Feb 2025 18:00:13 +0000
Subject: [PATCH] Bug 1943130 - [devtools] Avoid fetching breakable lines more
 than once per source. r=devtools-reviewers,bomsy

We can't use either `memoizableAction`, nor the `promise` middleware.
* `memoizableAction` helps memoize on the long run,
  but doesn't handle mutliple calls made for the same ressource
  *before* the async computation of the value.
* `promise` middleware explicitely prevents exposing the actual
  Promise to the reducer, which prevents exposing it to the selector.

Ultimately, we have to expose that async promise done to compute the reducer data,
that's the one the frontend should wait for on any subsequent run for the same ressource.

These two existing APIs are weak and should probably be removed in favor
of something similar to the current patch.
Last, but not least, this approach doesn't leak all data indefinitely
until DevTools closes (`memoizableAction` does)

Also tweak the new-source action in order to force fetching breakable lines
of HTML sources, only if that source is currently selected.
We were computing breakable lines for all HTML sources, for example when
having iframes.

Differential Revision: https://phabricator.services.mozilla.com/D235208
---
 .../src/actions/sources/breakableLines.js     | 87 ++++++++++++++-----
 .../src/actions/sources/newSources.js         | 25 ++++--
 .../debugger/src/reducers/source-actors.js    |  7 +-
 .../client/debugger/src/reducers/sources.js   |  6 +-
 .../debugger/src/selectors/source-actors.js   | 27 +++---
 .../client/debugger/src/selectors/sources.js  | 10 ++-
 6 files changed, 116 insertions(+), 46 deletions(-)

diff --git a/devtools/client/debugger/src/actions/sources/breakableLines.js b/devtools/client/debugger/src/actions/sources/breakableLines.js
index 98d0b49a3717d1..cec8357221df89 100644
--- a/devtools/client/debugger/src/actions/sources/breakableLines.js
+++ b/devtools/client/debugger/src/actions/sources/breakableLines.js
@@ -5,6 +5,7 @@
 import {
   getBreakableLines,
   getSourceActorBreakableLines,
+  getSourceActorsForSource,
 } from "../../selectors/index";
 import { setBreakpointPositions } from "../breakpoints/breakpointPositions";
 
@@ -22,47 +23,93 @@ function calculateBreakableLines(positions) {
 /**
  * Ensure that breakable lines for a given source are fetched.
  *
+ * This method is memoized, so that if concurrent calls are dispatched,
+ * it will return the first async promise processing the breakable lines.
+ *
  * @param Object location
  */
 export function setBreakableLines(location) {
   return async ({ getState, dispatch, client }) => {
-    let breakableLines;
     if (location.source.isOriginal) {
-      const positions = await dispatch(setBreakpointPositions(location));
-      breakableLines = calculateBreakableLines(positions);
-
-      const existingBreakableLines = getBreakableLines(
-        getState(),
-        location.source.id
-      );
-      if (existingBreakableLines) {
-        breakableLines = [
-          ...new Set([...existingBreakableLines, ...breakableLines]),
-        ];
+      // Original sources have a dedicated codepath to fetch locations
+      // from the generated source actor and then map them to "positions"
+      // in the original source.
+      let promise = getBreakableLines(getState(), location.source.id);
+      if (promise) {
+        return promise;
       }
-
+      promise = (async () => {
+        const positions = await dispatch(setBreakpointPositions(location));
+        return calculateBreakableLines(positions);
+      })();
+      dispatch({
+        type: "SET_ORIGINAL_BREAKABLE_LINES",
+        source: location.source,
+        promise,
+      });
+      const breakableLines = await promise;
       dispatch({
         type: "SET_ORIGINAL_BREAKABLE_LINES",
         source: location.source,
         breakableLines,
       });
+    } else if (location.source.isHTML) {
+      // For a given HTML page, we get a unique Source (for the html page),
+      // but many Source Actors (one per inline <script> tag and inlined event listeners).
+      // So that we have to get the breakable lines for each of them.
+      //
+      // Whereas, for regular source (with a url), when we have more than one source actor,
+      // it means that we evaled the source more than once.
+      const sourceActors = getSourceActorsForSource(
+        getState(),
+        location.source.id
+      );
+      if (!sourceActors) {
+        return null;
+      }
+      for (const sourceActor of sourceActors) {
+        let promise = getSourceActorBreakableLines(getState(), sourceActor.id);
+        if (promise) {
+          await promise;
+        } else {
+          promise = client.getSourceActorBreakableLines(sourceActor);
+          dispatch({
+            type: "SET_SOURCE_ACTOR_BREAKABLE_LINES",
+            sourceActor,
+            promise,
+          });
+          const breakableLines = await promise;
+          dispatch({
+            type: "SET_SOURCE_ACTOR_BREAKABLE_LINES",
+            sourceActor,
+            breakableLines,
+          });
+        }
+      }
     } else {
-      // Ignore re-fetching the breakable lines for source actor we already fetched
-      breakableLines = getSourceActorBreakableLines(
+      // Here is the handling of regular non-source-mapped non-html sources.
+      // We do fetch the breakable position for the specific source actor passed as argument.
+      // In case there is many source actors for the same URL, it will only affect one instance.
+      let promise = getSourceActorBreakableLines(
         getState(),
         location.sourceActor.id
       );
-      if (breakableLines) {
-        return;
+      if (promise) {
+        return promise;
       }
-      breakableLines = await client.getSourceActorBreakableLines(
-        location.sourceActor
-      );
+      promise = client.getSourceActorBreakableLines(location.sourceActor);
+      dispatch({
+        type: "SET_SOURCE_ACTOR_BREAKABLE_LINES",
+        sourceActor: location.sourceActor,
+        promise,
+      });
+      const breakableLines = await promise;
       dispatch({
         type: "SET_SOURCE_ACTOR_BREAKABLE_LINES",
         sourceActor: location.sourceActor,
         breakableLines,
       });
     }
+    return null;
   };
 }
diff --git a/devtools/client/debugger/src/actions/sources/newSources.js b/devtools/client/debugger/src/actions/sources/newSources.js
index e4859339d43791..345f164ae1d4a8 100644
--- a/devtools/client/debugger/src/actions/sources/newSources.js
+++ b/devtools/client/debugger/src/actions/sources/newSources.js
@@ -30,6 +30,7 @@ import {
   getSourceByActorId,
   getPendingSelectedLocation,
   getPendingBreakpointsForSource,
+  getSelectedLocation,
 } from "../../selectors/index";
 
 import { prefs } from "../../utils/prefs";
@@ -343,22 +344,28 @@ export function newGeneratedSources(sourceResources) {
     (async () => {
       await dispatch(loadSourceMapsForSourceActors(newSourceActors));
 
-      // We would like to sync breakpoints after we are done
-      // loading source maps as sometimes generated and original
-      // files share the same paths.
+      // We have to force fetching the breakable lines for any incoming source actor
+      // related to HTML page as we may have the HTML page selected,
+      // and already fetched its breakable lines and won't try to update
+      // the breakable lines for any late coming inline <script> tag.
+      const selectedLocation = getSelectedLocation(getState());
       for (const sourceActor of newSourceActors) {
-        // For HTML pages, we fetch all new incoming inline script,
-        // which will be related to one dedicated source actor.
-        // Whereas, for regular sources, if we have many source actors,
-        // this is for the same URL. And code expecting to have breakable lines
-        // will request breakable lines for that particular source actor.
-        if (sourceActor.sourceObject.isHTML) {
+        if (
+          selectedLocation?.source == sourceActor.sourceObject &&
+          sourceActor.sourceObject.isHTML
+        ) {
           await dispatch(
             setBreakableLines(
               createLocation({ source: sourceActor.sourceObject, sourceActor })
             )
           );
         }
+      }
+
+      // We would like to sync breakpoints after we are done
+      // loading source maps as sometimes generated and original
+      // files share the same paths.
+      for (const sourceActor of newSourceActors) {
         dispatch(
           checkPendingBreakpoints(sourceActor.sourceObject, sourceActor)
         );
diff --git a/devtools/client/debugger/src/reducers/source-actors.js b/devtools/client/debugger/src/reducers/source-actors.js
index fd99955742e99d..a745f567a18707 100644
--- a/devtools/client/debugger/src/reducers/source-actors.js
+++ b/devtools/client/debugger/src/reducers/source-actors.js
@@ -15,8 +15,9 @@ function initialSourceActorsState() {
     // See create.js: `createSourceActor` for the shape of the source actor objects.
     mutableSourceActors: new Map(),
 
-    // Map(Source Actor ID: string => Breakable lines: Array<Number>)
-    // The array is the list of all lines where breakpoints can be set
+    // Map(Source Actor ID: string => Breakable lines: Promise or Array<Number>)
+    // The array is the list of all lines where breakpoints can be set.
+    // The value can be a promise to indicate the lines are being loaded.
     mutableBreakableLines: new Map(),
 
     // Set(Source Actor ID: string)
@@ -71,7 +72,7 @@ export default function update(state = initialSourceActorsState(), action) {
     case "SET_SOURCE_ACTOR_BREAKABLE_LINES":
       state.mutableBreakableLines.set(
         action.sourceActor.id,
-        action.breakableLines
+        action.promise || action.breakableLines
       );
 
       return {
diff --git a/devtools/client/debugger/src/reducers/sources.js b/devtools/client/debugger/src/reducers/sources.js
index 97115a3342cc1b..6f86eb52289501 100644
--- a/devtools/client/debugger/src/reducers/sources.js
+++ b/devtools/client/debugger/src/reducers/sources.js
@@ -33,7 +33,9 @@ export function initialSourcesState() {
     /**
      * List of all breakable lines for original sources only.
      *
-     * Map(source id => array<int : breakable line numbers>)
+     * Map(source id => promise or array<int> : breakable line numbers>)
+     *
+     * The value can be a promise to indicate the lines are being loaded.
      */
     mutableOriginalBreakableLines: new Map(),
 
@@ -203,7 +205,7 @@ function update(state = initialSourcesState(), action) {
     case "SET_ORIGINAL_BREAKABLE_LINES": {
       state.mutableOriginalBreakableLines.set(
         action.source.id,
-        action.breakableLines
+        action.promise || action.breakableLines
       );
 
       return {
diff --git a/devtools/client/debugger/src/selectors/source-actors.js b/devtools/client/debugger/src/selectors/source-actors.js
index 3c9c08dc1dd604..ff985a6f16e5e4 100644
--- a/devtools/client/debugger/src/selectors/source-actors.js
+++ b/devtools/client/debugger/src/selectors/source-actors.js
@@ -76,8 +76,10 @@ export function getSourceActorsForThread(state, threadActorIDs) {
  * @param {Object} state
  * @param {String} sourceActorId
  *        Source Actor ID
- * @return {AsyncValue<Array<Number>>}
- *        List of all the breakable lines.
+ * @return {Promise<Array<Number>> | <Array<Number> | null}
+ *        - null when the breakable lines have not been requested yet
+ *        - Promise when the breakable lines are in process of being retrieved
+ *        - Array when the breakable lines are available
  */
 export function getSourceActorBreakableLines(state, sourceActorId) {
   return state.sourceActors.mutableBreakableLines.get(sourceActorId);
@@ -104,15 +106,18 @@ export function getSourceActorBreakableLines(state, sourceActorId) {
 export function getBreakableLinesForSourceActors(state, sourceActors, isHTML) {
   const allBreakableLines = [];
   for (const sourceActor of sourceActors) {
-    const breakableLines = state.sourceActors.mutableBreakableLines.get(
-      sourceActor.id
-    );
-    if (breakableLines) {
-      if (isHTML) {
-        allBreakableLines.push(...breakableLines);
-      } else {
-        return breakableLines;
-      }
+    const breakableLines = getSourceActorBreakableLines(state, sourceActor.id);
+
+    // Ignore the source actor if its breakable lines are still being fetched
+    // from the server
+    if (!breakableLines || breakableLines instanceof Promise) {
+      continue;
+    }
+
+    if (isHTML) {
+      allBreakableLines.push(...breakableLines);
+    } else {
+      return breakableLines;
     }
   }
   return allBreakableLines;
diff --git a/devtools/client/debugger/src/selectors/sources.js b/devtools/client/debugger/src/selectors/sources.js
index 80cfacd9f565b7..62b1b850009e39 100644
--- a/devtools/client/debugger/src/selectors/sources.js
+++ b/devtools/client/debugger/src/selectors/sources.js
@@ -355,7 +355,15 @@ export function getBreakableLines(state, sourceId) {
 export const getSelectedBreakableLines = createSelector(
   state => {
     const sourceId = getSelectedSourceId(state);
-    return sourceId && getBreakableLines(state, sourceId);
+    if (!sourceId) {
+      return null;
+    }
+    const breakableLines = getBreakableLines(state, sourceId);
+    // Ignore the breakable lines if they are still being fetched from the server
+    if (!breakableLines || breakableLines instanceof Promise) {
+      return null;
+    }
+    return breakableLines;
   },
   breakableLines => new Set(breakableLines || [])
 );