As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1254893 - [jsdbg2] Synthesize Debugger.Scripts and Debugger.Sources for wasm
: [jsdbg2] Synthesize Debugger.Scripts and Debugger.Sources for wasm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: Unspecified Unspecified
: -- normal (vote)
: mozilla48
Assigned To: Shu-yu Guo [:shu]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1254453 1254984 1257045
Blocks: 1232014 1255630
  Show dependency treegraph
 
Reported: 2016-03-08 23:50 PST by Shu-yu Guo [:shu]
Modified: 2016-11-09 07:40 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Prep Debugger.Script for a tagged union referent. (7.83 KB, patch)
2016-03-08 23:53 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Prep Debugger.Source for a tagged union referent. (15.98 KB, patch)
2016-03-08 23:53 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Keep a list of wasm::Modules per compartment. (8.37 KB, patch)
2016-03-08 23:53 PST, Shu-yu Guo [:shu]
terrence.d.cole: review+
luke: feedback+
Details | Diff | Splinter Review
Synthesize Debugger.Scripts for wasm modules and find them via findScripts. (16.88 KB, patch)
2016-03-08 23:53 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Synthesize Debugger.Sources for wasm modules. (14.74 KB, patch)
2016-03-08 23:53 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Display placeholder text for synthesized Debugger.Sources. (3.37 KB, patch)
2016-03-08 23:54 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Add a .format property on Debugger.Script and Debugger.Source. (4.14 KB, patch)
2016-03-08 23:54 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Synthesize Debugger.Scripts for wasm modules and find them via findScripts. (23.39 KB, patch)
2016-03-10 02:28 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Synthesize Debugger.Sources for wasm modules. (15.06 KB, patch)
2016-03-10 02:28 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Display placeholder text for synthesized Debugger.Sources. (3.44 KB, patch)
2016-03-10 02:29 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Add a .format property on Debugger.Script. (3.36 KB, patch)
2016-03-10 02:29 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Support wasm for most of the Debugger.Source properties. (13.24 KB, patch)
2016-03-10 02:30 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Fire onNewScript for new wasm modules. (8.08 KB, patch)
2016-03-10 02:30 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Perfunctory tests. (1.90 KB, patch)
2016-03-10 02:31 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Append "> wasm" to URLs of wasm Debugger.Sources to hack around blacklisting in devtools. (2.75 KB, patch)
2016-03-10 02:34 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Synthesize Debugger.Scripts for wasm modules and find them via findScripts. (28.63 KB, patch)
2016-03-10 19:56 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources. (4.39 KB, patch)
2016-03-10 19:57 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Keep a list of wasm::Modules per compartment. (13.10 KB, patch)
2016-03-10 22:07 PST, Shu-yu Guo [:shu]
no flags Details | Diff | Splinter Review
Keep a list of wasm::Modules per compartment. (9.75 KB, patch)
2016-03-11 12:31 PST, Shu-yu Guo [:shu]
terrence.d.cole: review+
Details | Diff | Splinter Review
Update Debugger.Script docs. (20.44 KB, patch)
2016-03-11 16:43 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
Update Debugger.Source docs. (12.51 KB, patch)
2016-03-11 17:36 PST, Shu-yu Guo [:shu]
jimb: review+
Details | Diff | Splinter Review
cross-the-streams (13.12 KB, patch)
2016-03-12 00:12 PST, Luke Wagner [:luke]
shu: review+
Details | Diff | Splinter Review

Description User image Shu-yu Guo [:shu] 2016-03-08 23:50:25 PST
The dream is to have as much of the existing devtools to work out of the box with wasm as we can. To that end we'll try to synthesize various Debugger wrappers to transparently support wasm so that devtools JS doesn't need to treat wasm differently.

