Bug 1532791 - Breaking on exception even if option is off. r=davidwalsh
authorJason Laster <jlaster@mozilla.com>
Tue, 12 Mar 2019 19:01:40 +0000
changeset 521586 a001f492200e62cf914374eebcde837f81f9e8e7
parent 521585 add98afa5f0ca1ca17aa87eaa045442a3e1fa07a
child 521587 e239591d8bf770d9af9275506d498d47342fb16b
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/server/tests/unit/test_blackboxing-05.js
devtools/server/tests/unit/test_pause_exceptions-01.js
devtools/server/tests/unit/test_pause_exceptions-03.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/server/tests/unit/test_blackboxing-05.js
+++ b/devtools/server/tests/unit/test_blackboxing-05.js
@@ -68,17 +68,17 @@ function test_black_box() {
   /* eslint-enable no-multi-spaces, no-unreachable, no-undef */
 }
 
 function test_black_box_exception() {
   gThreadClient.getSources(async function({error, sources}) {
     Assert.ok(!error, "Should not get an error: " + error);
     const sourceClient = await getSource(gThreadClient, BLACK_BOXED_URL);
     await blackBox(sourceClient);
-    gThreadClient.pauseOnExceptions(true);
+    gThreadClient.pauseOnExceptions(true, false);
 
     gClient.addOneTimeListener("paused", async function(event, packet) {
       const source = await getSourceById(gThreadClient, packet.frame.where.actor);
 
       Assert.equal(source.url, SOURCE_URL,
                    "We shouldn't pause while in the black boxed source.");
       finishClient(gClient);
     });
--- a/devtools/server/tests/unit/test_pause_exceptions-01.js
+++ b/devtools/server/tests/unit/test_pause_exceptions-01.js
@@ -36,17 +36,17 @@ function run_test() {
 function test_pause_frame() {
   gThreadClient.addOneTimeListener("paused", function(event, packet) {
     gThreadClient.addOneTimeListener("paused", function(event, packet) {
       Assert.equal(packet.why.type, "exception");
       Assert.equal(packet.why.exception, 42);
       gThreadClient.resume(() => finishClient(gClient));
     });
 
-    gThreadClient.pauseOnExceptions(true);
+    gThreadClient.pauseOnExceptions(true, false);
     gThreadClient.resume();
   });
 
   /* eslint-disable */
   gDebuggee.eval("(" + function () {
     function stopMe() {
       debugger;
       throw 42;
--- a/devtools/server/tests/unit/test_pause_exceptions-03.js
+++ b/devtools/server/tests/unit/test_pause_exceptions-03.js
@@ -7,17 +7,17 @@
 /**
  * Test that setting pauseOnExceptions to true will cause the debuggee to pause
  * when an exception is thrown.
  */
 
 add_task(threadClientTest(async ({ threadClient, client, debuggee }) => {
   await executeOnNextTickAndWaitForPause(() => evaluateTestCode(debuggee), client);
 
-  threadClient.pauseOnExceptions(true);
+  threadClient.pauseOnExceptions(true, false);
   await resume(threadClient);
   const paused = await waitForPause(client);
   Assert.equal(paused.why.type, "exception");
   equal(paused.frame.where.line, 4, "paused at throw");
 
   await resume(threadClient);
 }, {
   // Bug 1508289, exception tests fails in worker scope
--- 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;