949754 - Tracer actors should use source maps

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Rob Campbell [:rc] (:robcee)]

still assigned, marking as such.

Comment 2

[Rob Campbell [:rc] (:robcee)]

and p3.

Comment 3

[Jakub Jurovych]

Attachment: bug-949754.patch

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.

Comment 4

[Jakub Jurovych]

Attachment: bug-949754-2.patch

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.

Comment 5

[Jakub Jurovych]

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

Comment 6

[Jakub Jurovych]

Attachment: bug-949754-3.patch

Fixing tests.

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

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 8

[Jakub Jurovych]

Attachment: bug-949754-4.patch

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. :/

Comment 9

[jakub.jurovych]

Attachment: bug-949754-5.patch

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 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8485452 [details] [diff] [review]
bug-949754-5.patch

Review of attachment 8485452 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/9736477e18de

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9736477e18de