Bug 1532791 - Breaking on exception even if option is off. r=davidwalsh
☠☠ backed out by 8b95b04d367d ☠ ☠
authorJason Laster <jlaster@mozilla.com>
Tue, 12 Mar 2019 15:41:39 +0000
changeset 521562 e2316f37b9882820db8e31203058cbecb004de64
parent 521561 d468fbd73e73a1b6a9ef4a848370ae71407b3b60
child 521563 75b49fcc27432b8fbc25ea3c5226445fd25190df
push id10867
push userdvarga@mozilla.com
push dateThu, 14 Mar 2019 15:20:45 +0000
treeherdermozilla-beta@abad13547875 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdavidwalsh
bugs1532791
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1532791 - Breaking on exception even if option is off. r=davidwalsh Differential Revision: https://phabricator.services.mozilla.com/D22186
devtools/client/debugger/new/src/actions/pause/pauseOnExceptions.js
devtools/client/debugger/new/src/actions/types/PauseAction.js
devtools/client/debugger/new/src/client/firefox/commands.js
devtools/client/debugger/new/src/reducers/pause.js
devtools/client/debugger/new/test/mochitest/helpers.js
devtools/client/framework/attach-thread.js
devtools/server/actors/thread.js
devtools/shared/client/thread-client.js
devtools/shared/specs/script.js
--- a/devtools/client/debugger/new/src/actions/pause/pauseOnExceptions.js
+++ b/devtools/client/debugger/new/src/actions/pause/pauseOnExceptions.js
@@ -2,42 +2,36 @@
  * 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/>. */
 
 // @flow
 
 import { PROMISE } from "../utils/middleware/promise";
 import { recordEvent } from "../../utils/telemetry";
 import type { ThunkArgs } from "../types";
-import { getCurrentThread } from "../../selectors";
 
 /**
  *
  * @memberof actions/pause
  * @static
  */
 export function pauseOnExceptions(
   shouldPauseOnExceptions: boolean,
   shouldPauseOnCaughtExceptions: boolean
 ) {
   return ({ dispatch, getState, client }: ThunkArgs) => {
-    /* eslint-disable camelcase */
     recordEvent("pause_on_exceptions", {
       exceptions: shouldPauseOnExceptions,
       // There's no "n" in the key below (#1463117)
-      caught_exceptio: shouldPauseOnCaughtExceptions
+      ["caught_exceptio"]: shouldPauseOnCaughtExceptions
     });
-    /* eslint-enable camelcase */
 
-    const thread = getCurrentThread(getState());
     return dispatch({
       type: "PAUSE_ON_EXCEPTIONS",
-      thread,
       shouldPauseOnExceptions,
       shouldPauseOnCaughtExceptions,
       [PROMISE]: client.pauseOnExceptions(
-        thread,
         shouldPauseOnExceptions,
         shouldPauseOnCaughtExceptions
       )
     });
   };
 }
--- a/devtools/client/debugger/new/src/actions/types/PauseAction.js
+++ b/devtools/client/debugger/new/src/actions/types/PauseAction.js
@@ -27,17 +27,16 @@ export type PauseAction =
       +why: Why,
       +scopes: Scope,
       +frames: Frame[],
       +selectedFrameId: string,
       +loadedObjects: LoadedObject[]
     |}
   | {|
       +type: "PAUSE_ON_EXCEPTIONS",
-      +thread: string,
       +shouldPauseOnExceptions: boolean,
       +shouldPauseOnCaughtExceptions: boolean
     |}
   | PromiseAction<{|
       +type: "COMMAND",
       +thread: string,
       +command: Command
     |}>
--- a/devtools/client/debugger/new/src/client/firefox/commands.js
+++ b/devtools/client/debugger/new/src/client/firefox/commands.js
@@ -304,27 +304,33 @@ async function getFrameScopes(frame: Fra
   if (frame.scope) {
     return frame.scope;
   }
 
   const sourceThreadClient = lookupThreadClient(frame.thread);
   return sourceThreadClient.getEnvironment(frame.id);
 }
 
