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.

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.

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.

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.

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.

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?

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.

Comment on attachment 8498072 [details] [diff] [review] 1074745.patch Will ask for review for final patch w/tests

Making this depend on moving the sourceMapURL to the D.Source object because that will land today or tomorrow

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

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

(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! :)

Trying this on tbpl https://tbpl.mozilla.org/?tree=Try&rev=1c731e66fd97

Rebased. jimb you didn't actually r+ it before, mind looking again? Nothing has changed since you last looked at it.

(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

(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!

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

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.

(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.

(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.

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?

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.

(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.

There's an error in the above paragraph, I meant "... referencing the same source ..."