This is the very first part: synthesize enough to display placeholder source text for wasm modules.
Comment 1 User image Shu-yu Guo [:shu] 2016-03-08 23:53:01 PST
Created attachment 8728295 [details] [diff] [review]
Prep Debugger.Script for a tagged union referent.
Comment 2 User image Shu-yu Guo [:shu] 2016-03-08 23:53:12 PST
Created attachment 8728296 [details] [diff] [review]
Prep Debugger.Source for a tagged union referent.
Comment 3 User image Shu-yu Guo [:shu] 2016-03-08 23:53:27 PST
Created attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.
Comment 4 User image Shu-yu Guo [:shu] 2016-03-08 23:53:40 PST
Created attachment 8728298 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.
Comment 5 User image Shu-yu Guo [:shu] 2016-03-08 23:53:52 PST
Created attachment 8728299 [details] [diff] [review]
Synthesize Debugger.Sources for wasm modules.
Comment 6 User image Shu-yu Guo [:shu] 2016-03-08 23:54:03 PST
Created attachment 8728300 [details] [diff] [review]
Display placeholder text for synthesized Debugger.Sources.
Comment 7 User image Shu-yu Guo [:shu] 2016-03-08 23:54:14 PST
Created attachment 8728301 [details] [diff] [review]
Add a .format property on Debugger.Script and Debugger.Source.
Comment 8 User image Shu-yu Guo [:shu] 2016-03-08 23:54:34 PST
I'll write tests tomorrow. I am tired now.
Comment 9 User image Shu-yu Guo [:shu] 2016-03-08 23:56:58 PST
Example output right now:

shu@cz ~/m/c/j/s/Debug (dbg-wasm)
% cat test.js
var g = newGlobal();
g.eval(`o = Wasm.instantiateModule(wasmTextToBinary('(module (func) (export "" 0))'));`);
var dbg = new Debugger(g);
var scripts = dbg.findScripts();
for (let script of scripts) {
    if (script.format === "wasm")
        print(script.source.text);
}
                                                                                                                                                                
shu@cz ~/m/c/j/s/Debug (dbg-wasm)
% dist/bin/js test.js
.--.      .--.   ____       .-'''-. ,---.    ,---.
|  |_     |  | .'  __ `.   / _     \|    \  /    |
| _( )_   |  |/   '  \  \ (`' )/`--'|  ,  \/  ,  |
|(_ o _)  |  ||___|  /  |(_ o _).   |  |\_   /|  |
| (_,_) \ |  |   _.-`   | (_,_). '. |  _( )_/ |  |
|  |/    \|  |.'   _    |.---.  \  :| (_ o _) |  |
|  '  /\  `  ||  _( )_  |\    `-'  ||  (_,_)  |  |
|    /  \    |\ (_ o _) / \       / |  |      |  |
`---'    `---` '.(_,_).'   `-...-'  '--'      '--'
Comment 10 User image Jim Blandy :jimb 2016-03-09 00:56:16 PST
Comment on attachment 8728295 [details] [diff] [review]
Prep Debugger.Script for a tagged union referent.

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

::: js/src/vm/Debugger.cpp
@@ +4905,5 @@
> +//   2. A wasm JSFunction, denoting a synthesized wasm function script.
> +//      NYI!
> +using DebuggerScriptReferent = Variant<JSScript*, WasmModuleObject*>;
> +
> +// Very very scary. For internal use only.

Comments like this are unhelpful. And this function is perfectly well-typed anyway.

@@ +5049,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);                                       \
> +    RootedObject obj(cx, DebuggerScript_check(cx, args.thisv(), fnname));           \
> +    if (!obj)                                                                       \
> +        return false;                                                               \
> +    Rooted<DebuggerScriptReferent> referent(cx, GetScriptReferent(obj))

This is possible because we #included "js/GCVariant.h", from bug 1254453, right?
Comment 11 User image Jim Blandy :jimb 2016-03-09 00:57:05 PST
(In reply to Shu-yu Guo [:shu] from comment #9)
> Example output right now:

Oh, I love it.
Comment 12 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2016-03-09 08:16:38 PST
(In reply to Shu-yu Guo [:shu] from comment #9)
> .--.      .--.   ____       .-'''-. ,---.    ,---.
> |  |_     |  | .'  __ `.   / _     \|    \  /    |
> | _( )_   |  |/   '  \  \ (`' )/`--'|  ,  \/  ,  |
> |(_ o _)  |  ||___|  /  |(_ o _).   |  |\_   /|  |
> | (_,_) \ |  |   _.-`   | (_,_). '. |  _( )_/ |  |
> |  |/    \|  |.'   _    |.---.  \  :| (_ o _) |  |
> |  '  /\  `  ||  _( )_  |\    `-'  ||  (_,_)  |  |
> |    /  \    |\ (_ o _) / \       / |  |      |  |
> `---'    `---` '.(_,_).'   `-...-'  '--'      '--'