-function pauseOnExceptions(
-  thread: string,
+async function pauseOnExceptions(
   shouldPauseOnExceptions: boolean,
   shouldPauseOnCaughtExceptions: boolean
 ): Promise<*> {
-  return lookupThreadClient(thread).pauseOnExceptions(
+  await threadClient.pauseOnExceptions(
     shouldPauseOnExceptions,
     // Providing opposite value because server
     // uses "shouldIgnoreCaughtExceptions"
     !shouldPauseOnCaughtExceptions
   );
+
+  await forEachWorkerThread(thread =>
+    thread.pauseOnExceptions(
+      shouldPauseOnExceptions,
+      !shouldPauseOnCaughtExceptions
+    )
+  );
 }
 
 async function blackBox(
   sourceActor: SourceActor,
   isBlackBoxed: boolean,
   range?: Range
 ): Promise<*> {
   const sourceClient = threadClient.source({ actor: sourceActor.actor });
--- a/devtools/client/debugger/new/src/reducers/pause.js
+++ b/devtools/client/debugger/new/src/reducers/pause.js
@@ -60,39 +60,41 @@ type ThreadPauseState = {
     mappings: {
       [FrameId]: {
         [string]: string | null
       }
     }
   },
   selectedFrameId: ?string,
   loadedObjects: Object,
-  shouldPauseOnExceptions: boolean,
-  shouldPauseOnCaughtExceptions: boolean,
   command: Command,
   lastCommand: Command,
   wasStepping: boolean,
   previousLocation: ?MappedLocation
 };
 
 // Pause state describing all threads.
 export type PauseState = {
   currentThread: string,
   canRewind: boolean,
   threads: { [string]: ThreadPauseState },
   skipPausing: boolean,
-  mapScopes: boolean
+  mapScopes: boolean,
+  shouldPauseOnExceptions: boolean,
+  shouldPauseOnCaughtExceptions: boolean
 };
 
 export const createPauseState = (): PauseState => ({
   currentThread: "UnknownThread",
   threads: {},
   canRewind: false,
   skipPausing: prefs.skipPausing,
-  mapScopes: prefs.mapScopes
+  mapScopes: prefs.mapScopes,
+  shouldPauseOnExceptions: prefs.pauseOnExceptions,
+  shouldPauseOnCaughtExceptions: prefs.pauseOnCaughtExceptions
 });
 
 const resumedPauseState = {
   frames: null,
   frameScopes: {
     generated: {},
     original: {},
     mappings: {}
@@ -100,18 +102,16 @@ const resumedPauseState = {
   selectedFrameId: null,
   loadedObjects: {},
   why: null
 };
 
 const createInitialPauseState = () => ({
   ...resumedPauseState,
   isWaitingOnBreak: false,
-  shouldPauseOnExceptions: prefs.pauseOnExceptions,
-  shouldPauseOnCaughtExceptions: prefs.pauseOnCaughtExceptions,
   canRewind: false,
   command: null,
   lastCommand: null,
   previousLocation: null
 });
 
 function getThreadPauseState(state: PauseState, thread: string) {
   // Thread state is lazily initialized so that we don't have to keep track of
@@ -253,20 +253,21 @@ function update(
       const { shouldPauseOnExceptions, shouldPauseOnCaughtExceptions } = action;
 
       prefs.pauseOnExceptions = shouldPauseOnExceptions;
       prefs.pauseOnCaughtExceptions = shouldPauseOnCaughtExceptions;
 
       // Preserving for the old debugger
       prefs.ignoreCaughtExceptions = !shouldPauseOnCaughtExceptions;
 
-      return updateThreadState({
+      return {
+        ...state,
         shouldPauseOnExceptions,
         shouldPauseOnCaughtExceptions
-      });
+      };
     }
 
     case "COMMAND":
       if (action.status === "start") {
         return updateThreadState({
           ...resumedPauseState,
           command: action.command,
           lastCommand: action.command,
@@ -403,21 +404,21 @@ export function getPopupObjectProperties
   return getAllPopupObjectProperties(state)[objectId];
 }
 
 export function getIsWaitingOnBreak(state: OuterState) {
   return getCurrentPauseState(state).isWaitingOnBreak;
 }
 
 export function getShouldPauseOnExceptions(state: OuterState) {
-  return getCurrentPauseState(state).shouldPauseOnExceptions;
+  return state.pause.shouldPauseOnExceptions;
 }
 
 export function getShouldPauseOnCaughtExceptions(state: OuterState) {
-  return getCurrentPauseState(state).shouldPauseOnCaughtExceptions;
+  return state.pause.shouldPauseOnCaughtExceptions;
 }
 
 export function getCanRewind(state: OuterState) {
   return state.pause.canRewind;
 }
 
 export function getFrames(state: OuterState) {
   return getCurrentPauseState(state).frames;
--- a/devtools/client/debugger/new/test/mochitest/helpers.js
+++ b/devtools/client/debugger/new/test/mochitest/helpers.js
@@ -926,26 +926,20 @@ function removeBreakpoint(dbg, sourceId,
  * @return {Promise}
  * @static
  */
 async function togglePauseOnExceptions(
   dbg,
   pauseOnExceptions,
   pauseOnCaughtExceptions
 ) {
-  const command = dbg.actions.pauseOnExceptions(
+  return dbg.actions.pauseOnExceptions(
     pauseOnExceptions,
     pauseOnCaughtExceptions
   );
-
-  if (!isPaused(dbg)) {
-    await waitForThreadEvents(dbg, "resumed");
-  }
-
-  return command;
 }
 
 function waitForActive(dbg) {
   return waitForState(dbg, state => !dbg.selectors.isPaused(state), "active");
 }
 
 // Helpers
 
--- a/devtools/client/framework/attach-thread.js
+++ b/devtools/client/framework/attach-thread.js
@@ -34,31 +34,27 @@ function handleThreadState(toolbox, even
   }
 }
 
 async function attachThread(toolbox) {
   const target = toolbox.target;
   const threadOptions = {
     autoBlackBox: false,
     ignoreFrameEnvironment: true,
+    pauseOnExceptions:
+      Services.prefs.getBoolPref("devtools.debugger.pause-on-exceptions"),
+    ignoreCaughtExceptions:
+      Services.prefs.getBoolPref("devtools.debugger.ignore-caught-exceptions"),
   };
+
   const [, threadClient] = await target.attachThread(threadOptions);
   if (!threadClient.paused) {
     throw new Error("Thread in wrong state when starting up, should be paused.");
   }
 
-  // These flags need to be set here because the client sends them
-  // with the `resume` request. We make sure to do this before
-  // resuming to avoid another interrupt. We can't pass it in with
-  // `threadOptions` because the resume request will override them.
-  threadClient.pauseOnExceptions(
-    Services.prefs.getBoolPref("devtools.debugger.pause-on-exceptions"),
-    Services.prefs.getBoolPref("devtools.debugger.ignore-caught-exceptions")
-  );
-
   try {
     await threadClient.resume();
   } catch (ex) {
     // Interpret a possible error thrown by ThreadActor.resume
     if (ex.error === "wrongOrder") {
       const box = toolbox.getNotificationBox();
       box.appendNotification(
         L10N.getStr("toolbox.resumeOrderWarning"),
--- a/devtools/server/actors/thread.js
+++ b/devtools/server/actors/thread.js
@@ -934,21 +934,17 @@ const ThreadActor = ActorClassWithSpec(t
     if (request && request.resumeLimit) {
       resumeLimitHandled = this._handleResumeLimit(request);
     } else {
       this._clearSteppingHooks();
       resumeLimitHandled = Promise.resolve(true);
     }
 
     return resumeLimitHandled.then(() => {
-      if (request) {
-        this._options.pauseOnExceptions = request.pauseOnExceptions;
-        this._options.ignoreCaughtExceptions = request.ignoreCaughtExceptions;
-        this.maybePauseOnExceptions();
-      }
+      this.maybePauseOnExceptions();
 
       // When replaying execution in a separate process we need to explicitly
       // notify that process when to resume execution.
       if (this.dbg.replaying) {
         if (request && request.resumeLimit && request.resumeLimit.type == "warp") {
           this.dbg.replayTimeWarp(request.resumeLimit.target);
         } else if (rewinding) {
           this.dbg.replayResumeBackward();
@@ -1538,16 +1534,22 @@ const ThreadActor = ActorClassWithSpec(t
     return this._pauseAndRespond(frame, { type: "debuggerStatement" });
   },
 
   onSkipBreakpoints: function({ skip }) {
     this.skipBreakpoints = skip;
     return { skip };
   },
 
+  onPauseOnExceptions: function({pauseOnExceptions, ignoreCaughtExceptions}) {
+    Object.assign(this._options, { pauseOnExceptions, ignoreCaughtExceptions });
+    this.maybePauseOnExceptions();
+    return {};
+  },
+
   /*
    * A function that the engine calls when a recording/replaying process has
    * changed its position: a checkpoint was reached or a switch between a
    * recording and replaying child process occurred.
    */
   replayingOnPositionChange: function(sendProgress) {
     const recording = this.dbg.replayIsRecording();
     const executionPoint = this.dbg.replayCurrentExecutionPoint();
@@ -1742,16 +1744,17 @@ Object.assign(ThreadActor.prototype.requ
   "reconfigure": ThreadActor.prototype.onReconfigure,
   "resume": ThreadActor.prototype.onResume,
   "clientEvaluate": ThreadActor.prototype.onClientEvaluate,
   "frames": ThreadActor.prototype.onFrames,
   "interrupt": ThreadActor.prototype.onInterrupt,
   "sources": ThreadActor.prototype.onSources,
   "threadGrips": ThreadActor.prototype.onThreadGrips,
   "skipBreakpoints": ThreadActor.prototype.onSkipBreakpoints,
+  "pauseOnExceptions": ThreadActor.prototype.onPauseOnExceptions,
   "dumpThread": ThreadActor.prototype.onDump,
 });
 
 exports.ThreadActor = ThreadActor;
 
 /**
  * Creates a PauseActor.
  *
--- a/devtools/shared/client/thread-client.js
+++ b/devtools/shared/client/thread-client.js
@@ -1,28 +1,24 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* 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";
 
-const promise = require("devtools/shared/deprecated-sync-thenables");
-
 const {arg, DebuggerClient} = require("devtools/shared/client/debugger-client");
 const eventSource = require("devtools/shared/client/event-source");
 const {ThreadStateTypes} = require("devtools/shared/client/constants");
 
 loader.lazyRequireGetter(this, "ArrayBufferClient", "devtools/shared/client/array-buffer-client");
 loader.lazyRequireGetter(this, "LongStringClient", "devtools/shared/client/long-string-client");
 loader.lazyRequireGetter(this, "ObjectClient", "devtools/shared/client/object-client");
 loader.lazyRequireGetter(this, "SourceClient", "devtools/shared/client/source-client");
 
-const noop = () => {};
-
 /**
  * Creates a thread client for the remote debugging protocol server. This client
  * is a front to the thread actor created in the server side, hiding the
  * protocol details in a traditional JavaScript API.
  *
  * @param client DebuggerClient
  * @param actor string
  *        The actor ID for this thread.
@@ -39,20 +35,18 @@ ThreadClient.prototype = {
   _state: "paused",
   get state() {
     return this._state;
   },
   get paused() {
     return this._state === "paused";
   },
 
-  _pauseOnExceptions: false,
-  _ignoreCaughtExceptions: false,
+  _actor: null,
 
-  _actor: null,
   get actor() {
     return this._actor;
   },
 
   get _transport() {
     return this.client._transport;
   },
 
@@ -85,22 +79,16 @@ ThreadClient.prototype = {
     before: function(packet) {
       this._assertPaused("resume");
 
       // Put the client in a tentative "resuming" state so we can prevent
       // further requests that should only be sent in the paused state.
       this._previousState = this._state;
       this._state = "resuming";
 
-      if (this._pauseOnExceptions) {
-        packet.pauseOnExceptions = this._pauseOnExceptions;
-      }
-      if (this._ignoreCaughtExceptions) {
-        packet.ignoreCaughtExceptions = this._ignoreCaughtExceptions;
-      }
       return packet;
     },
     after: function(response) {
       if (response.error && this._state == "resuming") {
         // There was an error resuming, update the state to the new one
         // reported by the server, if given (only on wrongState), otherwise
         // reset back to the previous state.
         if (response.state) {
@@ -269,37 +257,21 @@ ThreadClient.prototype = {
    *
    * @param boolean pauseOnExceptions
    *        Enables pausing if true, disables otherwise.
    * @param boolean ignoreCaughtExceptions
    *        Whether to ignore caught exceptions
    * @param function onResponse
    *        Called with the response packet.
    */
-  pauseOnExceptions: function(pauseOnExceptions,
-                               ignoreCaughtExceptions,
-                               onResponse = noop) {
-    this._pauseOnExceptions = pauseOnExceptions;
-    this._ignoreCaughtExceptions = ignoreCaughtExceptions;
-
-    // Otherwise send the flag using a standard resume request.
-    if (!this.paused) {
-      return this.interrupt(response => {
-        if (response.error) {
-          // Can't continue if pausing failed.
-          onResponse(response);
-          return response;
-        }
-        return this.resume(onResponse);
-      });
-    }
-
-    onResponse();
-    return promise.resolve();
-  },
+  pauseOnExceptions: DebuggerClient.requester({
+    type: "pauseOnExceptions",
+    pauseOnExceptions: arg(0),
+    ignoreCaughtExceptions: arg(1),
+  }),
 
   /**
    * Send a clientEvaluate packet to the debuggee. Response
    * will be a resume packet.
    *
    * @param string frame
    *        The actor ID of the frame where the evaluation should take place.
    * @param string expression
--- a/devtools/shared/specs/script.js
+++ b/devtools/shared/specs/script.js
@@ -57,12 +57,18 @@ const threadSpec = generateActorSpec({
         ids: RetVal("array:string"),
       },
     },
     setActiveEventBreakpoints: {
       request: {
         ids: Arg(0, "array:string"),
       },
     },
+    pauseOnExceptions: {
+      request: {
+        pauseOnExceptions: Arg(0, "string"),
+        ignoreCaughtExceptions: Arg(1, "string"),
+      },
+    },
   },
 });
 
 exports.threadSpec = threadSpec;