From 673896a47f8524dea5014111f87e3e5e074e3b2d Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 16 Aug 2013 14:59:04 -0700 Subject: [PATCH] Bug 901712 - black boxing doesn't work with source maps; r=dcamp --- toolkit/devtools/server/actors/script.js | 312 +++++++++++++++--- .../devtools/server/tests/unit/head_dbg.js | 7 + .../server/tests/unit/test_blackboxing-06.js | 104 ++++++ .../server/tests/unit/test_nesting-01.js | 38 +++ .../server/tests/unit/test_nesting-02.js | 68 ++++ .../server/tests/unit/test_sourcemaps-02.js | 2 - .../devtools/server/tests/unit/xpcshell.ini | 3 + 7 files changed, 478 insertions(+), 56 deletions(-) create mode 100644 toolkit/devtools/server/tests/unit/test_blackboxing-06.js create mode 100644 toolkit/devtools/server/tests/unit/test_nesting-01.js create mode 100644 toolkit/devtools/server/tests/unit/test_nesting-02.js diff --git a/toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js index 29c4757d01c..0148597024b 100644 --- a/toolkit/devtools/server/actors/script.js +++ b/toolkit/devtools/server/actors/script.js @@ -131,9 +131,7 @@ BreakpointStore.prototype = { }, /** - * Checks if the breakpoint store has a requested breakpoint - * Returns the stored breakpoint if it exists - * null otherwise + * Checks if the breakpoint store has a requested breakpoint. * * @param Object aLocation * The location of the breakpoint you are retrieving. It is an object @@ -141,6 +139,7 @@ BreakpointStore.prototype = { * - url * - line * - column (optional) + * @returns The stored breakpoint if it exists, null otherwise. */ hasBreakpoint: function BS_hasBreakpoint(aLocation) { let { url, line, column } = aLocation; @@ -179,7 +178,7 @@ BreakpointStore.prototype = { for (let url of this._iterUrls(aSearchParams.url)) { for (let line of this._iterLines(url, aSearchParams.line)) { // Always yield whole line breakpoints first. See comment in - // |BreakpointStore.prototype.getBreakpoint|. + // |BreakpointStore.prototype.hasBreakpoint|. if (aSearchParams.column == null && this._wholeLineBreakpoints[url] && this._wholeLineBreakpoints[url][line]) { @@ -255,6 +254,136 @@ BreakpointStore.prototype = { }, }; +/** + * Manages pushing event loops and automatically pops and exits them in the + * correct order as they are resolved. + * + * @param nsIJSInspector inspector + * The underlying JS inspector we use to enter and exit nested event + * loops. + * @param Object hooks + * An object with the following properties: + * - url: The URL string of the debuggee we are spinning an event loop + * for. + * - preNest: function called before entering a nested event loop + * - postNest: function called after exiting a nested event loop + * @param ThreadActor thread + * The thread actor instance that owns this EventLoopStack. + */ +function EventLoopStack({ inspector, thread, hooks }) { + this._inspector = inspector; + this._hooks = hooks; + this._thread = thread; +} + +EventLoopStack.prototype = { + /** + * The number of nested event loops on the stack. + */ + get size() { + return this._inspector.eventLoopNestLevel; + }, + + /** + * The URL of the debuggee who pushed the event loop on top of the stack. + */ + get lastPausedUrl() { + return this.size > 0 + ? this._inspector.lastNestRequestor.url + : null; + }, + + /** + * Push a new nested event loop onto the stack. + * + * @returns EventLoop + */ + push: function () { + return new EventLoop({ + inspector: this._inspector, + thread: this._thread, + hooks: this._hooks + }); + } +}; + +/** + * An object that represents a nested event loop. It is used as the nest + * requestor with nsIJSInspector instances. + * + * @param nsIJSInspector inspector + * The JS Inspector that runs nested event loops. + * @param ThreadActor thread + * The thread actor that is creating this nested event loop. + * @param Object hooks + * The same hooks object passed into EventLoopStack during its + * initialization. + */ +function EventLoop({ inspector, thread, hooks }) { + this._inspector = inspector; + this._thread = thread; + this._hooks = hooks; + + this.enter = this.enter.bind(this); + this.resolve = this.resolve.bind(this); +} + +EventLoop.prototype = { + entered: false, + resolved: false, + get url() { return this._hooks.url; }, + + /** + * Enter this nested event loop. + */ + enter: function () { + let nestData = this._hooks.preNest + ? this._hooks.preNest() + : null; + + this.entered = true; + this._inspector.enterNestedEventLoop(this); + + // Keep exiting nested event loops while the last requestor is resolved. + if (this._inspector.eventLoopNestLevel > 0) { + const { resolved } = this._inspector.lastNestRequestor; + if (resolved) { + this._inspector.exitNestedEventLoop(); + } + } + + dbg_assert(this._thread.state === "running", + "Should be in the running state"); + + if (this._hooks.postNest) { + this._hooks.postNest(nestData); + } + }, + + /** + * Resolve this nested event loop. + * + * @returns boolean + * True if we exited this nested event loop because it was on top of + * the stack, false if there is another nested event loop above this + * one that hasn't resolved yet. + */ + resolve: function () { + if (!this.entered) { + throw new Error("Can't resolve an event loop before it has been entered!"); + } + if (this.resolved) { + throw new Error("Already resolved this nested event loop!"); + } + this.resolved = true; + if (this === this._inspector.lastNestRequestor) { + this._inspector.exitNestedEventLoop(); + return true; + } + return false; + }, +}; + /** * JSD2 actors. */ @@ -280,6 +409,11 @@ function ThreadActor(aHooks, aGlobal) this._environmentActors = []; this._hooks = aHooks; this.global = aGlobal; + this._nestedEventLoops = new EventLoopStack({ + inspector: DebuggerServer.xpcInspector, + hooks: aHooks, + thread: this + }); // A map of actorID -> actor for breakpoints created and managed by the server. this._hiddenBreakpoints = new Map(); @@ -326,6 +460,27 @@ ThreadActor.prototype = { return this._sources; }, + /** + * Keep track of all of the nested event loops we use to pause the debuggee + * when we hit a breakpoint/debugger statement/etc in one place so we can + * resolve them when we get resume packets. We have more than one (and keep + * them in a stack) because we can pause within client evals. + */ + _threadPauseEventLoops: null, + _pushThreadPause: function TA__pushThreadPause() { + if (!this._threadPauseEventLoops) { + this._threadPauseEventLoops = []; + } + const eventLoop = this._nestedEventLoops.push(); + this._threadPauseEventLoops.push(eventLoop); + eventLoop.enter(); + }, + _popThreadPause: function TA__popThreadPause() { + const eventLoop = this._threadPauseEventLoops.pop(); + dbg_assert(eventLoop, "Should have an event loop."); + eventLoop.resolve(); + }, + clearDebuggees: function TA_clearDebuggees() { if (this.dbg) { this.dbg.removeAllDebuggees(); @@ -485,7 +640,7 @@ ThreadActor.prototype = { this.conn.send(packet); // Start a nested event loop. - this._nest(); + this._pushThreadPause(); // We already sent a response to this request, don't send one // now. @@ -529,7 +684,7 @@ ThreadActor.prototype = { * promise. */ _pauseAndRespond: function TA__pauseAndRespond(aFrame, aReason, - onPacket=function (k) k) { + onPacket=function (k) { return k; }) { try { let packet = this._paused(aFrame); if (!packet) { @@ -548,14 +703,17 @@ ThreadActor.prototype = { message: error.message + "\n" + error.stack }; }) - .then(packet => this.conn.send(packet)); + .then(packet => { + this.conn.send(packet) + }); }); - return this._nest(); + this._pushThreadPause(); } catch(e) { reportError(e, "Got an exception during TA__pauseAndRespond: "); - return undefined; } + + return undefined; }, /** @@ -573,13 +731,13 @@ ThreadActor.prototype = { // In case of multiple nested event loops (due to multiple debuggers open in // different tabs or multiple debugger clients connected to the same tab) // only allow resumption in a LIFO order. - if (DebuggerServer.xpcInspector.eventLoopNestLevel > 1) { - let lastNestRequestor = DebuggerServer.xpcInspector.lastNestRequestor; - if (lastNestRequestor.connection != this.conn) { - return { error: "wrongOrder", - message: "trying to resume in the wrong order.", - lastPausedUrl: lastNestRequestor.url }; - } + if (this._nestedEventLoops.size + && this._nestedEventLoops.lastPausedUrl !== this._hooks.url) { + return { + error: "wrongOrder", + message: "trying to resume in the wrong order.", + lastPausedUrl: this._nestedEventLoops.lastPausedUrl + }; } if (aRequest && aRequest.forceCompletion) { @@ -592,7 +750,7 @@ ThreadActor.prototype = { this.dbg.getNewestFrame().pop(aRequest.completionValue); let packet = this._resumed(); - DebuggerServer.xpcInspector.exitNestedEventLoop(); + this._popThreadPause(); return { type: "resumeLimit", frameFinished: aRequest.forceCompletion }; } @@ -614,17 +772,27 @@ ThreadActor.prototype = { // Define the JS hook functions for stepping. let onEnterFrame = aFrame => { - if (this.sources.isBlackBoxed(aFrame.script.url)) { - return undefined; - } - return pauseAndRespond(aFrame); + let { url } = this.synchronize(this.sources.getOriginalLocation( + aFrame.script.url, + aFrame.script.getOffsetLine(aFrame.offset), + getOffsetColumn(aFrame.offset, aFrame.script))); + + return this.sources.isBlackBoxed(url) + ? undefined + : pauseAndRespond(aFrame); }; let thread = this; let onPop = function TA_onPop(aCompletion) { // onPop is called with 'this' set to the current frame. - if (thread.sources.isBlackBoxed(this.script.url)) { + + let { url } = thread.synchronize(thread.sources.getOriginalLocation( + this.script.url, + this.script.getOffsetLine(this.offset), + getOffsetColumn(this.offset, this.script))); + + if (thread.sources.isBlackBoxed(url)) { return undefined; } @@ -650,7 +818,12 @@ ThreadActor.prototype = { let onStep = function TA_onStep() { // onStep is called with 'this' set to the current frame. - if (thread.sources.isBlackBoxed(this.script.url)) { + let { url } = thread.synchronize(thread.sources.getOriginalLocation( + this.script.url, + this.script.getOffsetLine(this.offset), + getOffsetColumn(this.offset, this.script))); + + if (thread.sources.isBlackBoxed(url)) { return undefined; } @@ -712,10 +885,44 @@ ThreadActor.prototype = { } let packet = this._resumed(); - DebuggerServer.xpcInspector.exitNestedEventLoop(); + this._popThreadPause(); return packet; }, + /** + * Spin up a nested event loop so we can synchronously resolve a promise. + * + * @param aPromise + * The promise we want to resolve. + * @returns The promise's resolution. + */ + synchronize: function(aPromise) { + let needNest = true; + let eventLoop; + let returnVal; + + aPromise + .then((aResolvedVal) => { + needNest = false; + returnVal = aResolvedVal; + }) + .then(null, (aError) => { + reportError(aError, "Error inside synchronize:"); + }) + .then(() => { + if (eventLoop) { + eventLoop.resolve(); + } + }); + + if (needNest) { + eventLoop = this._nestedEventLoops.push(); + eventLoop.enter(); + } + + return returnVal; + }, + /** * Set the debugging hook to pause on exceptions if configured to do so. */ @@ -1278,7 +1485,7 @@ ThreadActor.prototype = { this.conn.send(packet); // Start a nested event loop. - this._nest(); + this._pushThreadPause(); // We already sent a response to this request, don't send one // now. @@ -1428,26 +1635,6 @@ ThreadActor.prototype = { return packet; }, - _nest: function TA_nest() { - if (this._hooks.preNest) { - var nestData = this._hooks.preNest(); - } - - let requestor = Object.create(null); - requestor.url = this._hooks.url; - requestor.connection = this.conn; - DebuggerServer.xpcInspector.enterNestedEventLoop(requestor); - - dbg_assert(this.state === "running", "Should be in the running state"); - - if (this._hooks.postNest) { - this._hooks.postNest(nestData) - } - - // "continue" resumption value. - return undefined; - }, - _resumed: function TA_resumed() { this._state = "running"; @@ -1753,10 +1940,14 @@ ThreadActor.prototype = { onDebuggerStatement: function TA_onDebuggerStatement(aFrame) { // Don't pause if we are currently stepping (in or over) or the frame is // black-boxed. - if (this.sources.isBlackBoxed(aFrame.script.url) || aFrame.onStep) { - return undefined; - } - return this._pauseAndRespond(aFrame, { type: "debuggerStatement" }); + let { url } = this.synchronize(this.sources.getOriginalLocation( + aFrame.script.url, + aFrame.script.getOffsetLine(aFrame.offset), + getOffsetColumn(aFrame.offset, aFrame.script))); + + return this.sources.isBlackBoxed(url) || aFrame.onStep + ? undefined + : this._pauseAndRespond(aFrame, { type: "debuggerStatement" }); }, /** @@ -1769,9 +1960,15 @@ ThreadActor.prototype = { * The exception that was thrown. */ onExceptionUnwind: function TA_onExceptionUnwind(aFrame, aValue) { - if (this.sources.isBlackBoxed(aFrame.script.url)) { + let { url } = this.synchronize(this.sources.getOriginalLocation( + aFrame.script.url, + aFrame.script.getOffsetLine(aFrame.offset), + getOffsetColumn(aFrame.offset, aFrame.script))); + + if (this.sources.isBlackBoxed(url)) { return undefined; } + try { let packet = this._paused(aFrame); if (!packet) { @@ -1781,11 +1978,13 @@ ThreadActor.prototype = { packet.why = { type: "exception", exception: this.createValueGrip(aValue) }; this.conn.send(packet); - return this._nest(); + + this._pushThreadPause(); } catch(e) { reportError(e, "Got an exception during TA_onExceptionUnwind: "); - return undefined; } + + return undefined; }, /** @@ -2781,8 +2980,13 @@ BreakpointActor.prototype = { hit: function BA_hit(aFrame) { // Don't pause if we are currently stepping (in or over) or the frame is // black-boxed. - if (this.threadActor.sources.isBlackBoxed(this.location.url) || - aFrame.onStep) { + let { url } = this.threadActor.synchronize( + this.threadActor.sources.getOriginalLocation( + this.location.url, + this.location.line, + this.location.column)); + + if (this.threadActor.sources.isBlackBoxed(url) || aFrame.onStep) { return undefined; } diff --git a/toolkit/devtools/server/tests/unit/head_dbg.js b/toolkit/devtools/server/tests/unit/head_dbg.js index 746a14f9ecc..4d95bb5576d 100644 --- a/toolkit/devtools/server/tests/unit/head_dbg.js +++ b/toolkit/devtools/server/tests/unit/head_dbg.js @@ -18,6 +18,7 @@ Services.prefs.setBoolPref("devtools.debugger.remote-enabled", true); Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); Cu.import("resource://gre/modules/devtools/Loader.jsm"); +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm"); function testExceptionHook(ex) { try { @@ -346,3 +347,9 @@ function StubTransport() { } StubTransport.prototype.ready = function () {}; StubTransport.prototype.send = function () {}; StubTransport.prototype.close = function () {}; + +function executeSoon(aFunc) { + Services.tm.mainThread.dispatch({ + run: DevToolsUtils.makeInfallible(aFunc) + }, Ci.nsIThread.DISPATCH_NORMAL); +} diff --git a/toolkit/devtools/server/tests/unit/test_blackboxing-06.js b/toolkit/devtools/server/tests/unit/test_blackboxing-06.js new file mode 100644 index 00000000000..009ec285e1e --- /dev/null +++ b/toolkit/devtools/server/tests/unit/test_blackboxing-06.js @@ -0,0 +1,104 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +/** + * Test that we can black box source mapped sources. + */ + +var gDebuggee; +var gClient; +var gThreadClient; + +Components.utils.import('resource:///modules/devtools/SourceMap.jsm'); + +const promise = devtools.require("sdk/core/promise"); + +function run_test() +{ + initTestDebuggerServer(); + gDebuggee = addTestGlobal("test-black-box"); + gClient = new DebuggerClient(DebuggerServer.connectPipe()); + gClient.connect(function() { + attachTestTabAndResume(gClient, "test-black-box", function(aResponse, aTabClient, aThreadClient) { + gThreadClient = aThreadClient; + + promise.resolve(setup_code()) + .then(black_box_code) + .then(run_code) + .then(test_correct_location) + .then(null, function (error) { + do_check_true(false, "Should not get an error, got " + error); + }) + .then(function () { + finishClient(gClient); + }); + }); + }); + do_test_pending(); +} + +function setup_code() { + let { code, map } = (new SourceNode(null, null, null, [ + new SourceNode(1, 0, "a.js", "" + function a() { + return b(); + }), + "\n", + new SourceNode(1, 0, "b.js", "" + function b() { + debugger; // Don't want to stop here. + return c(); + }), + "\n", + new SourceNode(1, 0, "c.js", "" + function c() { + debugger; // Want to stop here. + }), + "\n" + ])).toStringWithSourceMap({ + file: "abc.js", + sourceRoot: "http://example.com/" + }); + + code += "//# sourceMappingURL=data:text/json," + map.toString(); + + Components.utils.evalInSandbox(code, + gDebuggee, + "1.8", + "http://example.com/abc.js"); +} + +function black_box_code() { + const d = promise.defer(); + + gThreadClient.getSources(function ({ sources, error }) { + do_check_true(!error, "Shouldn't get an error getting sources"); + const source = sources.filter((s) => { + return s.url.indexOf("b.js") !== -1; + })[0]; + do_check_true(!!source, "We should have our source in the sources list"); + + gThreadClient.source(source).blackBox(function ({ error }) { + do_check_true(!error, "Should not get an error black boxing"); + d.resolve(true); + }); + }); + + return d.promise; +} + +function run_code() { + const d = promise.defer(); + + gClient.addOneTimeListener("paused", function (aEvent, aPacket) { + d.resolve(aPacket); + gThreadClient.resume(); + }); + gDebuggee.a(); + + return d.promise; +} + +function test_correct_location(aPacket) { + do_check_eq(aPacket.why.type, "debuggerStatement", + "Should hit a debugger statement."); + do_check_eq(aPacket.frame.where.url, "http://example.com/c.js", + "Should have skipped over the debugger statement in the black boxed source"); +} diff --git a/toolkit/devtools/server/tests/unit/test_nesting-01.js b/toolkit/devtools/server/tests/unit/test_nesting-01.js new file mode 100644 index 00000000000..bae0edf015d --- /dev/null +++ b/toolkit/devtools/server/tests/unit/test_nesting-01.js @@ -0,0 +1,38 @@ +/* -*- Mode: javascript; js-indent-level: 2; -*- */ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Test that we can nest event loops when needed in +// ThreadActor.prototype.synchronize. + +const { defer } = devtools.require("sdk/core/promise"); + +function run_test() { + initTestDebuggerServer(); + do_test_pending(); + test_nesting(); +} + +function test_nesting() { + const thread = new DebuggerServer.ThreadActor(DebuggerServer); + const { resolve, reject, promise } = defer(); + + let currentStep = 0; + + executeSoon(function () { + // Should be on the first step + do_check_eq(++currentStep, 1); + // We should have one nested event loop from synchronize + do_check_eq(thread._nestedEventLoops.size, 1); + resolve(true); + }); + + do_check_eq(thread.synchronize(promise), true); + + // Should be on the second step + do_check_eq(++currentStep, 2); + // There shouldn't be any nested event loops anymore + do_check_eq(thread._nestedEventLoops.size, 0); + + do_test_finished(); +} diff --git a/toolkit/devtools/server/tests/unit/test_nesting-02.js b/toolkit/devtools/server/tests/unit/test_nesting-02.js new file mode 100644 index 00000000000..ce1f9aa64da --- /dev/null +++ b/toolkit/devtools/server/tests/unit/test_nesting-02.js @@ -0,0 +1,68 @@ +/* -*- Mode: javascript; js-indent-level: 2; -*- */ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Test that we can nest event loops and then automatically exit nested event +// loops when requested. + +const { defer } = devtools.require("sdk/core/promise"); + +function run_test() { + initTestDebuggerServer(); + do_test_pending(); + test_nesting(); +} + +function test_nesting() { + const thread = new DebuggerServer.ThreadActor(DebuggerServer); + const { resolve, reject, promise } = defer(); + + // The following things should happen (in order): + // 1. In the new event loop (created by synchronize) + // 2. Resolve the promise (shouldn't exit any event loops) + // 3. Exit the event loop (should also then exit synchronize's event loop) + // 4. Be after the synchronize call + let currentStep = 0; + + executeSoon(function () { + let eventLoop; + + executeSoon(function () { + // Should be at step 2 + do_check_eq(++currentStep, 2); + // Before resolving, should have the synchronize event loop and the one just created. + do_check_eq(thread._nestedEventLoops.size, 2); + + executeSoon(function () { + // Should be at step 3 + do_check_eq(++currentStep, 3); + // Before exiting the manually created event loop, should have the + // synchronize event loop and the manual event loop. + do_check_eq(thread._nestedEventLoops.size, 2); + // Should have the event loop + do_check_true(!!eventLoop); + eventLoop.resolve(); + }); + + resolve(true); + // Shouldn't exit any event loops because a new one started since the call to synchronize + do_check_eq(thread._nestedEventLoops.size, 2); + }); + + // Should be at step 1 + do_check_eq(++currentStep, 1); + // Should have only the synchronize event loop + do_check_eq(thread._nestedEventLoops.size, 1); + eventLoop = thread._nestedEventLoops.push(); + eventLoop.enter(); + }); + + do_check_eq(thread.synchronize(promise), true); + + // Should be on the fourth step + do_check_eq(++currentStep, 4); + // There shouldn't be any nested event loops anymore + do_check_eq(thread._nestedEventLoops.size, 0); + + do_test_finished(); +} diff --git a/toolkit/devtools/server/tests/unit/test_sourcemaps-02.js b/toolkit/devtools/server/tests/unit/test_sourcemaps-02.js index 76e5a170ee6..6d63c9a2f26 100644 --- a/toolkit/devtools/server/tests/unit/test_sourcemaps-02.js +++ b/toolkit/devtools/server/tests/unit/test_sourcemaps-02.js @@ -33,8 +33,6 @@ function test_simple_source_map() "http://example.com/www/js/b.js", "http://example.com/www/js/c.js"]); - let numNewSources = 0; - gClient.addOneTimeListener("paused", function (aEvent, aPacket) { gThreadClient.getSources(function (aResponse) { do_check_true(!aResponse.error, "Should not get an error"); diff --git a/toolkit/devtools/server/tests/unit/xpcshell.ini b/toolkit/devtools/server/tests/unit/xpcshell.ini index 1d997db9ad8..9c96bb55b1e 100644 --- a/toolkit/devtools/server/tests/unit/xpcshell.ini +++ b/toolkit/devtools/server/tests/unit/xpcshell.ini @@ -2,6 +2,8 @@ head = head_dbg.js tail = +[test_nesting-01.js] +[test_nesting-02.js] [test_forwardingprefix.js] [test_getyoungestframe.js] [test_nsjsinspector.js] @@ -18,6 +20,7 @@ reason = bug 821285 [test_blackboxing-03.js] [test_blackboxing-04.js] [test_blackboxing-05.js] +[test_blackboxing-06.js] [test_frameactor-01.js] [test_frameactor-02.js] [test_frameactor-03.js]