https://youtu.be/Mh5LY4Mz15o?t=3s
Comment 13 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2016-03-09 10:07:27 PST
Comment on attachment 8728301 [details] [diff] [review]
Add a .format property on Debugger.Script and Debugger.Source.

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

Drive-by nit (the best kind of nit): needs docs.
Comment 14 User image Shu-yu Guo [:shu] 2016-03-09 12:20:57 PST
(In reply to Jim Blandy :jimb from comment #10)
> Comment on attachment 8728295 [details] [diff] [review]
> Prep Debugger.Script for a tagged union referent.
> 
> Review of attachment 8728295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +5049,5 @@
> > +    CallArgs args = CallArgsFromVp(argc, vp);                                       \
> > +    RootedObject obj(cx, DebuggerScript_check(cx, args.thisv(), fnname));           \
> > +    if (!obj)                                                                       \
> > +        return false;                                                               \
> > +    Rooted<DebuggerScriptReferent> referent(cx, GetScriptReferent(obj))
> 
> This is possible because we #included "js/GCVariant.h", from bug 1254453,
> right?

That's right. I'll land the other bug with this one.
Comment 15 User image Terrence Cole [:terrence] 2016-03-09 12:45:59 PST
Comment on attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

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

Looks great!
Comment 16 User image Luke Wagner [:luke] 2016-03-09 12:53:41 PST
Comment on attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

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

Perfect, thanks!
Comment 17 User image Shu-yu Guo [:shu] 2016-03-10 02:28:35 PST
Created attachment 8728903 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.
Comment 18 User image Shu-yu Guo [:shu] 2016-03-10 02:28:49 PST
Created attachment 8728904 [details] [diff] [review]
Synthesize Debugger.Sources for wasm modules.
Comment 19 User image Shu-yu Guo [:shu] 2016-03-10 02:29:03 PST
Created attachment 8728905 [details] [diff] [review]
Display placeholder text for synthesized Debugger.Sources.
Comment 20 User image Shu-yu Guo [:shu] 2016-03-10 02:29:55 PST
Created attachment 8728907 [details] [diff] [review]
Add a .format property on Debugger.Script.

Debugger.Source can be distinguished via .introductionType == "wasm", so
.format isn't needed.
Comment 21 User image Shu-yu Guo [:shu] 2016-03-10 02:30:24 PST
Created attachment 8728908 [details] [diff] [review]
Support wasm for most of the Debugger.Source properties.
Comment 22 User image Shu-yu Guo [:shu] 2016-03-10 02:30:38 PST
Created attachment 8728909 [details] [diff] [review]
Fire onNewScript for new wasm modules.
Comment 23 User image Shu-yu Guo [:shu] 2016-03-10 02:31:41 PST
Created attachment 8728910 [details] [diff] [review]
Perfunctory tests.

These really aren't very good, but I don't want to put too much effort into
testing something that's moving so quickly.
Comment 24 User image Shu-yu Guo [:shu] 2016-03-10 02:34:41 PST
Created attachment 8728912 [details] [diff] [review]
Append "> wasm" to URLs of wasm Debugger.Sources to hack around blacklisting in devtools.

The problem here is that the wasm modules have the caller's filename (i.e. the
file that called Wasm.instantiateModule, which is the current nonstandard way
to make wasm modules). If that filename happens to be blacklisted for whatever
reason, no wasm sources will get displayed.

This is a quick and dirty hack to work around this until wasm modules become
real modules and have real URLs.
Comment 25 User image Jim Blandy :jimb 2016-03-10 14:05:23 PST
Comment on attachment 8728296 [details] [diff] [review]
Prep Debugger.Source for a tagged union referent.

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

::: js/src/vm/Debugger.cpp
@@ +102,5 @@
> +
> +// Either a real ScriptSourceObject or a synthesized.
> +//
> +// If synthesized, the referent is a WasmModuleObject, denoting the
> +// synthesized source of a wasm module.

Simplify:

// Either a ScriptSourceObject, for ordinary JS, or a WasmModuleObject, denoting
// the synthesized source of a wasm module.

@@ +3914,5 @@
> +            DebuggerSourceReferent referent = GetSourceReferent(&debuggerSource.toObject());
> +            if (!referent.is<ScriptSourceObject*>()) {
> +                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_UNEXPECTED_TYPE,
> +                                     "query object's 'source' property",
> +                                     "not undefined nor a Debugger.Source object for a JS script");

