Closed Bug 1107541 Opened 5 years ago Closed 5 years ago

Fix eval scripts with a sourceURL pragma

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch 1107541.patch (obsolete) — Splinter Review
Attachment #8532103 - Flags: review?(nfitzgerald)
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?
Attachment #8532103 - Flags: review?(nfitzgerald) → review+
Duplicate of this bug: 833744
Attached patch 1107541.patch (obsolete) — Splinter Review
Attachment #8532103 - Attachment is obsolete: true
Keywords: checkin-needed
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
Assignee: nobody → jlong
Whiteboard: [fixed-in-fx-team]
[Tracking Requested - why for this release]: Strange / broken eval behavior added in 36 as part of bug 905700
Attached patch 1107541.patchSplinter Review
Doh! e10s issue should be fixed.
Attachment #8532653 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f04adec1982d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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');
(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.
@jdalton what Nick said. Love that you commented here though. Please continue to point out places where it's broken!
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.
https://hg.mozilla.org/mozilla-central/rev/f04adec1982d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
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]:
Attachment #8534004 - Flags: approval-mozilla-aurora?
Attachment #8534004 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
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!
You need to log in before you can comment on or make changes to this bug.