Bug 1720512 - [devtools] Avoid storing and restoring broken breakpoints. r=jdescottes,bomsy
authorAlexandre Poirot <poirot.alex@gmail.com>
Mon, 19 Jul 2021 15:30:34 +0000
changeset 585934 9c579e2ce174b063bdba7b0a7d43896949c19822
parent 585933 80dc8f306af0182c503cd73b7eebcd44544e89f4
child 585935 f0914306ef83eedbdb4d6cbf81a93b4a7e7d7dfd
push id38624
push usernerli@mozilla.com
push dateMon, 19 Jul 2021 21:50:29 +0000
treeherdermozilla-central@9c579e2ce174 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes, bomsy
bugs1720512
milestone92.0a1
first release with
nightly linux32
9c579e2ce174 / 92.0a1 / 20210719215029 / files
nightly linux64
9c579e2ce174 / 92.0a1 / 20210719215029 / files
nightly mac
9c579e2ce174 / 92.0a1 / 20210719215029 / files
nightly win32
9c579e2ce174 / 92.0a1 / 20210719215029 / files
nightly win64
9c579e2ce174 / 92.0a1 / 20210719215029 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1720512 - [devtools] Avoid storing and restoring broken breakpoints. r=jdescottes,bomsy This is a bit surprising that broken breakpoints are stored as the frontend seems to try to do a couple of assertions: https://searchfox.org/mozilla-central/rev/c0fc8c4852e927b0ae75d893d35772b8c60ee06b/devtools/client/debugger/src/utils/breakpoint/index.js#84-106 But at least, now, we start using same assertions in both sides. Differential Revision: https://phabricator.services.mozilla.com/D120002
devtools/client/debugger/src/main.js
devtools/client/debugger/src/utils/bootstrap.js
devtools/client/shared/thread-utils.js
devtools/server/actors/watcher/WatchedDataHelpers.jsm
devtools/shared/moz.build
devtools/shared/validate-breakpoint.jsm
--- a/devtools/client/debugger/src/main.js
+++ b/devtools/client/debugger/src/main.js
@@ -12,20 +12,21 @@ import {
   bootstrapStore,
   bootstrapWorkers,
   unmountRoot,
   teardownWorkers,
 } from "./utils/bootstrap";
 
 import { initialBreakpointsState } from "./reducers/breakpoints";
 import { initialSourcesState } from "./reducers/sources";
+const { sanitizeBreakpoints } = require("devtools/client/shared/thread-utils");
 
 async function syncBreakpoints() {
   const breakpoints = await asyncStore.pendingBreakpoints;
-  const breakpointValues = Object.values(breakpoints);
+  const breakpointValues = Object.values(sanitizeBreakpoints(breakpoints));
   return Promise.all(
     breakpointValues.map(({ disabled, options, generatedLocation }) => {
       if (!disabled) {
         return firefox.clientCommands.setBreakpoint(generatedLocation, options);
       }
     })
   );
 }