"neither undefined"

@@ +6144,2 @@
>          TraceManuallyBarrieredCrossCompartmentEdge(trc, obj, &referent,
> +                                                   "Debugger.Source source referent");

Why this change to the string?

@@ +6210,5 @@
>      return false;
>  }
>  
>  static NativeObject*
> +DebuggerSource_check(JSContext* cx, const Value& thisv, const char* fnname)

Wouldn't it be better to use HandleValue here?
Comment 26 User image Shu-yu Guo [:shu] 2016-03-10 19:56:49 PST
Created attachment 8729333 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

The previous version's filtering of sources was plain wrong. Keep a variant and
match on that; it's simpler and correcter.
Comment 27 User image Shu-yu Guo [:shu] 2016-03-10 19:57:30 PST
Created attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.
Comment 28 User image Shu-yu Guo [:shu] 2016-03-10 22:07:46 PST
Created attachment 8729359 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

I ended up having to add a sweep method to update the weak backpointer on
wasm::Module due to WasmModuleObjects moving due to compaction.

Could use a second look. Is adding a sweep phase the only way to fix up these
weak backpointers?
Comment 29 User image Terrence Cole [:terrence] 2016-03-11 12:19:04 PST
Comment on attachment 8729359 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

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

As discussed on IRC, the Module -> ModuleObject edge should just be strong so that the normal heap traversal will update the edge when compacting.
Comment 30 User image Shu-yu Guo [:shu] 2016-03-11 12:31:41 PST
Created attachment 8729683 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

Strong reference version. Much simpler.
Comment 31 User image Jim Blandy :jimb 2016-03-11 14:40:28 PST
Comment on attachment 8728903 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

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

Posting comments I had so far on the old version of patch. I will look at the new one.

::: js/src/vm/Debugger.h
@@ +678,5 @@
> +
> +    template <typename ReferentVariant, typename Referent, typename Map>
> +    JSObject* wrapVariantReferent(JSContext* cx, Map& map, CrossCompartmentKey::Kind keyKind,
> +                                  Handle<ReferentVariant> referent);
> +    JSObject* wrapVariantReferent(JSContext* cx, Handle<DebuggerScriptReferent> referent);

These function names (newVariantWrapper, wrapVariantReferent) are terrible. I have no idea what a "variant wrapper" is.

@@ +984,5 @@
> +     * script), synthesizing a new one if needed. The context |cx| must be in
> +     * the debugger compartment; |wasmModule| must be a WasmModuleObject in
> +     * the debuggee compartment.
> +     */
> +    JSObject* synthesizeWasmScript(JSContext* cx, Handle<WasmModuleObject*> wasmModule);

This should be named wrapWasmModule.
Comment 32 User image Jim Blandy :jimb 2016-03-11 15:45:56 PST
Comment on attachment 8729333 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

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

::: js/src/doc/Debugger/Debugger.Script.md
@@ +42,5 @@
>  equivalent functions or evaluated code—that is, scripts representing the
>  same source code, at the same position in the same source file,
>  evaluated in the same lexical environment.
>  
> +# Debugger.Script for WebAssembly

The number of '#' marks determines the header level, which affects the nesting of headers. By making this a top-level header, you've made all the subsequent sections children of "Debugger.Script for WebAssembly", which is wrong.

It's not adequate to simply say, at the top, "A `Debugger.Script` instance may refer to a sequence of bytecode", and then mention "these instances" six paragraphs later. You need to start at the top and say something like:

A `Debugger.Script` refers to either a sequence of JavaScript bytecode, or a block of WebAssembly code. The two cases are distinguished by the `format` property, which is either `"js"` or `"wasm"`.

For JavaScript, ...

For WebAssembly, ...

The format property must also be documented in its place along with all the other accessors.

This is not the first time I've sent back doc patches to you for being obviously inadequate, in ways that you shouldn't need a patch review to see. You have experience reading documentation; I feel like these problems should be apparent to you. I suspect that you didn't put much care into writing the documentation. If I allow patches like this to land, the documentation will become worse and worse. 

If you feel that the documentation has no audience and isn't worth maintaining, then that's a issue we can discuss with the team. Perhaps I'm overestimating the value of internal documentation of this sort. But until such a decision is reached, we should maintain a certain level of quality.

