Add API for setting the sourceMapURL of a source

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: jlongster, Assigned: jlongster)

Tracking

unspecified
mozilla36
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

4 years ago
We need the ability to set the sourceMapURL property of a source. When we pretty print a source in the devtools, we generate a source map and we need to save it to the source so that all Debugger objects that might get the source will also get our generated source map.

Right now we generate the source map and save it locally within the SourceActor, but the problem is that when other Debugger objects access the source they don't see the sourcemap. To have a consistent experience we need the sourcemap to actually change and all things will work regardless of where they are coming from.
(Assignee)

Comment 1

4 years ago
The ScriptSource object actually already has a setSourceMapURL function, but it emits a warning if there's already a sourcemap. We need the ability to apply a sourcemap even if it already has one, so need to figure a way to provide that without the warning.
(Assignee)

Comment 2

4 years ago
Created attachment 8497635 [details] [diff] [review]
1074745.patch

This is a small patch that I haven't gotten to work yet. Any pointers would be appreciated. The patch removes the warning in setSourceMapURL on the ScriptSource object and makes the bytecode compiler give the warning itself. This lets us add a setter on the Debugger.Source object that lets us set it.

For some reason, when you set the sourceMapURL it doesn't copy the string correctly. See here where it gets back the wrong value:

> source.sourceMapURL
null
> source.sourceMapURL = "foo.js.map"
setting sourcemap
foo.js.map
> source.sourceMapURL
潦⹯獪洮灡

jimb helped me with the string code in `DebuggerSource_setSourceMapUrl` so I think it's close, but I can't figure out why it doesn't get the right value.
Attachment #8497635 - Flags: review?(jorendorff)
(Assignee)

Comment 3

4 years ago
Created attachment 8497636 [details] [diff] [review]
1074745.patch
Attachment #8497635 - Attachment is obsolete: true
Attachment #8497635 - Flags: review?(jorendorff)
(Assignee)

Comment 4

4 years ago
Evidently I wasn't using a debug build with assertions enabled so I can do that and try to figure out what's going on. I thought I had assertions enabled.
(Assignee)

Comment 5

4 years ago
Created attachment 8497953 [details] [diff] [review]
1074745.patch

Rewrote DebuggerSource_getSourceMapURL with string functions that actually work. Setting the sourceMapURL property seems to work now.

The key was using AutoStableStringChars which has an `initTwoByte` method which will handle both two byte and latin1 strings, inflating if necessary. Previously I assumed it was always a two byte string but we seem to have both formats internally.
Attachment #8497636 - Attachment is obsolete: true

Comment 6

4 years ago
Comment on attachment 8497953 [details] [diff] [review]
1074745.patch

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