@@ -45,17 +46,19 @@ function setPauseOnExceptions() {
   const { pauseOnExceptions, pauseOnCaughtException } = prefs;
   return firefox.clientCommands.pauseOnExceptions(
     pauseOnExceptions,
     pauseOnCaughtException
   );
 }
 
 async function loadInitialState() {
-  const pendingBreakpoints = await asyncStore.pendingBreakpoints;
+  const pendingBreakpoints = sanitizeBreakpoints(
+    await asyncStore.pendingBreakpoints
+  );
   const tabs = { tabs: await asyncStore.tabs };
   const xhrBreakpoints = await asyncStore.xhrBreakpoints;
   const tabsBlackBoxed = await asyncStore.tabsBlackBoxed;
   const eventListenerBreakpoints = await asyncStore.eventListenerBreakpoints;
   const breakpoints = initialBreakpointsState(xhrBreakpoints);
   const sources = initialSourcesState({ tabsBlackBoxed });
 
   return {
--- a/devtools/client/debugger/src/utils/bootstrap.js
+++ b/devtools/client/debugger/src/utils/bootstrap.js
@@ -17,16 +17,17 @@ import * as prettyPrint from "../workers
 import { ParserDispatcher } from "../workers/parser";
 
 import configureStore from "../actions/utils/create-store";
 import reducers from "../reducers";
 import * as selectors from "../selectors";
 import App from "../components/App";
 import { asyncStore, prefs } from "./prefs";
 import { persistTabs } from "../utils/tabs";
+const { sanitizeBreakpoints } = require("devtools/client/shared/thread-utils");
 
 let parser;
 
 export function bootstrapStore(client, workers, panel, initialState) {
   const debugJsModules = AppConstants.DEBUG_JS_MODULES == "1";
   const createStore = configureStore({
     log: prefs.logging || flags.testing,
     timing: debugJsModules,
@@ -103,17 +104,19 @@ function registerStoreObserver(store, su
   });
 }
 
 function updatePrefs(state, oldState) {
   const hasChanged = selector =>
     selector(oldState) && selector(oldState) !== selector(state);
 
   if (hasChanged(selectors.getPendingBreakpoints)) {
-    asyncStore.pendingBreakpoints = selectors.getPendingBreakpoints(state);
+    asyncStore.pendingBreakpoints = sanitizeBreakpoints(
+      selectors.getPendingBreakpoints(state)
+    );
   }
 
   if (
     oldState.eventListenerBreakpoints &&
     oldState.eventListenerBreakpoints !== state.eventListenerBreakpoints
   ) {
     asyncStore.eventListenerBreakpoints = state.eventListenerBreakpoints;
   }
--- a/devtools/client/shared/thread-utils.js
+++ b/devtools/client/shared/thread-utils.js
@@ -1,15 +1,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
 var Services = require("Services");
 const asyncStoreHelper = require("devtools/client/shared/async-store-helper");
+const {
+  validateBreakpointLocation,
+} = require("devtools/shared/validate-breakpoint.jsm");
 
 const asyncStore = asyncStoreHelper("debugger", {
   pendingBreakpoints: ["pending-breakpoints", {}],
   tabs: ["tabs", []],
   xhrBreakpoints: ["xhr-breakpoints", []],
   eventListenerBreakpoints: ["event-listener-breakpoints", undefined],
   tabsBlackBoxed: ["tabsBlackBoxed", []],
 });
@@ -35,17 +38,53 @@ exports.getThreadOptions = async functio
     skipBreakpoints: Services.prefs.getBoolPref(
       "devtools.debugger.skip-pausing"
     ),
     logEventBreakpoints: Services.prefs.getBoolPref(
       "devtools.debugger.log-event-breakpoints"
     ),
     // This option is always true. See Bug 1654590 for removal.
     observeAsmJS: true,
-    breakpoints: await asyncStore.pendingBreakpoints,
+    breakpoints: sanitizeBreakpoints(await asyncStore.pendingBreakpoints),
     // XXX: `event-listener-breakpoints` is a copy of the event-listeners state
     // of the debugger panel. The `active` property is therefore linked to
     // the `active` property of the state.
     // See devtools/client/debugger/src/reducers/event-listeners.js
     eventBreakpoints:
       ((await asyncStore.eventListenerBreakpoints) || {}).active || [],
   };
 };
+
+/**
+ * Bug 1720512 - We used to store invalid breakpoints, leading to blank debugger.
+ * Filter out only the one that look invalid.
+ */
+function sanitizeBreakpoints(breakpoints) {
+  if (typeof breakpoints != "object") {
+    return {};
+  }
+  // We are not doing any assertion against keys,
+  // as it looks like we are never using them anywhere in frontend, nor backend.
+  const validBreakpoints = {};
+  for (const key in breakpoints) {
+    const bp = breakpoints[key];
+    try {
+      if (!bp) {
+        throw new Error("Undefined breakpoint");
+      }
+      // Debugger's main.js's `syncBreakpoints` will only use generatedLocation
+      // when restoring breakpoints.
+      validateBreakpointLocation(bp.generatedLocation);
+      // But Toolbox will still pass location to thread actor's reconfigure
+      // for target that don't support watcher+BreakpointListActor
+      validateBreakpointLocation(bp.location);
+      validBreakpoints[key] = bp;
+    } catch (e) {
+      console.error(
+        "Ignore invalid breakpoint from debugger store",
+        bp,
+        e.message
+      );
+    }
+  }
+  return validBreakpoints;
+}
+exports.sanitizeBreakpoints = sanitizeBreakpoints;
--- a/devtools/server/actors/watcher/WatchedDataHelpers.jsm
+++ b/devtools/server/actors/watcher/WatchedDataHelpers.jsm
@@ -6,58 +6,40 @@
 
 /**
  * Helper module alongside WatcherRegistry, which focus on updating the "watchedData" object.
  * This object is shared across processes and threads and have to be maintained in all these runtimes.
  */
 
 var EXPORTED_SYMBOLS = ["WatchedDataHelpers"];
 
+// Allow this JSM to also be loaded as a CommonJS module
+// Because this module is used from the worker thread,
+// (via target-actor-mixin), and workers can't load JSMs via ChromeUtils.import.
+const { validateBreakpointLocation } =
+  typeof module == "object"
+    ? require("devtools/shared/validate-breakpoint.jsm")
+    : ChromeUtils.import("resource://devtools/shared/validate-breakpoint.jsm");
+
 // List of all arrays stored in `watchedData`, which are replicated across processes and threads
 const SUPPORTED_DATA = {
   BREAKPOINTS: "breakpoints",
   XHR_BREAKPOINTS: "xhr-breakpoints",
   RESOURCES: "resources",
   TARGET_CONFIGURATION: "target-configuration",
   THREAD_CONFIGURATION: "thread-configuration",
   TARGETS: "targets",
 };
 
 // Optional function, if data isn't a primitive data type in order to produce a key
 // for the given data entry
 const DATA_KEY_FUNCTION = {
-  [SUPPORTED_DATA.BREAKPOINTS]: function({
-    location: { sourceUrl, sourceId, line, column },
-  }) {
-    if (!sourceUrl && !sourceId) {
-      throw new Error(
-        `Breakpoints expect to have either a sourceUrl or a sourceId.`
-      );
-    }
-    if (sourceUrl && typeof sourceUrl != "string") {
-      throw new Error(
-        `Breakpoints expect to have sourceUrl string, got ${typeof sourceUrl} instead.`
-      );
-    }
-    // sourceId may be undefined for some sources keyed by URL
-    if (sourceId && typeof sourceId != "string") {
-      throw new Error(
-        `Breakpoints expect to have sourceId string, got ${typeof sourceId} instead.`
-      );
-    }
-    if (typeof line != "number") {
-      throw new Error(
-        `Breakpoints expect to have line number, got ${typeof line} instead.`
-      );
-    }
-    if (typeof column != "number") {
-      throw new Error(
-        `Breakpoints expect to have column number, got ${typeof column} instead.`
-      );
-    }
+  [SUPPORTED_DATA.BREAKPOINTS]: function({ location }) {
+    validateBreakpointLocation(location);
+    const { sourceUrl, sourceId, line, column } = location;
     return `${sourceUrl}:${sourceId}:${line}:${column}`;
   },
   [SUPPORTED_DATA.TARGET_CONFIGURATION]: function({ key }) {
     // Configuration data entries are { key, value } objects, `key` can be used
     // as the unique identifier for the entry.
     return key;
   },
   [SUPPORTED_DATA.THREAD_CONFIGURATION]: function({ key }) {
--- a/devtools/shared/moz.build
+++ b/devtools/shared/moz.build
@@ -67,12 +67,13 @@ DevToolsModules(
     "natural-sort.js",
     "path.js",
     "picker-constants.js",
     "plural-form.js",
     "protocol.js",
     "system.js",
     "ThreadSafeDevToolsUtils.js",
     "throttle.js",
+    "validate-breakpoint.jsm",
 )
 
 with Files("**"):
     BUG_COMPONENT = ("DevTools", "General")
new file mode 100644
--- /dev/null
+++ b/devtools/shared/validate-breakpoint.jsm
@@ -0,0 +1,49 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// Because this function is used from WatchedDataHelpers.jsm,
+// this has to be a JSM.
+
+var EXPORTED_SYMBOLS = ["validateBreakpointLocation"];
+
+/**
+ * Given a breakpoint location object, throws if the breakpoint look invalid
+ */
+function validateBreakpointLocation({ sourceUrl, sourceId, line, column }) {
+  if (!sourceUrl && !sourceId) {
+    throw new Error(
+      `Breakpoints expect to have either a sourceUrl or a sourceId.`
+    );
+  }
+  if (sourceUrl && typeof sourceUrl != "string") {
+    throw new Error(
+      `Breakpoints expect to have sourceUrl string, got ${typeof sourceUrl} instead.`
+    );
+  }
+  // sourceId may be undefined for some sources keyed by URL
+  if (sourceId && typeof sourceId != "string") {
+    throw new Error(
+      `Breakpoints expect to have sourceId string, got ${typeof sourceId} instead.`
+    );
+  }
+  if (typeof line != "number") {
+    throw new Error(
+      `Breakpoints expect to have line number, got ${typeof line} instead.`
+    );
+  }
+  if (typeof column != "number") {
+    throw new Error(
+      `Breakpoints expect to have column number, got ${typeof column} instead.`
+    );
+  }
+}
+
+// Allow this JSM to also be loaded as a CommonJS module
+// Because this module is used from the worker thread,
+// and workers can't load JSMs.
+if (typeof module == "object") {
+  module.exports.validateBreakpointLocation = validateBreakpointLocation;
+}