As things stand, this makes me pretty upset, because I put a lot of time into writing these docs originally, and tried to make them good.

@@ +47,5 @@
> +
> +A `Debugger.Script` instance may also refer to WebAssembly code. These
> +instances are distinguished by their `format` property being `"wasm"`.
> +
> +Currently, only the `format` and `text` properties works for WebAssembly

There is no `text` property on a Debugger.Script.

::: js/src/vm/Debugger.cpp
@@ +3927,5 @@
>                  return false;
>              }
>  
> +            hasSource = true;
> +            source = GetSourceReferent(&debuggerSource.toObject());

Yes, this is much nicer.

@@ +5066,5 @@
> +
> +        CrossCompartmentKey key(keyKind, object, untaggedReferent);
> +        if (!object->compartment()->putWrapper(cx, key, ObjectValue(*wrapper))) {
> +            map.remove(untaggedReferent);
> +            ReportOutOfMemory(cx);

Wouldn't the call to putWrapper have reported the OOM already itself?

@@ +5099,3 @@
>  
> +JSObject*
> +Debugger::synthesizeWasmScript(JSContext* cx, Handle<WasmModuleObject*> wasmModule)

This should be named wrapWasmScript.
Comment 33 User image Jim Blandy :jimb 2016-03-11 16:23:11 PST
Comment on attachment 8728907 [details] [diff] [review]
Add a .format property on Debugger.Script.

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

::: js/src/vm/Debugger.cpp
@@ +5304,5 @@
> +struct DebuggerScriptGetFormatMatcher
> +{
> +    using ReturnType = const char*;
> +    ReturnType match(HandleScript script) { return "js"; }
> +    ReturnType match(Handle<WasmModuleObject*> wasmModule) { return "wasm"; }

You could just put these in CommonPropertyNames.h; but it's fine like this too.
Comment 34 User image Shu-yu Guo [:shu] 2016-03-11 16:43:01 PST
Created attachment 8729748 [details] [diff] [review]
Update Debugger.Script docs.
Comment 35 User image Jim Blandy :jimb 2016-03-11 16:47:12 PST
Comment on attachment 8728908 [details] [diff] [review]
Support wasm for most of the Debugger.Source properties.

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

::: js/src/vm/Debugger.cpp
@@ +6713,5 @@
>      args.rval().setUndefined();
>      return true;
>  }
>  
> +class DebuggerSourceGetSourceMapUrlMatcher

Is there a reason we can't have ReturnType = const char16_t *, and then leave all the string allocation logic in DebuggerSource_getSourceMapUrl?
Comment 36 User image Shu-yu Guo [:shu] 2016-03-11 16:57:28 PST
(In reply to Jim Blandy :jimb from comment #35)
> Comment on attachment 8728908 [details] [diff] [review]
> Support wasm for most of the Debugger.Source properties.
> 
> Review of attachment 8728908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Debugger.cpp
> @@ +6713,5 @@
> >      args.rval().setUndefined();
> >      return true;
> >  }
> >  
> > +class DebuggerSourceGetSourceMapUrlMatcher
> 
> Is there a reason we can't have ReturnType = const char16_t *, and then
> leave all the string allocation logic in DebuggerSource_getSourceMapUrl?

Because then we can't distinguish a nullptr between being OOM and simply not having a source map URL, for which we return null.
Comment 37 User image Shu-yu Guo [:shu] 2016-03-11 17:00:19 PST
(In reply to Shu-yu Guo [:shu] from comment #36)
> (In reply to Jim Blandy :jimb from comment #35)
> > Comment on attachment 8728908 [details] [diff] [review]
> > Support wasm for most of the Debugger.Source properties.
> > 
> > Review of attachment 8728908 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/vm/Debugger.cpp
> > @@ +6713,5 @@
> > >      args.rval().setUndefined();
> > >      return true;
> > >  }
> > >  
> > > +class DebuggerSourceGetSourceMapUrlMatcher
> > 
> > Is there a reason we can't have ReturnType = const char16_t *, and then
> > leave all the string allocation logic in DebuggerSource_getSourceMapUrl?
> 
> Because then we can't distinguish a nullptr between being OOM and simply not
> having a source map URL, for which we return null.

Wait, that doesn't make any sense. I'll change ReturnType = const char16_t*.
Comment 38 User image Jim Blandy :jimb 2016-03-11 17:03:47 PST
Comment on attachment 8728912 [details] [diff] [review]
Append "> wasm" to URLs of wasm Debugger.Sources to hack around blacklisting in devtools.

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

::: js/src/vm/Debugger.cpp
@@ +6511,5 @@
>  struct DebuggerSourceGetURLMatcher
>  {
> +    JSContext* cx_;
> +    explicit DebuggerSourceGetURLMatcher(JSContext* cx) : cx_(cx) { }
> +  public:

Why the 'public:'? It's a struct.

@@ +6527,5 @@
> +        // TODOshu: Until wasm modules have real URLs, append "> wasm" to the
> +        // end to prevent them from being blacklisted by devtools by having
> +        // the same value as a source mapped URL.
> +        char buf[4096];
> +        JS_snprintf(buf, sizeof(buf), "%s > wasm", wasmModule->module().filename());

Could we use JS_smprintf and JS_smprintf_free here instead?
Comment 39 User image Shu-yu Guo [:shu] 2016-03-11 17:36:12 PST
Created attachment 8729759 [details] [diff] [review]
Update Debugger.Source docs.
Comment 40 User image Jim Blandy :jimb 2016-03-11 17:38:58 PST
Comment on attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.

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

Need to check that the .format of an ordinary Debugger.Script is js.

Need to check cases of more than one Wasm module in the map; I didn't see anything that creates more than one.

Need to check that wrapping the same Wasm module twice gets the identical Debugger.Script; I guess doing two findScripts calls is the only way to make this happen.

Need to check that findScript; compacting GC; findScript gives you same Wasm module wrapper (i.e. that your weakmaps are getting correctly updated for object relocations)

::: js/src/jit-test/tests/debug/wasm-01.js
@@ +14,5 @@
> +    break;
> +  }
> +}
> +
> +print(dbg.findScripts().length);

This can probably come out, right?
Comment 41 User image Jim Blandy :jimb 2016-03-11 17:41:20 PST
Comment on attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.

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

Test that onNewScript reports a distinct wrapper for each new Wasm module.
Comment 45 User image Luke Wagner [:luke] 2016-03-12 00:12:58 PST
Created attachment 8729812 [details] [diff] [review]
cross-the-streams
Comment 46 User image Shu-yu Guo [:shu] 2016-03-12 00:35:21 PST
Comment on attachment 8729812 [details] [diff] [review]
cross-the-streams

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

The usability issue with having to reload is a bummer, but fine by me since this is all experimental.

::: js/src/asmjs/Wasm.cpp
@@ +1600,5 @@
>  
>      if (!CreateInstance(cx, exportObj, instance))
>          return false;
>  
> +    if (cx->compartment()->debuggerObservesAsmJS()) {

This use is fine by me for the demo, but I'm not sure how this flag should interact with wasm yet. This flag being on *prevents* asm.js compilation at all, so firstly, logically I'm not sure we should reach this point if the flag is meant to behave analogously for wasm.

Secondly, I'm all about the debugger working seamlessly whether it was opened after the fact or before. I hope we eventually can generate text regardless of when the debugger was opened (by refetching the code, maybe).
Comment 48 User image Luke Wagner [:luke] 2016-03-12 11:34:37 PST
(In reply to Shu-yu Guo [:shu] from comment #46)
> This flag being on *prevents* asm.js compilation at
> all, so firstly, logically I'm not sure we should reach this point if the
> flag is meant to behave analogously for wasm.

With asm.js we can fall back to JS.  Not so for wasm.  Actually we have an improvement here over asm.js: with debugger open, wasm games run at full speed.

> Secondly, I'm all about the debugger working seamlessly whether it was
> opened after the fact or before. I hope we eventually can generate text
> regardless of when the debugger was opened (by refetching the code, maybe).

Yes, agreed we should improve here.  I like the idea of (1) always save bits when "names" (i.e. symbols) section is present and (2) if not, attempt to fetch bits from the network (though that requires es6 module integration).
Comment 51 User image Pulsebot 2016-11-08 15:37:16 PST
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/584c27117cd6
Forgot to commit test.
Comment 52 User image Carsten Book [:Tomcat] 2016-11-09 07:40:15 PST
https://hg.mozilla.org/mozilla-central/rev/584c27117cd6

Note You need to log in before you can comment on or make changes to this bug.