This patch needs a test in js/src/jit-test/tests/debug.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +422,5 @@
>       */
>      if (options.sourceMapURL()) {
> +        // Warn about the replacement, but use the new one.
> +        if (ss->hasSourceMapURL()) {
> +            parser.report(ParseError, false, nullptr, JSMSG_ALREADY_HAS_PRAGMA,

We want ParseWarning here, don't we?

@@ +431,1 @@
>              return nullptr;

I think you dropped a ! here.

::: js/src/vm/Debugger.cpp
@@ +4214,5 @@
> +    JSFlatString *flat = str->ensureFlat(cx);
> +    if (!flat)
> +        return false;
> +
> +    AutoStableStringChars stableChars(cx);

We looked into this and decided that, if we use AutoStableStringChars, that takes care of the ensureFlat for us.

@@ +4251,5 @@
>      JS_PSG("introductionOffset", DebuggerSource_getIntroductionOffset, 0),
>      JS_PSG("introductionType", DebuggerSource_getIntroductionType, 0),
>      JS_PSG("elementAttributeName", DebuggerSource_getElementProperty, 0),
> +    JS_PSGS("sourceMapURL", DebuggerSource_getSourceMapUrl,
> +            DebuggerSource_setSourceMapUrl, 0),

The line length limit in SpiderMonkey is 100 columns; can this be one line?
(Assignee)

Comment 7

4 years ago
Created attachment 8498072 [details] [diff] [review]
1074745.patch

Fixed up everything in the above comment (except adding a test). I actually had run jit-tests but it didn't catch the bad `if` check in BytecodeCompiler.cpp... are there other tests I should run or is that code not being tested?

I will add a new test that tests the new sourceMapURL setter, and possibly more.
Attachment #8497953 - Attachment is obsolete: true
Attachment #8498072 - Flags: review?(jimb)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8498072 [details] [diff] [review]
1074745.patch

Will ask for review for final patch w/tests
Attachment #8498072 - Flags: review?(jimb)
(Assignee)

Comment 9

4 years ago
Making this depend on moving the sourceMapURL to the D.Source object because that will land today or tomorrow
Depends on: 1056409
(Assignee)

Comment 10

4 years ago
Gonna see if this patch gets a green try. I added some code to the existing sourceMapURL tests. https://tbpl.mozilla.org/?tree=Try&rev=a98a3126c59e
(Assignee)

Comment 11

4 years ago
Created attachment 8498153 [details] [diff] [review]
1074745.patch
Attachment #8498072 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → jlong
(Assignee)

Comment 12

4 years ago
Created attachment 8498164 [details]
warning-output.png

I tested that the warning is still generated for web code, here's proof.
(Assignee)

Comment 13

4 years ago
Created attachment 8499523 [details] [diff] [review]
1074745.patch
Attachment #8498153 - Attachment is obsolete: true
Attachment #8499523 - Flags: review?(jimb)

Comment 14

4 years ago
(In reply to James Long (:jlongster) from comment #12)
> I tested that the warning is still generated for web code, here's proof.

If you tell me it does, I will certainly believe you! :)
(Assignee)

Comment 16

4 years ago
Created attachment 8501890 [details] [diff] [review]
1074745.patch

Rebased. jimb you didn't actually r+ it before, mind looking again? Nothing has changed since you last looked at it.
Attachment #8499523 - Attachment is obsolete: true
Attachment #8499523 - Flags: review?(jimb)
Attachment #8501890 - Flags: review?(jimb)
(In reply to James Long (:jlongster) from comment #16)
> Created attachment 8501890 [details] [diff] [review]
> 1074745.patch
> 
> Rebased. jimb you didn't actually r+ it before, mind looking again? Nothing
> has changed since you last looked at it.

... Did you fix the comments he made in the last review?
Looks like you did, in which case, things certainly have changed :P
(Assignee)

Comment 19

4 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #18)
> Looks like you did, in which case, things certainly have changed :P

I was referring to comment 14 where he said he believed me :) but maybe he didn't actually look at the updated patch then. anyway, r? plz!
(Assignee)

Comment 20

4 years ago
@jimb any chance you could review this today/tomorrow?

Comment 21

4 years ago
Comment on attachment 8501890 [details] [diff] [review]
1074745.patch

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

This looks great, but I'd like to see some revisions and re-review.

This requires a small change to js/src/doc/Debugger/Debugger.Source.md mentioning that sourceMapURL is writable. As explained in js/src/doc/Debugger/Conventions.md, accessor properties do not include setters unless otherwise specified. When you document the setter, you need to explain that storing the null string as a value has no effect. 

Speaking of that behavior: I don't know why js::ScriptSource::setSourceMapURL does that, but there's no reason our Debugger.Source setter should emulate it; please file a follow-up bug to fix this.

We don't check whether we're overriding an extant URL in BytecodeCompiler.cpp's SetSourceMap. I think this is correct, because SetSourceMap is only called from CompileScript, and before we've handled whatever source map URL specified in the compile options. But if this is so, it would still be nice to assert that we're not overriding any existing source map URL in SetSourceMap.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +422,5 @@
>       */
>      if (options.sourceMapURL()) {
> +        // Warn about the replacement, but use the new one.
> +        if (ss->hasSourceMapURL()) {
> +            parser.report(ParseWarning, false, nullptr, JSMSG_ALREADY_HAS_PRAGMA,

You need to check the return value here, or else we'll fail to propagate OOM terminations from the error-reporting code. Just a simple "if (!...) return nullptr" should suffice. Note that this function will normally return true, since you passed ParseWarning, but will return false if werrorOption was set, so it's just what you want.

::: js/src/jit-test/tests/debug/Source-sourceMapURL-deprecated.js
@@ +74,5 @@
>             {sourceMapURL: 'http://example.com/bar.js.map'});
>  assertEq(getSourceMapURL(), 'http://example.com/foo.js.map');
> +
> +// Make sure setting the sourceMapURL manually works (used in the
> +// debugger)

"(used in the debugger)" is not a very helpful comment in a Debugger test. This isn't the place to explain why a certain API function is useful.

::: js/src/jit-test/tests/debug/Source-sourceMapURL.js
@@ +71,5 @@
>  // With both a comment and the evaluate option.
>  g.evaluate('function f() {}\n' +
>             '//# sourceMappingURL=http://example.com/foo.js.map',
>             {sourceMapURL: 'http://example.com/bar.js.map'});
>  assertEq(getSourceMapURL(), 'http://example.com/foo.js.map');

Why does this test still pass? It seems like js.cpp's Evaluate calls script->scriptSource()->setSourceMapURL after JS::Compile, so there's no way the URL from the directive could persist.

@@ +74,5 @@
>             {sourceMapURL: 'http://example.com/bar.js.map'});
>  assertEq(getSourceMapURL(), 'http://example.com/foo.js.map');
> +
> +// Make sure setting the sourceMapURL manually works (used in the
> +// debugger)

Fix comment here too.
Attachment #8501890 - Flags: review?(jimb)
(Assignee)

Comment 22

4 years ago
(In reply to Jim Blandy :jimb from comment #21)
> ::: js/src/jit-test/tests/debug/Source-sourceMapURL.js
> @@ +71,5 @@
> >  // With both a comment and the evaluate option.
> >  g.evaluate('function f() {}\n' +
> >             '//# sourceMappingURL=http://example.com/foo.js.map',
> >             {sourceMapURL: 'http://example.com/bar.js.map'});
> >  assertEq(getSourceMapURL(), 'http://example.com/foo.js.map');
> 
> Why does this test still pass? It seems like js.cpp's Evaluate calls
> script->scriptSource()->setSourceMapURL after JS::Compile, so there's no way
> the URL from the directive could persist.
> 

Not entirely sure about that, it was like that before: https://github.com/mozilla/gecko-dev/blob/master/js/src/jit-test/tests/debug/Source-sourceMapURL.js#L70. Looking into it.
(Assignee)

Comment 23

4 years ago
(In reply to James Long (:jlongster) from comment #22)
> (In reply to Jim Blandy :jimb from comment #21)
> > ::: js/src/jit-test/tests/debug/Source-sourceMapURL.js
> > @@ +71,5 @@
> > >  // With both a comment and the evaluate option.
> > >  g.evaluate('function f() {}\n' +
> > >             '//# sourceMappingURL=http://example.com/foo.js.map',
> > >             {sourceMapURL: 'http://example.com/bar.js.map'});
> > >  assertEq(getSourceMapURL(), 'http://example.com/foo.js.map');
> > 
> > Why does this test still pass? It seems like js.cpp's Evaluate calls
> > script->scriptSource()->setSourceMapURL after JS::Compile, so there's no way
> > the URL from the directive could persist.
> > 
> 
> Not entirely sure about that, it was like that before:
> https://github.com/mozilla/gecko-dev/blob/master/js/src/jit-test/tests/debug/
> Source-sourceMapURL.js#L70. Looking into it.

js.cpp's evaluate function only sets the sourceMapURL from the options if there isn't already a sourcemap: `if (sourceMapURL && !script->scriptSource()->hasSourceMapURL()) { ... }`

That's why that test passes. I'll attach a new patch once this build finishes and I run the tests.
(Assignee)

Comment 24

4 years ago
Created attachment 8524784 [details] [diff] [review]
1074745.patch
Attachment #8501890 - Attachment is obsolete: true
Attachment #8524784 - Flags: review?(jimb)

Comment 25

4 years ago
Comment on attachment 8524784 [details] [diff] [review]
1074745.patch

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

Looks good, with the doc problem fixed.

::: js/src/doc/Debugger/Debugger.Source.md
@@ +85,5 @@
>      specially formatted comment in the JavaScript source code, or via a
>      header in the HTTP reply that carried the generated JavaScript.)
>  
> +    You can change the source map URL by writing to this property.
> +    Setting `null` has no affect and will not change existing value.

"the empty string", not "`null`", right?
Attachment #8524784 - Flags: review?(jimb) → review+
(Assignee)

Comment 26

4 years ago
try link: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2ae7b14c4c4

Unfortunately I rebased on the latest fx-team and there seems to be a bunch of other commits in my try push... It looks good though. Those oranges look unrelated but I'm running them again. Worst case I'll do another try push without all those other commits.
(Assignee)

Comment 27

4 years ago
(In reply to Jim Blandy :jimb from comment #25)
> 
> "the empty string", not "`null`", right?

Ah yes!  I actually changed the paragraph to this, is that good?

    This property is writable, so you can change the source map URL by
    setting it. All Debugger.Source objects referencing the same
    script will see the change. Setting an empty string has no affect
    and will not change existing value.
(Assignee)

Comment 28

4 years ago
There's an error in the above paragraph, I meant "... referencing the same source ..."
(Assignee)

Comment 29

4 years ago
Created attachment 8525659 [details] [diff] [review]
1074745.patch
Attachment #8524784 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7775bda3aa20
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1325551
You need to log in before you can comment on or make changes to this bug.