The tracer actors should use source maps to translate line/col/source/name of each frame entrance and exit. Should probably share the data with the script actors so that we don't duplicate data. Perhaps the tracer actors should be moved into the script actors? Or perhaps we need a session wide source map repository for all of devtools.

still assigned, marking as such.

and p3.

I've tried to make onEnterFrame work with source map promises and send out packets in order. However, a test for checking the correct original location (test_sourcemaps-14.js) does not pass yet. Reported location is sometimes one line below function declaration, probably due to Bug 1052855.

This patch contains Bug 1052855. Due to Bug 901138, we know only function's line, not column. This works well enough most of the time, though.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb6a582f8484

Fixing tests. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa3d754882a7

Comment on attachment 8476345 [details] [diff] [review] bug-949754-3.patch Review of attachment 8476345 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I'd like to take another look after you've added a test interweaving source mapped and non-source mapped calls to exercise the scheduler as it is really used. ::: browser/devtools/debugger/test/browser_dbg_tracing-03.js @@ +55,5 @@ > function clickTraceLog() { > filterTraces(gPanel, t => t.querySelector(".trace-name[value=main]"))[0].click(); > } > function testCorrectLine() { > + is(gDebugger.DebuggerView.editor.getCursor().line, 18, Is this from the other patch involving startLine? ::: toolkit/devtools/server/actors/common.js @@ +163,5 @@ > this._cleanups = {}; > } > } > exports.ActorPool = ActorPool; > +// TODO bug 863089: use Debugger.Script.prototype.getOffsetColumn when it is Nit: newline after the exports, before this comment. ::: toolkit/devtools/server/actors/script.js @@ +5057,5 @@ > if (url in this._sourceMapsByGeneratedSource) { > column = column || 0; > return this._sourceMapsByGeneratedSource[url] > .then((aSourceMap) => { > + let { source: aSourceURL, line: aLine, column: aColumn, name: aName } = aSourceMap.originalPositionFor({ While you're here can you break this line down so it isn't so long? Something like let { source: aSourceURL, ... } = ... ::: toolkit/devtools/server/actors/tracer.js @@ +8,5 @@ > const Debugger = require("Debugger"); > +const { getOffsetColumn } = require("devtools/server/actors/common"); > + > +Cu.import("resource://gre/modules/Task.jsm"); > +Cu.import("resource://gre/modules/Promise.jsm"); Nit: please use const promise = require("promise"); instead of Cu.import. This helps make it easier for us to migrate to native promises when the time comes and we can just swap out what the loader returns as the promise module. @@ +261,5 @@ > * @param aFrame Debugger.Frame > * The stack frame that was entered. > */ > onEnterFrame: function(aFrame) { > + Task.async(function*() { FYI if you want to run a task immediately, you can use Task.spawn, but really I think we want to do: onEnterFrame: Task.async(...) @@ +297,5 @@ > + : "(" + aFrame.type + ")"; > + } > + } > + if (this._requestsForTraceType.location) { > + if (sourceMappedLocation) { The source map module and the ThreadSources wrappers have a funky API and don't return null if there is no mapping, but an object with all the properties set to null, eg { source: null, line: null, ... }. The result is that sourceMappedLocation should always be an object and you can just check for sourceMappedLocation.source here and sourceMappedLocation.name above. ::: toolkit/devtools/server/tests/unit/test_sourcemaps-14.js @@ +42,5 @@ > + > + yield startTrace(); > + > + yield executeOnNextTickAndWaitForPause(evalCode, gClient); > + yield resume(gThreadClient); This test doesn't need the pause for anything, so you could just eval it on this tick and there's no need to mess with pausing and resuming. ::: toolkit/devtools/server/tests/unit/test_trace_actor-05.js @@ +86,5 @@ > do_check_eq(traces[1].name, "foo"); > // XXX: foo's definition is at tracerlocations.js:3:0, but Debugger.Script > + // does not provide complete definition locations (bug 901138). |column| > + // is inferred from the offset of the first statement in the function. > + check_location(traces[1].location, { url: url, line: 3, column: 2 }); Is this change from the other startLine bug?

Yes, the previous patch required startLine bug fixed. Since it has landed in the mean time, this patch does no longer include the changes. I have added a new test to verify that source-mapped traces are in the correct order. My local tests (when run together) do not work again, so I only tried a few of them individually. And I also lost access to try. :/

Rebased and fixed. The evaled code in the test has to be paused, otherwise the |traces| event won't work (I don't know why).

Comment on attachment 8485452 [details] [diff] [review] bug-949754-5.patch Review of attachment 8485452 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!

Woops meant to replace the checkin? flag with checkin-needed previously.