Closed Bug 949754 Opened 6 years ago Closed 5 years ago

Tracer actors should use source maps

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: fitzgen, Assigned: jjurovych)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → nfitzgerald
still assigned, marking as such.
Status: NEW → ASSIGNED
and p3.
Priority: -- → P3
Assignee: nfitzgerald → nobody
Assignee: nobody → jjurovych
Attached patch bug-949754.patch (obsolete) — Splinter Review
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.
Depends on: 1052855
No longer depends on: 929349
Attached patch bug-949754-2.patch (obsolete) — Splinter Review
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.
Attachment #8473242 - Attachment is obsolete: true
Attachment #8476251 - Flags: review?(nfitzgerald)
Attached patch bug-949754-3.patch (obsolete) — Splinter Review
Fixing tests.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa3d754882a7
Attachment #8476251 - Attachment is obsolete: true
Attachment #8476251 - Flags: review?(nfitzgerald)
Attachment #8476345 - Flags: review?(nfitzgerald)
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?
Attachment #8476345 - Flags: review?(nfitzgerald)
Attached patch bug-949754-4.patch (obsolete) — Splinter Review
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. :/
Attachment #8476345 - Attachment is obsolete: true
Attachment #8479509 - Flags: review?(nfitzgerald)
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).
Attachment #8485452 - Flags: review?(nfitzgerald)
Comment on attachment 8485452 [details] [diff] [review]
bug-949754-5.patch

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

Looks good, thanks!
Attachment #8485452 - Flags: review?(nfitzgerald) → review+
Attachment #8479509 - Attachment is obsolete: true
Attachment #8479509 - Flags: review?(nfitzgerald)
Attachment #8485452 - Flags: checkin?(nfitzgerald)
Attachment #8485452 - Flags: checkin?(nfitzgerald)
Woops meant to replace the checkin? flag with checkin-needed previously.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9736477e18de
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.