From 14e008b382cd0e8db84c8646faec010aa5766e51 Mon Sep 17 00:00:00 2001 From: Jakub Jurovych Date: Wed, 16 Jul 2014 14:56:01 -0700 Subject: [PATCH] Bug 897567 - JS debugger: setting breakpoints can be confused by script GC. r=fitzgen --- toolkit/devtools/client/dbg-client.jsm | 7 +- toolkit/devtools/server/actors/script.js | 32 +++--- .../devtools/server/tests/unit/head_dbg.js | 11 +++ .../server/tests/unit/test_breakpoint-15.js | 97 ++++++++++--------- .../server/tests/unit/test_breakpoint-19.js | 69 +++++++++++++ .../server/tests/unit/test_breakpointstore.js | 26 +++++ .../devtools/server/tests/unit/xpcshell.ini | 1 + 7 files changed, 180 insertions(+), 63 deletions(-) create mode 100644 toolkit/devtools/server/tests/unit/test_breakpoint-19.js diff --git a/toolkit/devtools/client/dbg-client.jsm b/toolkit/devtools/client/dbg-client.jsm index 3bf6b94deff..4e195bc7782 100644 --- a/toolkit/devtools/client/dbg-client.jsm +++ b/toolkit/devtools/client/dbg-client.jsm @@ -1757,14 +1757,17 @@ ThreadClient.prototype = { this.client.request(packet, (aResponse) => { // Ignoring errors, since the user may be setting a breakpoint in a // dead script that will reappear on a page reload. - if (aOnResponse) { + let bpClient; + if (aResponse.actor) { let root = this.client.mainRoot; - let bpClient = new BreakpointClient( + bpClient = new BreakpointClient( this.client, aResponse.actor, location, root.traits.conditionalBreakpoints ? condition : undefined ); + } + if (aOnResponse) { aOnResponse(aResponse, bpClient); } if (aCallback) { diff --git a/toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js index 5f6c9e894cf..dcc944d2690 100644 --- a/toolkit/devtools/server/actors/script.js +++ b/toolkit/devtools/server/actors/script.js @@ -98,7 +98,7 @@ BreakpointStore.prototype = { get size() { return this._size; }, /** - * Add a breakpoint to the breakpoint store. + * Add a breakpoint to the breakpoint store if it doesn't already exist. * * @param Object aBreakpoint * The breakpoint to be added (not copied). It is an object with the @@ -109,10 +109,11 @@ BreakpointStore.prototype = { * the whole line) * - condition (optional) * - actor (optional) + * @returns Object aBreakpoint + * The new or existing breakpoint. */ addBreakpoint: function (aBreakpoint) { let { url, line, column } = aBreakpoint; - let updating = false; if (column != null) { if (!this._breakpoints[url]) { @@ -121,16 +122,22 @@ BreakpointStore.prototype = { if (!this._breakpoints[url][line]) { this._breakpoints[url][line] = []; } - this._breakpoints[url][line][column] = aBreakpoint; + if (!this._breakpoints[url][line][column]) { + this._breakpoints[url][line][column] = aBreakpoint; + this._size++; + } + return this._breakpoints[url][line][column]; } else { // Add a breakpoint that breaks on the whole line. if (!this._wholeLineBreakpoints[url]) { this._wholeLineBreakpoints[url] = []; } - this._wholeLineBreakpoints[url][line] = aBreakpoint; + if (!this._wholeLineBreakpoints[url][line]) { + this._wholeLineBreakpoints[url][line] = aBreakpoint; + this._size++; + } + return this._wholeLineBreakpoints[url][line]; } - - this._size++; }, /** @@ -1439,9 +1446,7 @@ ThreadActor.prototype = { let locationPromise = this.sources.getGeneratedLocation(aRequest.location); return locationPromise.then(({url, line, column}) => { - if (line == null || - line < 0 || - this.dbg.findScripts({ url: url }).length == 0) { + if (line == null || line < 0) { return { error: "noScript", message: "Requested setting a breakpoint on " @@ -1537,12 +1542,11 @@ ThreadActor.prototype = { // Find all scripts matching the given location let scripts = this.dbg.findScripts(aLocation); if (scripts.length == 0) { + // Since we did not find any scripts to set the breakpoint on now, return + // early. When a new script that matches this breakpoint location is + // introduced, the breakpoint actor will already be in the breakpoint store + // and will be set at that time. return { - error: "noScript", - message: "Requested setting a breakpoint on " - + aLocation.url + ":" + aLocation.line - + (aLocation.column != null ? ":" + aLocation.column : "") - + " but there is no Debugger.Script at that location", actor: actor.actorID }; } diff --git a/toolkit/devtools/server/tests/unit/head_dbg.js b/toolkit/devtools/server/tests/unit/head_dbg.js index a9474f65551..30b7ca531d2 100644 --- a/toolkit/devtools/server/tests/unit/head_dbg.js +++ b/toolkit/devtools/server/tests/unit/head_dbg.js @@ -510,6 +510,17 @@ function resume(threadClient) { return rdpRequest(threadClient, threadClient.resume); } +/** + * Interrupt JS execution for the specified thread. + * + * @param ThreadClient threadClient + * @returns Promise + */ +function interrupt(threadClient) { + dumpn("Interrupting."); + return rdpRequest(threadClient, threadClient.interrupt); +} + /** * Resume JS execution for the specified thread and then wait for the next pause * event. diff --git a/toolkit/devtools/server/tests/unit/test_breakpoint-15.js b/toolkit/devtools/server/tests/unit/test_breakpoint-15.js index c6ad0815b16..146a4dd6deb 100644 --- a/toolkit/devtools/server/tests/unit/test_breakpoint-15.js +++ b/toolkit/devtools/server/tests/unit/test_breakpoint-15.js @@ -8,62 +8,65 @@ var gDebuggee; var gClient; var gThreadClient; -var gCallback; function run_test() { - run_test_with_server(DebuggerServer, function () { - run_test_with_server(WorkerDebuggerServer, do_test_finished); - }); - do_test_pending(); -}; - -function run_test_with_server(aServer, aCallback) -{ - gCallback = aCallback; - initTestDebuggerServer(aServer); - gDebuggee = addTestGlobal("test-stack", aServer); - gClient = new DebuggerClient(aServer.connectPipe()); + initTestDebuggerServer(); + gDebuggee = addTestGlobal("test-stack"); + gClient = new DebuggerClient(DebuggerServer.connectPipe()); gClient.connect(function () { attachTestTabAndResume(gClient, "test-stack", function (aResponse, aTabClient, aThreadClient) { gThreadClient = aThreadClient; - test_same_breakpoint(); + testSameBreakpoint(); }); }); + do_test_pending(); } -function test_same_breakpoint() -{ - gThreadClient.addOneTimeListener("paused", function (aEvent, aPacket) { - let path = getFilePath('test_breakpoint-01.js'); - let location = { - url: path, - line: gDebuggee.line0 + 3 - }; - gThreadClient.setBreakpoint(location, function (aResponse, bpClient) { - gThreadClient.setBreakpoint(location, function (aResponse, secondBpClient) { - do_check_eq(bpClient.actor, secondBpClient.actor, - "Should get the same actor w/ whole line breakpoints"); - let location = { - url: path, - line: gDebuggee.line0 + 2, - column: 6 - }; - gThreadClient.setBreakpoint(location, function (aResponse, bpClient) { - gThreadClient.setBreakpoint(location, function (aResponse, secondBpClient) { - do_check_eq(bpClient.actor, secondBpClient.actor, - "Should get the same actor column breakpoints"); - gClient.close(gCallback); - }); - }); - }); - }); - }); +const SOURCE_URL = "http://example.com/source.js"; - Components.utils.evalInSandbox("var line0 = Error().lineNumber;\n" + - "debugger;\n" + // line0 + 1 - "var a = 1;\n" + // line0 + 2 - "var b = 2;\n", // line0 + 3 - gDebuggee); -} +const testSameBreakpoint = Task.async(function* () { + yield executeOnNextTickAndWaitForPause(evalCode, gClient); + + // Whole line + + let wholeLineLocation = { + url: SOURCE_URL, + line: 2 + }; + + let [firstResponse, firstBpClient] = yield setBreakpoint(gThreadClient, wholeLineLocation); + let [secondResponse, secondBpClient] = yield setBreakpoint(gThreadClient, wholeLineLocation); + + do_check_eq(firstBpClient.actor, secondBpClient.actor, "Should get the same actor w/ whole line breakpoints"); + + // Specific column + + let columnLocation = { + url: SOURCE_URL, + line: 2, + column: 6 + }; + + let [firstResponse, firstBpClient] = yield setBreakpoint(gThreadClient, columnLocation); + let [secondResponse, secondBpClient] = yield setBreakpoint(gThreadClient, columnLocation); + + do_check_eq(secondBpClient.actor, secondBpClient.actor, "Should get the same actor column breakpoints"); + + finishClient(gClient); +}); + +function evalCode() { + Components.utils.evalInSandbox( + "" + function doStuff(k) { // line 1 + let arg = 15; // line 2 - Step in here + k(arg); // line 3 + } + "\n" // line 4 + + "debugger;", // line 5 + gDebuggee, + "1.8", + SOURCE_URL, + 1 + ); +} \ No newline at end of file diff --git a/toolkit/devtools/server/tests/unit/test_breakpoint-19.js b/toolkit/devtools/server/tests/unit/test_breakpoint-19.js new file mode 100644 index 00000000000..f99305cae93 --- /dev/null +++ b/toolkit/devtools/server/tests/unit/test_breakpoint-19.js @@ -0,0 +1,69 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +/** + * Make sure that setting a breakpoint in a not-yet-existing script doesn't throw + * an error (see bug 897567). Also make sure that this breakpoint works. + */ + +var gDebuggee; +var gClient; +var gThreadClient; +var gCallback; + +function run_test() +{ + run_test_with_server(DebuggerServer, function () { + run_test_with_server(WorkerDebuggerServer, do_test_finished); + }); + do_test_pending(); +}; + +function run_test_with_server(aServer, aCallback) +{ + gCallback = aCallback; + initTestDebuggerServer(aServer); + gDebuggee = addTestGlobal("test-breakpoints", aServer); + gDebuggee.console = { log: x => void x }; + gClient = new DebuggerClient(aServer.connectPipe()); + gClient.connect(function () { + attachTestTabAndResume(gClient, + "test-breakpoints", + function (aResponse, aTabClient, aThreadClient) { + gThreadClient = aThreadClient; + testBreakpoint(); + }); + }); +} + +const URL = "test.js"; + +function setUpCode() { + Cu.evalInSandbox( + "" + function test() { // 1 + var a = 1; // 2 + debugger; // 3 + } + // 4 + "\ndebugger;", // 5 + gDebuggee, + "1.8", + URL + ); +} + +const testBreakpoint = Task.async(function* () { + const [response, bpClient] = yield setBreakpoint(gThreadClient, {url: URL, line: 2}); + ok(!response.error); + + const actor = response.actor; + ok(actor); + + yield executeOnNextTickAndWaitForPause(setUpCode, gClient); + yield resume(gThreadClient); + + const packet = yield executeOnNextTickAndWaitForPause(gDebuggee.test, gClient); + equal(packet.why.type, "breakpoint") + notEqual(packet.why.actors.indexOf(actor), -1); + + finishClient(gClient); +}); \ No newline at end of file diff --git a/toolkit/devtools/server/tests/unit/test_breakpointstore.js b/toolkit/devtools/server/tests/unit/test_breakpointstore.js index 2e80000cf3d..6c57e5f2d5b 100644 --- a/toolkit/devtools/server/tests/unit/test_breakpointstore.js +++ b/toolkit/devtools/server/tests/unit/test_breakpointstore.js @@ -16,6 +16,7 @@ function run_test() test_add_breakpoint(); test_remove_breakpoint(); test_find_breakpoints(); + test_duplicate_breakpoints(); } function test_has_breakpoint() { @@ -165,3 +166,28 @@ function test_find_breakpoints() { do_check_eq(bpSet.size, 0, "Should be able to filter the iteration by url and line"); } + +function test_duplicate_breakpoints() { + let bpStore = new BreakpointStore(); + + // Breakpoint with column + let location = { + url: "http://example.com/foo.js", + line: 10, + column: 9 + }; + bpStore.addBreakpoint(location); + bpStore.addBreakpoint(location); + do_check_eq(bpStore.size, 1, "We should have only 1 column breakpoint"); + bpStore.removeBreakpoint(location); + + // Breakpoint without column (whole line breakpoint) + location = { + url: "http://example.com/foo.js", + line: 15 + }; + bpStore.addBreakpoint(location); + bpStore.addBreakpoint(location); + do_check_eq(bpStore.size, 1, "We should have only 1 whole line breakpoint"); + bpStore.removeBreakpoint(location); +} diff --git a/toolkit/devtools/server/tests/unit/xpcshell.ini b/toolkit/devtools/server/tests/unit/xpcshell.ini index 1c7d9c3997f..3eee53d9cb0 100644 --- a/toolkit/devtools/server/tests/unit/xpcshell.ini +++ b/toolkit/devtools/server/tests/unit/xpcshell.ini @@ -110,6 +110,7 @@ reason = bug 820380 [test_breakpoint-16.js] [test_breakpoint-17.js] [test_breakpoint-18.js] +[test_breakpoint-19.js] [test_conditional_breakpoint-01.js] [test_conditional_breakpoint-02.js] [test_conditional_breakpoint-03.js]