1107541 - Fix eval scripts with a sourceURL pragma

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

We can debug eval scripts now from bug 905700, but how we show scripts with a sourceURL pragma is broken. The correct filename is displayed, but it still has the "> eval" suffix, and worse, the source contents can't be loaded.

This is easily fixed with a few tweaks to how we handle it. We should probably uplift this as well since the train moved up on Tuesday.

Comment 1

[James Long (:jlongster)]

Attachment: 1107541.patch

Comment 2

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

Comment on attachment 8532103 [details] [diff] [review]
1107541.patch

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

::: browser/devtools/debugger/test/browser_dbg_sources-eval-01.js
@@ +19,5 @@
> +    gBreakpoints = gDebugger.DebuggerController.Breakpoints;
> +
> +    waitForSourceShown(gPanel, "-eval.js")
> +      .then(run)
> +      .then(null, aError => {

Why mix using promises directly and using Task?

Why not spawn the task here and yield waitForSourceShown, etc? Would be cleaner, IMO.

Same comments for the other test file.

@@ +25,5 @@
> +      });
> +  });
> +
> +  function run() {
> +    return Task.spawn(function*() {

You can skip the function def + spawn and just do:

  const run = Task.async(function* () {
    ...

Same comments for the other test file.

::: browser/devtools/debugger/test/code_script-eval.js
@@ +4,5 @@
>  function evalSource() {
>    eval('bar = function() {\nvar x = 5;\n}');
>  }
> +
> +function evalSourceWithPragma() {

Nit: Which pragma? //# sourceMappingURL? (nope)

evalSourceWithDisplayURL?

Comment 4

[James Long (:jlongster)]

Attachment: 1107541.patch

Comment 5

[James Long (:jlongster)]

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

Comment 6

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

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

Comment 7

[Phil Ringnalda (:philor)]

e10s angry. e10s SMASH. https://treeherder.mozilla.org/logviewer.html#?job_id=1380316&repo=fx-team etc.

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b46dc702c59c

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

[Tracking Requested - why for this release]: Strange / broken eval behavior added in 36 as part of bug 905700

Comment 9

[James Long (:jlongster)]

Attachment: 1107541.patch

Doh! e10s issue should be fixed.

Comment 10

[James Long (:jlongster)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[John-David Dalton]

Firefox Nightly reports Error loading source: loadSourceError.
http://f.cl.ly/items/1l2t043U0w1u2K0n0P3P/ff_srcurl.png

When testing this in the console:
Function('return true;//# sourceURL=something/cool');

Comment 13

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

(In reply to John-David Dalton from comment #12)
> Firefox Nightly reports Error loading source: loadSourceError.
> http://f.cl.ly/items/1l2t043U0w1u2K0n0P3P/ff_srcurl.png
> 
> When testing this in the console:
> Function('return true;//# sourceURL=something/cool');

This fix has only just landed in fx-team, where it will incubate before being merged into mozilla-central. Tomorrow's Nightly should have the fix.

Comment 14

[James Long (:jlongster)]

@jdalton what Nick said. Love that you commented here though. Please continue to point out places where it's broken!

Comment 15

[John-David Dalton]

Rock. I came here after digging into FF's support based on http://fitzgeraldnick.com/weblog/59/.Support is being discussed for Underscore _.template (https://github.com/jashkenas/underscore/issues/1973), Lo-Dash already supports sourceURLs.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f04adec1982d

Comment 17

[James Long (:jlongster)]

Comment on attachment 8534004 [details] [diff] [review]
1107541.patch

Approval Request Comment
[Feature/regressing bug #]: the major patch that has a bug is in bug 905700
[User impact if declined]: the new eval source functionality in the debugger is broken for scripts that have the `sourceURL` pragma, which is a pretty significant part of eval source debugging. they will show up as sources, but when clicked it will error when trying to show the source
[Describe test coverage new/current, TBPL]: new tests added in this bug, has landed on m-c
[Risks and why]: not much risk, small patch
[String/UUID change made/needed]:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/993c07956f3d

Comment 19

[jochen.kuehner]

Why is this closed with fixed? It still does not work. For Example look at https://shop.polymer-project.org/ with current Firefox. You will see in the script debugger when you look at "no domain", there are javascripts with "sourceurls" but that name is not used!