Closed Bug 637572 Opened 13 years ago Closed 7 years ago

[jsdbg2] [meta] Implement Debugger.Source, providing script source code and detailed origin information

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [leave-open])

Attachments

(13 files, 34 obsolete files)

7.67 KB, patch
Details | Diff | Splinter Review
9.97 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
17.54 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
18.40 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
6.02 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
6.25 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
1.81 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
8.76 KB, patch
jimb
: review+
jimb
: checkin+
Details | Diff | Splinter Review
11.77 KB, patch
terrence
: review+
jimb
: checkin+
Details | Diff | Splinter Review
1.98 KB, patch
billm
: review+
jimb
: checkin+
Details | Diff | Splinter Review
11.37 KB, patch
sfink
: review+
jimb
: checkin+
Details | Diff | Splinter Review
6.29 KB, patch
sfink
: review+
jimb
: checkin+
Details | Diff | Splinter Review
9.24 KB, patch
sfink
: review+
jimb
: checkin+
Details | Diff | Splinter Review
At the moment, the only information we have about a script's origin is a filename/url and line number. But we should be able to do better than this for code passed to eval, inline code from HTML documents, code added via element.appendChild, document.write, evalInSandBox --- every way that code enters the system should be properly identified.

When requested, we should even retain the full source code. Developers want this. V8 implements it.

One idea is to store the origin information not as fields in a C++ structure but as properties of a JavaScript object the script points to. This would give embeddings like the browser flexibility in how they describe the origin of the code they're handing to SpiderMonkey (a DOM node reference, perhaps?), and calls to eval and Function could extend that:

Simple case:

{ type:"url", line:40, url:"http://foo.com" }

That calls appendChild, and that code calls eval; to refer to line 10 of that evaluated text:

{ type:"eval", line:10,
  calledAt: {type: "appendChild", line:20,
             calledAt: {type:"inlineHTML", line:40, url:"http://foo.com"}}}

If I'm understanding correctly, JJB says that Firebug goes through a lot of hairy contortions now to try to reconstruct this information from the call stack. If we could just get it straight off the JSScript, that would be much better.

We could also store the original source code in these objects, when we're asked to retain it.
YES!  See bug 624621 comment 6.

Note that ideally we'd be able to serialize and deserialize this stuff via XDR, so using DOM nodes won't fly, but something JSON-like per above could definitely work.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=449464#c35

Bug 449464 used an event to pass information. At the time this was just expedience. But now I think it may be a good solution. The issue we face here is dynamics: 
{ type:"url", line:40, url:"http://foo.com" }
-------------------^ what do I do if 400 more lines are inserted at line 20?
Any information we set onto the script object only has meaning at the point it is set. The debugger will have to extract the information at creation and analyze it; it probably won't be able to use it later. Thus an event oriented API might make sense (though any thing you want to throw at this problem is welcome ;-).
It would be nice if this extra info could be stored compactly.  JSScript structs already easily takes up 10s of MBs of memory, and that doesn't count things that hang off them.
Does the JSScript for a function just point to the main script's filename?  Or does it copy it?  Seems like a lot of this info should be the same for each function's script and the overall script the functions are defined in...
Hence my suggestion for a 'compilation unit" in bug 449464.
(In reply to comment #3)
> It would be nice if this extra info could be stored compactly.  JSScript
> structs already easily takes up 10s of MBs of memory, and that doesn't count
> things that hang off them.

We should measure as we go. If we atomize the strings involved (filenames, etc.), that should avoid duplication there. How bad are JSObjects? It seems like if they can share shapes, and we don't have too much internal fragmentation from unused slots, they should be pretty dense.

(In reply to comment #4)
> Does the JSScript for a function just point to the main script's filename?  Or
> does it copy it?  Seems like a lot of this info should be the same for each
> function's script and the overall script the functions are defined in...

Exactly --- I would expect a lot of the structure to be shareable.

This is a data structure we know something about, so we can have bespoke code to accurately measure its impact.
(In reply to comment #2)
> See also https://bugzilla.mozilla.org/show_bug.cgi?id=449464#c35

Sounds like pretty much the same idea --- I'm glad we're thinking along the same lines! :)

> Any information we set onto the script object only has meaning at the point it
> is set. The debugger will have to extract the information at creation and
> analyze it; it probably won't be able to use it later. Thus an event oriented
> API might make sense (though any thing you want to throw at this problem is
> welcome ;-).

If these structures included a DOM node reference, one could use that to "float" the script's position as other parts of the document changed, couldn't one? (We could document the line/column numbers to be "at creation"; sophisticated clients would ignore it, and simple-minded clients would at least work in simple cases.) We'd have to drop DOM node references when serializing, as bz points out in comment 1, but that doesn't seem like the end of the world.

We don't want to prevent clever debugger implementers from doing a better job than we anticipated being possible. So perhaps we should keep the script creation and deletion events, but design the script-attached data to directly provide whatever Firebug is currently inferring from the ambient stack frames. Firebug can continue to jump through hoops to track dynamism; it will just have a reliable source of information about where scripts originated, at the time of origin.
> but that doesn't seem like the end of the world.

At least until we store serialized precompiled scripts in our cache... ;)
(In reply to comment #7)

> We don't want to prevent clever debugger implementers from doing a better job
> than we anticipated being possible. So perhaps we should keep the script
> creation and deletion events, but design the script-attached data to directly
> provide whatever Firebug is currently inferring from the ambient stack frames.
> Firebug can continue to jump through hoops to track dynamism; it will just have
> a reliable source of information about where scripts originated, at the time of
> origin.

A stream or event-oriented debugger would be a different paradigm and I understand that brings with it some uncertainty about how to proceed. But if we tackle dynamics from the beginning we will create significant new opportunity for improvements. I guess as long as the debugger can get control to analyze information as it is created it should not matter if it is stored or passed as an event.
From Bug 636907
>> > The wiki sketch linked to in comment 0 doesn't pass a frame to the script
>> > creation and destruction hooks; 
>> 
>> Why not?
>
>Primarily because the JSDBGAPI's JSDebugHooks members don't, but that's almost
>a non sequitur. A better argument is that JSScript creation isn't intrinsically
>related to any particular JS action; if some C++ code calls JS_CompileScript,
>that may not have anything to do with the top JS stack frame in the context.

But something caused the JS_CompileScript. A stack is just chain of causes. JS devs don't need the C++ stack, but they do need to know what action the engine was processing when it compiled code.  Even if we decide that providing information on C++ origins is too difficult to support, it just means that occasionally the frame will be null. That's *much* better than no frame.
(In reply to comment #1)
> YES!  See bug 624621 comment 6.

Okay, so we need pre- and post-redirect URLs for scripts; what properties would you like to see there? I'm pretty ignorant of the big picture once we get outside JS.
(In reply to comment #9)
> I guess as long as the debugger can get control to analyze
> information as it is created it should not matter if it is stored or passed as
> an event.

I'm fine with the newScript event; I don't want to take it out. I just want to get the debugger out of the business of reading tea leaves by looking at the JS stack frames that happen to be around when the script is created.
(In reply to comment #12)
> I'm fine with the newScript event; I don't want to take it out. I just want to
> get the debugger out of the business of reading tea leaves by looking at the JS
> stack frames that happen to be around when the script is created.

In other words, the debugger would get a newScript event, and instead of inspecting the call stack, it would ask the script itself for its origin information, and then begin doing its dynamism-tracking magic.
> what properties would you like to see there?

I'm not sure what you mean.  Ideally I'd have the original URI linked to and the final principal, I think...
Taking comments on bug 636907 and turning them into requirements here.

The origin information needs to distinguish these cases, among others (that's your cue, folks --- what else?!?):

(In reply to comment #18)
> But
> in terms of API we want to know if a function-that-isn't-a-function is a
> browser-generated event handler or a top-level script code.

(In reply to comment #20)
> The string form of setTimeout will be a case like indirect eval, I think. 
> javascript: URLs as well.

(In reply to comment #21)
> Yes, event handlers are just functions.
Here's a first cut. Probably plenty of dumb errors, and incomplete; please let me know. JJB, if you could get this kind of data from a JSScript, would that be a substantial improvement? (If it's only a minor help, then I've misunderstood the problem.)

https://wiki.mozilla.org/Script_Origin_Tracking
Of course anything resembling that page would be a giant leap forward.

Two comments. First:
>Any script origin value may also have a property named source, whose value is the original source code of the script.

Obviously this property has limited value if we can't count on it. What do we do in the case where there is no source property?

Second: 
>Usually, the code in 'javascript:' URLs is so ephemeral that debuggers won't come across it,

In reality, all code is ephemeral. It just that we have a couple of ways of halting the world to study it: avoid generating events and wait for the current turn to finish or halt the program with the debugger, for examples. Any code can be buggy and thus the debugger needs to 'come across' all code. 

So what mechanism do we have to deal with dynamics? If I understand the proposal, the values available in the data structures are correct immediately after the code is compiled. But any time after that the values can become invalid. Most of the time this is not an issue and since many of the values are references they will be somewhat robust to change around them (eg the DOM structure can change but we still reference the element). 

A common case where we could have problems: code that inserts a script tag to compile JS, then removes the tag. The static information will tell us that the code was added by a script tag, but I guess the element will be invalid? or at least not in the document. 

I imagine that the static information gives enough information to halt the program before the location information is invalid. For example, I could set DOM mutation event handlers to look for script tag removal. But tool development would be much faster and less prone to failures if we had an event just when the information was available and certainly correct, eg just after compile.

In any case, this would be an awesome improvement!
(In reply to comment #17)
> Of course anything resembling that page would be a giant leap forward.
> 
> Two comments. First:
> >Any script origin value may also have a property named source, whose value is the original source code of the script.
> 
> Obviously this property has limited value if we can't count on it. What do
> we do in the case where there is no source property?

I would like to have an option that directs Firefox to save all the source, which the debugger could turn on. I'm not sure if it's practical to have that option on by default, but I would like to collect the rest of the information by default. If it works out, we could simply require the 'source' property.

> Second: 
> >Usually, the code in 'javascript:' URLs is so ephemeral that debuggers won't come across it,
> 
> In reality, all code is ephemeral. It just that we have a couple of ways of
> halting the world to study it: avoid generating events and wait for the
> current turn to finish or halt the program with the debugger, for examples.
> Any code can be buggy and thus the debugger needs to 'come across' all code. 

Absolutely.

> So what mechanism do we have to deal with dynamics? If I understand the
> proposal, the values available in the data structures are correct
> immediately after the code is compiled. But any time after that the values
> can become invalid. Most of the time this is not an issue and since many of
> the values are references they will be somewhat robust to change around them
> (eg the DOM structure can change but we still reference the element).

The element references are pretty much the only mechanism. It seems to me there are several views that are useful, depending on circumstances:

- If you're trying to relate the code back to markup you got from the server, then the original line numbers are actually what you want. You don't want subsequent DOM manipulation to make it difficult to track things back.

- If you're trying to relate the code to some sort of DOM tree-like display (DOM browser), then the element is the right thing.

- If you're trying to relate the code to markup produced from the current DOM, then this is the case where the line numbers are wrong. However (and granted, there's nothing in the proposal for this), such markup is always generated by an on-demand traversal of the DOM, and such a traversal could easily generate corresponding line numbers for the elements it encounters.

> A common case where we could have problems: code that inserts a script tag
> to compile JS, then removes the tag. The static information will tell us
> that the code was added by a script tag, but I guess the element will be
> invalid? or at least not in the document.

Yeah; it seems to me that the best the debugger can do in such a case is 1) keep the source code around, and 2) explain when presenting it that it is a <script> element no longer in the document. The 'source' property does 1); the element reference allows us to do 2).

> I imagine that the static information gives enough information to halt the
> program before the location information is invalid. For example, I could set
> DOM mutation event handlers to look for script tag removal. But tool
> development would be much faster and less prone to failures if we had an
> event just when the information was available and certainly correct, eg just
> after compile.

So, line numbers in script locations are always relative to the start of the <script> element or the element with the handler. If you can dynamically compute the line number of the <script> element (which you'll need to do either way), then just adding the script location's line number to that will give you the accurate line number.

> In any case, this would be an awesome improvement!

Okay!
Will we be able to have something like this work:

http://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/

specifically, for injected script elements (where `text` is set, not `src`)?
I'm getting a little confused figuring out what is a dupe of what, so for now I'm going to list out some related bugs:

bug 332176 - we need to properly record eval locations
bug 307984 - specific case of evalInSandbox, with a way for the caller to pass in the info (which makes it unpredictable what crap you'll need to deal with)
bug 667514 - another eval filename:line proposal

Should we just dupe them all to this one?
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Blocks: 332176
Blocks: 11240
With bug 761723, we'll be retaining source code for scripts. This means it makes sense to present that data via a Debugger.Source type. I've drafted some documentation for Debugger.Source here:

https://github.com/jimblandy/DebuggerDocs/compare/master...Debugger.Source

I think this ends up being a pretty nice way to relate source code to how it was introduced into the system, whether by 'eval', dynamic <script> element insertion, or what have you, addressing this bug's intentions nicely.
Unfortunately, github diffs are really terrible with long lines. You can clone the repo yourself and look at the 'Debugger.Source' branch versus 'master' using your favorite tools, or visit this and search for Debugger.Source:

https://raw.github.com/jimblandy/DebuggerDocs/Debugger.Source/debug-api
I very much like this, especially the association with DOM elements, which will let us do some cool things in the debugger UI.
(In reply to Kyle Simpson from comment #19)
> Will we be able to have something like this work:
> 
> http://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
> 
> specifically, for injected script elements (where `text` is set, not `src`)?

With the feature as proposed, the debugger will be able to generate names like:

"call to eval at line 10 of text stored as <script> body at line 53 of foo.js"

(or something denser; but the lineage will be there)
Using MD5 hashes to identify specific texts passed to eval or Function or whatever isn't very user-friendly. We'll have distinct Debugger.Source instances for each text, which I think is really what one wants internally, and enough meta-information to generate readable names.
Summary: Scripts should carry detailed information about their origin → Implement Debugger.Source, providing script source code and detailed origin information
Summary: Implement Debugger.Source, providing script source code and detailed origin information → [jsdbg2] Implement Debugger.Source, providing script source code and detailed origin information
Things have been renamed; the link to the branch is now:
https://github.com/jimblandy/DebuggerDocs/blob/Debugger.Source/api
Attachment #749427 - Flags: review?(jimb)
Attachment #749427 - Attachment is obsolete: true
Attachment #749427 - Flags: review?(jimb)
Attachment #749428 - Flags: review?(jimb)
Assignee: nobody → ejpbruel
Improved tests a little bit
Attachment #749428 - Attachment is obsolete: true
Attachment #749428 - Flags: review?(jimb)
Attachment #749502 - Flags: review?(jimb)
Attachment #749503 - Flags: review?(jimb)
Attachment #749503 - Attachment description: Implement Debugger.source.prototype.text → Implement Debugger.Source.prototype.text
Attachment #749995 - Flags: review?(jimb)
The next series of patches implements Debugger.Source.prototype.element in 4 steps:

1. Allow an element to be passed as a compile option
2. Store the element on the script source object
3. Reflect the element property on the debugger object
4. Pass the script element from the browser
Attachment #750595 - Flags: review?(jimb)
After this patch, what is left is:
- enclosingStart
- elementProperty
- introductionScript
- introductionScriptOffset
- introductionKind

I need to figure out about the different ways that script can enter the system (specifically, Worker, importScript and DOM event handlers arent clear to me), before I can continue.
Attachment #750675 - Flags: review?(jimb)
Attachment #750675 - Attachment is patch: true
Attachment #750675 - Attachment mime type: text/x-patch → text/plain
Oh Jim, another thing I haven't figured out yet is how to make Debugger.Source.prototype.element work with IDL event handler attributes. Right now, it only works with script elements.
Comment on attachment 749502 [details] [diff] [review]
Implement Debugger.Script.prototype.source

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

This looks great. Everything that it does is what one would expect to be necessary.

Just a few comments that should be addressed:

::: js/src/jit-test/tests/debug/Script-source-01.js
@@ +1,1 @@
> +// Script.prototype.source should be an object

A little more detail will help people tracking down failures get oriented. Perhaps:

"Script.prototype.source returns the same Source for a script and all its descendant scripts."

@@ +14,5 @@
> +g.eval("2 * 3");
> +g.eval("function f() {}");
> +g.eval("function f() { function g() {} }");
> +g.eval("eval('2 * 3')");
> +g.eval("new Function('2 * 3')");

In tests like these, you need to assert, at the end, that the onNewScript handler fired the expected number of times. Otherwise, the test will pass if onNewScript never fires at all.

Which is, in fact, the case, since you never add g as a debuggee of dbg. :D

::: js/src/jsscript.h
@@ +1135,5 @@
>      }
> +
> +    JSCompartment *compartment() { return compartment_; }
> +
> +    /* The debugger maintains a map of strong references from instances of

You could say "Each Debugger instance maintains...". "The debugger" is kind of vague.

nit: SpiderMonkey style for multi-line comments puts the /* on its own line.

@@ +1139,5 @@
> +    /* The debugger maintains a map of strong references from instances of
> +     * ScriptSource to instances of Debugger.Source. Since each instance of
> +     * Debugger.Source also maintains a strong reference to its corresponding
> +     * ScriptSource, this constitutes a cycle. And since instances of
> +     * ScriptSource are reference counted, this cycle is not detectable by the GC.

Would the following be any better than "And since instances..."?

"We must not include the Debugger.Source's reference in the ScriptSource's main reference count, lest the cycle prevent the reference count from ever reaching zero."

@@ +1144,5 @@
> +     *
> +     * To break this cycle, we remove the entry for an instance of ScriptSource
> +     * from the map when it is no longer reachable from anywhere but instances
> +     * of Debugger.Source. This is safe because we no longer need these entries
> +     * to find the corresponding instances of Debugger.source in that case.

"Debugger.Source" (capitalization)

::: js/src/vm/Debugger.cpp
@@ +413,5 @@
>  }
>  
>  JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGFRAME_OWNER) == unsigned(JSSLOT_DEBUGSCRIPT_OWNER));
>  JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGFRAME_OWNER) == unsigned(JSSLOT_DEBUGOBJECT_OWNER));
>  JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGFRAME_OWNER) == unsigned(JSSLOT_DEBUGENV_OWNER));

Do we need another one of these assertions for JSSLOT_DEBUGSOURCE_OWNER?

@@ +3519,5 @@
> +static inline ScriptSource *
> +GetSourceReferent(JSObject *obj)
> +{
> +    JS_ASSERT(obj->getClass() == &DebuggerSource_class);
> +    return static_cast<ScriptSource *>(obj->getPrivate());

I'm told that nowadays we should be using "private values" (JS::Value::setPrivate/toPrivate/...) stored in another reserved slot (like the "OWNER" we already have), and avoiding obj->getPrivate.

@@ +3525,5 @@
> +
> +static void
> +DebuggerSource_finalize(FreeOp *fop, JSObject *obj)
> +{
> +    /* We need an explicit NULL check here because both instances and the

nit: "/*" on its own line.

@@ +3536,5 @@
> +        source->decdbgref();
> +}
> +
> +Class DebuggerSource_class = {
> +    "Source", JSCLASS_HAS_PRIVATE | JSCLASS_HAS_RESERVED_SLOTS(JSSLOT_DEBUGSOURCE_COUNT),

This needs JSCLASS_IMPLEMENTS_BARRIERS, I think. Terrence says that that flag is appropriate for almost all objects that aren't doing anything too insane, because without it, the GC treats it very carefully. If we do find a problem, we can take it out.

::: js/src/vm/Debugger.h
@@ +356,5 @@
>       */
>      JSObject *newDebuggerScript(JSContext *cx, HandleScript script);
>  
>      /*
> +     * Allocate and initialize a Debugger.Sourcei instance whose referent is

"Sourcei"
Comment on attachment 749503 [details] [diff] [review]
Implement Debugger.Source.prototype.text

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

Looks great!
Attachment #749503 - Flags: review?(jimb) → review+
Comment on attachment 749503 [details] [diff] [review]
Implement Debugger.Source.prototype.text

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

Looks great!

::: js/src/jit-test/tests/debug/Source-text-01.js
@@ +3,5 @@
> +let g = newGlobal('new-compartment');
> +let dbg = new Debugger(g);
> +
> +var count = 0;
> +dbg.onNewScript = function (script, fun) {

onNewScript handlers only get passed one argument. (The spec says it's also passed a global, but that's not implemented.)

::: js/src/jit-test/tests/debug/Source-text-02.js
@@ +9,5 @@
> +    if (count % 2 == 0)
> +        assertEq(script.source.text, text);
> +}
> +
> +g.eval("eval('" + (text = "2 * 3") + "')");

This is fine, but I like to write:

g.eval("eval(" + uneval(text = "2 * 3") + ")")

and let 'uneval' do all the necessary quoting for me.
Comment on attachment 749958 [details] [diff] [review]
Implement Debugger.Script.prototype.sourceStart

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

This looks good, but I think we need more tests before this can land.

We should have:
- Function constructor
- nested eval
- generator expressions
- function with two nested functions
- arrow function
- function statement (our non-standard, non-body-level JSOP_DEFFUN functions)
- getter
- setter

Basically, we should try every syntactic form that generates a child JSScript.

Also, as entire 'eval' arguments:
- an empty script
- a script containing a single expression
- scripts that do and do not end in newlines

Having covered all those cases, it'd be better to test both sourceStart and sourceLength at the same time. Perhaps merge with patch with the sourceLength patch, and just test them both together?

::: js/src/jit-test/tests/debug/Script-sourceStart.js
@@ +2,5 @@
> +
> +let g = newGlobal('new-compartment');
> +let dbg = new Debugger;
> +
> +dbg.onNewScript = function (script) {

This handler never runs, because you never add g as a debuggee.
It looks like a few of the other tests have the handler-never-actually-runs-because-no-debuggee bug.
New patch with comments addressed. I've also added an extra test to make sure that different debuggers get different source objects for the same script.

I will file a followup bug to replace the use of private fields with reserved slots, since we currently do this everywhere in the debugger.
Attachment #749502 - Attachment is obsolete: true
Attachment #749502 - Flags: review?(jimb)
Attachment #751117 - Flags: review?(jimb)
Forgot to do hg qref before attaching the patch. Because, you know, I am an idiot.
Attachment #751117 - Attachment is obsolete: true
Attachment #751117 - Flags: review?(jimb)
Attachment #751120 - Flags: review?(jimb)
(In reply to Eddy Bruel [:ejpbruel] from comment #45)
> I will file a followup bug to replace the use of private fields with
> reserved slots, since we currently do this everywhere in the debugger.

Fair enough.
(In reply to Eddy Bruel [:ejpbruel] from comment #46)
> Forgot to do hg qref before attaching the patch. Because, you know, I am an
> idiot.

You're the first person ever to do that!

(I'm totally going to try out the 'hg bzexport' extension that sfink demonstrated at the work week; he says it will catch errors like this.)
Comment on attachment 751120 [details] [diff] [review]
Implement Debugger.Script.prototype.source (v2)

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

Rah. r=me, with remaining comments fixed.

::: js/src/jit-test/tests/debug/Script-source-01.js
@@ +1,1 @@
> +let g = newGlobal('new-compartment');

I like the comments in-line as you've done, but we do have a strong convention in js/src/jit-test/tests/debug of having an introductory comment on the first line. These tests should follow that.

::: js/src/jit-test/tests/debug/Script-source-02.js
@@ +9,5 @@
> +    // Script.prototype.source should be an object
> +    let source1 = frame.script.source;
> +    assertEq(typeof source1, "object");
> +
> +    // Script.prototype.source should be the same object for each child script

This comment doesn't really describe the code.

::: js/src/jit-test/tests/debug/Script-source-03.js
@@ +11,5 @@
> +dbg2.onNewScript = function (script) {
> +    ++count;
> +
> +    // Script.prototype.source should be a different object for each debugger
> +    assertEq(script.source != source, true);

The order in which Debuggers' hooks are called isn't specified, so dbg2's onNewScript handler might not always be called after dbg1's.

Perhaps you could have a single handler function shared by both, which asks 'if (source)'. Then you could be sure of the test working regardless of the handler order.

::: js/src/jsscript.h
@@ +1150,5 @@
> +     * When the main reference count of a ScriptSource instance reaches zero,
> +     * it is said to be detached. A detached ScriptSource instance no longer has
> +     * any JSScript instances referring to it. This implies that there are no
> +     * longer any Debugger.Script instances from which the Debugger.Source
> +     * instance for that ScriptSource instance can be accessed.

This is great.
Attachment #751120 - Flags: review?(jimb) → review+
Comment on attachment 751120 [details] [diff] [review]
Implement Debugger.Script.prototype.source (v2)

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

::: js/src/jsscript.h
@@ +1102,5 @@
>          jschar *source;
>          unsigned char *compressed;
>      } data;
> +    JSContext *cx_;
> +    JSCompartment *compartment_;

Giving a ScriptSource a compartment is not correct. A ScriptSource does not necessarily live in one compartment and can potentially be runtime wide. When a script is copied across compartments (see CloneScript), its ScriptSource is not copied, so a ScriptSource can be shared by scripts in different compartments.
(In reply to :Benjamin Peterson from comment #50)
> Giving a ScriptSource a compartment is not correct. A ScriptSource does not
> necessarily live in one compartment and can potentially be runtime wide.
> When a script is copied across compartments (see CloneScript), its
> ScriptSource is not copied, so a ScriptSource can be shared by scripts in
> different compartments.

Oh dear.
(In reply to :Benjamin Peterson from comment #50)
> Comment on attachment 751120 [details] [diff] [review]
> Implement Debugger.Script.prototype.source (v2)
> 
> Review of attachment 751120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsscript.h
> @@ +1102,5 @@
> >          jschar *source;
> >          unsigned char *compressed;
> >      } data;
> > +    JSContext *cx_;
> > +    JSCompartment *compartment_;
> 
> Giving a ScriptSource a compartment is not correct. A ScriptSource does not
> necessarily live in one compartment and can potentially be runtime wide.
> When a script is copied across compartments (see CloneScript), its
> ScriptSource is not copied, so a ScriptSource can be shared by scripts in
> different compartments.

If that's the case, I'd like to propose an invariant where ScriptSource instances can only be shared between JSScript instances in the same compartment. This is slightly less efficient memory wise, but we need this invariant to be able to break the cyclic reference between ScriptSource instances and their corresponding Debugger.Source instances.

Not having this invariant would imply a memory leak, unless you could come up with some smart scheme that would avoid creating this cycle in the first place (see the comment the patch for details)
After an IRC chat with benjamin, ejpbruel, and till, we're going to try this approach:

Replace Debugger instances' hash map from ScriptSource pointers to Debugger.Script instances with a js::Vector of Debugger.Script instances stored on each ScriptSource. To find the D.S for a given <ScriptSource, Debugger> pair, we would do a linear search of the ScriptSource's vector. (Usually there are not too many debuggers interested in a single ScriptSource, so this shouldn't be too bad.) The D.S creation method would add the D.S to its ScriptSource's vector, and the D.S finalize method would remove it. The ScriptSource would use the vector's length in place of the second reference count, as the vector is a direct record of all the D.S instances referring to the ScriptSource.
Hey Jim.

I had to move ScriptSource from jsscript.h to jsscriptinlines.h to circumvent a circular header dependency. This makes the patch somewhat hard to read. Sorry about that!

To make it easier, here's what I've added to ScriptSource:
- A typedef SourceMap, which is a hash map from debugger instances to Debugger.Source instances
- A member variable sources, which is an instance of SourceMap
- An init method, which is required to initialise the sources map
- A trace method for the entries in the sources map
- A detach method to remove entries from the sources map
- A big comment in decref to explain how ScriptSource lifetimes work

Everything else is a straightforward copy paste. Hope that helps.

I've also addressed the comments you had regarding the tests in the previous version.
Attachment #751120 - Attachment is obsolete: true
Attachment #751965 - Flags: review?(jimb)
Comment on attachment 749995 [details] [diff] [review]
Implement Debugger.Source.prototype.url

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

Too simple to be wrong!

::: js/src/jit-test/tests/debug/Source-url.js
@@ +5,5 @@
>  var gw = dbg.addDebuggee(g);
>  for (var fileName of ['file:///var/foo.js', null]) {
>      g.evaluate("function f(x) { return 2*x; }", {fileName: fileName});
>      var fw = gw.getOwnPropertyDescriptor('f').value;
> +    print("bla#" + fw.script.source.text);

You probably want to take out the 'print'.
Attachment #749995 - Flags: review?(jimb) → review+
Attached patch Implement ScriptSourceObject (obsolete) — Splinter Review
After a long discussion with jimb and billm, we discovered even more issues with the current approach. The root of the problem seems to be that ScriptSource is not a gcthing.

To solve this, we're going to introduce a new JSObject, ScriptSourceObject, which acts as a container for instances of ScriptSource.

It's still possible for multiple ScriptSourceObject instances to share the same ScriptSource instance, but unlike ScriptSource instances, each ScriptSourceObject instance will live in the same compartment as its associated JSSCript.

This patch introduces ScriptSourceObject itself. The next patch will rewrite JSScript and its consumers to use ScriptSourceObject instead of ScriptSource
Attachment #752523 - Flags: review?(jimb)
Comment on attachment 752523 [details] [diff] [review]
Implement ScriptSourceObject

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

Simple and beautiful.

::: js/src/jsscript.h
@@ +1093,5 @@
> +        if (this->source())
> +            this->source()->decref();
> +        setReservedSlot(SOURCE_SLOT, PrivateValue(source));
> +        if (this->source())
> +            this->source()->incref();

There's a classic kink of reference counting here: when handling an assignment to an owning pointer like this, always *increment* the new value's refcount first, and then *decrement* the old value's refcount. This ensures that, if the old and new values happen to be the same object, the code doesn't accidentally free it.
Attachment #752523 - Flags: review?(jimb) → review+
Comment on attachment 752525 [details] [diff] [review]
Use ScriptSourceObject instead of ScriptSource

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

::: js/src/jsscript.cpp
@@ +1711,5 @@
> +        if (!sourceObject)
> +            return NULL;
> +
> +        script->setSourceObject(sourceObject);
> +    }

I don't think this is right. The ScriptSourceObject needs to be created when we create a ScriptSource, not along with the Script. Each compartment should have only one ScriptSourceObject per ScriptSource. (We can deal with cloning later.)

If we create a ScriptSourceObject per JSScript like this, then using the SSOs as keys in the Debugger's weak map will lead to different Debugger.Source instances for different scripts in the same compilation unit.

::: js/src/jsscript.h
@@ +633,1 @@
>      }

Would it work to simply retain JSScript::scriptSource, with a definition like:

js::ScriptSource *scriptSource() const {
  return sourceObject()->source();
}

It seems like a lot of the uses could remain unchanged, then. Most of this patch would go away! :)

(Naturally, JSScript::setScriptSource needs to change.)

::: js/src/vm/GlobalObject.cpp
@@ +239,5 @@
>                                                   /* savedCallerFun = */ false,
>                                                   options,
>                                                   /* staticLevel = */ 0,
> +                                                 NULL,
> +                                                 0, 0));

I talked with jorendorff, and he was of the opinion that we don't really have a circular dependency here; for example, further up in this function, we create an object by calling NewObjectWithGivenProto. Couldn't we do that here?

We probably don't want to have exactly one JSScript running around without a source...
Attachment #752525 - Flags: review?(jimb)
Only minor changes. Fixed the refcount issue, added a typedef HandleScriptSource, and used NewObjectWithGivenProto as suggested by jorendorff.
Attachment #752523 - Attachment is obsolete: true
Attachment #752945 - Flags: review?
Attachment #752945 - Flags: review? → review?(jimb)
New patch with comments by jimb addressed.
Attachment #752525 - Attachment is obsolete: true
Attachment #752947 - Flags: review?(jimb)
Comment on attachment 752945 [details] [diff] [review]
Implement ScriptSourceObject (v2)

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

Except for the missing null check, looks great!

::: js/src/jsscript.h
@@ +1089,5 @@
> +        return static_cast<ScriptSource *>(getReservedSlot(SOURCE_SLOT).toPrivate());
> +    }
> +
> +    void setSource(ScriptSource *source) {
> +        source->incref();

source can be null, right? (It is in ScriptSourceObject::finalize.)
Attachment #752945 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #62)
> source can be null, right? (It is in ScriptSourceObject::finalize.)

Oh, I see, you fixed this in the next patch, just forgot to move it around in your queue. That's fine.
Comment on attachment 752947 [details] [diff] [review]
Use ScriptSourceObject instead of ScriptSource (v2)

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

This looks good.

::: js/src/jsscript.h
@@ +628,5 @@
>      JSFlatString *sourceData(JSContext *cx);
>  
>      static bool loadSource(JSContext *cx, js::HandleScript scr, bool *worked);
>  
> +    js::ScriptSource *scriptSource() const;

If we mark this 'inline', then we can get complaints when someone uses it without #including jsscriptinlines.h. I think.
Attachment #752947 - Flags: review?(jimb) → review+
Latest version, based on the use of ScriptSourceObject
Attachment #751965 - Attachment is obsolete: true
Attachment #751965 - Flags: review?(jimb)
Attachment #752978 - Flags: review?(jimb)
billm pointed out on irc that we need to do some special handling related to GC'ing for the referent -> Debugger.Source instance map. New patch with comments addressed.
Attachment #752978 - Attachment is obsolete: true
Attachment #752978 - Flags: review?(jimb)
Attachment #752993 - Flags: review?(jimb)
Comment on attachment 752993 [details] [diff] [review]
Implement Debugger.Script.prototype.source (v5)

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

Looks good.

We need to update the comment above Debugger::markCrossCompartmentDebuggerObjectReferents, too.

::: js/src/jit-test/tests/debug/Script-source-01.js
@@ +9,5 @@
> +dbg.onNewScript = function (script) {
> +    ++count;
> +    assertEq(typeof script.source, "object");
> +    script.getChildScripts().forEach(function (childScript) {
> +        assertEq(childScript.source, script.source);

I think this loop won't reach the 'g' nested within 'f', because that's a child of a child. How about placing the '++count' on this innermost 'assertEq'?
Attachment #752993 - Flags: review?(jimb) → review+
Attachment #749503 - Attachment is obsolete: true
Attachment #753035 - Flags: review?(jimb)
Merged into one patch and with MOAR tests as requested
Attachment #749958 - Attachment is obsolete: true
Attachment #749972 - Attachment is obsolete: true
Attachment #749958 - Flags: review?(jimb)
Attachment #749972 - Flags: review?(jimb)
Attachment #753039 - Flags: review?
Attachment #753039 - Flags: review? → review?(jimb)
Comment on attachment 753035 [details] [diff] [review]
Implement Debugger.Source.prototype.text (v2)

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

This looks good; it needs some more tests:

- Function with an empty body
- text passed to 'evaluate', which evaluates stuff as 'global code'
- try applying Debugger.Source.prototype.text to:
  - a non-object
  - a non-Debugger.Source instance
  - Debugger.Source.prototype itself
- try evaluating exactly the same text twice; verify that we hit the eval cache (i.e. we get the same Debugger.Script instance), and then check the Debugger.Source instance

Also: are there any JSScript instances that have no source? If so, we need to test applying Debugger.Script.prototype.source to those.

::: js/src/vm/Debugger.cpp
@@ +3509,5 @@
> +static inline ScriptSourceObject *
> +GetSourceReferent(JSObject *obj)
> +{
> +    JS_ASSERT(obj->getClass() == &DebuggerSource_class);
> +    return static_cast<ScriptSourceObject *>(obj->getPrivate());

Could this just use asScriptSource?
Comment on attachment 753039 [details] [diff] [review]
Implement Debugger.Script.prototype.sourceStart/Length (v2)

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

::: js/src/jit-test/tests/debug/Script-sourceStart-03.js
@@ +27,5 @@
> +test("() => {}", [[0, 8]]);
> +test("(x, y) => { x * y }", [[0, 19]]);
> +test("x => x * x", [[0, 10]]);
> +test("x => x => x * x", [[0, 15], [5, 10]]);
> +test("x => x => x => x * x", [[0, 20], [5, 15], [10, 10]]);

I love it.
Attachment #753039 - Flags: review?(jimb) → review+
> - try applying Debugger.Source.prototype.text to:
>   - a non-object
>   - a non-Debugger.Source instance
>   - Debugger.Source.prototype itself

These are going to be common to all the inherited accessors; it'd probably make sense to just create jit-test/tests/debug/Source-surfaces.js and add things to that as we go.
Comment on attachment 750675 [details] [diff] [review]
Implement Debugger.Source.prototype.lineCount

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +213,5 @@
>  
>          parser.handler.freeTree(pn);
>      }
>  
> +    ss->setLineCount(parser.tokenStream.getLineno() - parser.tokenStream.getLinestart() + 1);

I think we need to check whether there were characters after the last newline here; see my comment on the Source-lineCount.js test.

::: js/src/jit-test/tests/debug/Source-lineCount.js
@@ +18,5 @@
> +    g.eval(str);
> +}
> +
> +test("2 * 3", 1);
> +test("function f() {}\n", 2);

This should be '1'. From the spec:

"If there are characters after the last newline in the source code, those count as a final line; otherwise, <code>lineCount</code> is equal to the number of newlines in the source code."
New patch with comments addressed.

The last test you described has nothing to do with testing Debugger.Source.prototype.text, and we already have a similar test for it in the patch for Debugger.Script.prototype.source, so I skipped over that.

We cannot use asScriptSource on that line because toPrivate doesn't return a JSObject *, so I left that code as is.
Attachment #753035 - Attachment is obsolete: true
Attachment #753035 - Flags: review?(jimb)
Attachment #753388 - Flags: review?(jimb)
Essentially the same patch you r+'d before, but with some changes due to the fact that we now use ScriptSourceObject. Just needs a quick look.
Attachment #749995 - Attachment is obsolete: true
Attachment #753470 - Flags: review?(jimb)
Rebase of the previous patch
Attachment #750595 - Attachment is obsolete: true
Attachment #750596 - Attachment is obsolete: true
Attachment #750597 - Attachment is obsolete: true
Attachment #750600 - Attachment is obsolete: true
Attachment #750595 - Flags: review?(jimb)
Attachment #750596 - Flags: review?(jimb)
Attachment #750597 - Flags: review?(jimb)
Attachment #750600 - Flags: review?(jimb)
Attachment #753471 - Flags: review?(jimb)
Rebase of the previous patch
Attachment #753472 - Flags: review?(jimb)
Attachment #753471 - Attachment description: Implement Debugger.Source.prototype.element (1/4) (v2) → Implement Debugger.Source.prototype.element (1/3) (v2)
Attachment #753472 - Attachment description: Implement Debugger.Source.prototype.element (2/4) (v3) → Implement Debugger.Source.prototype.element (2/3) (v2)
Rebase of the previous patch
Attachment #753480 - Flags: review?(jimb)
Comment on attachment 753388 [details] [diff] [review]
Implement Debugger.Source.prototype.text (v3)

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

This looks great. Some minor comments:

::: js/src/vm/Debugger.cpp
@@ +3513,5 @@
> +    return static_cast<ScriptSourceObject *>(obj->getPrivate());
> +}
> +
> +static inline void
> +SetScriptReferent(JSObject *obj, ScriptSourceObject *source)

Will we ever need this? For all our other Debugger.Foo types, the referent can't be changed.

@@ +3612,5 @@
> +                             "Debugger.Source", fnname, thisobj->getClass()->name);
> +        return NULL;
> +    }
> +
> +    if (!thisobj->getPrivate()) {

Can we use GetSourceReferent here?

@@ +3628,5 @@
> +    if (!obj)                                                                       \
> +        return false;                                                               \
> +    JS::RootedScriptSource sourceObject(cx, GetSourceReferent(obj));                \
> +    if (!sourceObject)                                                              \
> +        return false;

I think this 'if' could be an assertion, or just omitted, as DebuggerSource_checkThis should always return false if the referent is NULL.

@@ +3636,5 @@
> +{
> +    THIS_DEBUGSOURCE_REFERENT(cx, argc, vp, "(get text)", args, obj, sourceObject);
> +
> +    ScriptSource *ss = sourceObject->source();
> +    JSString *str = ss->substring(cx, 0, ss->length());

The copies here irk me, but I don't think there's anything we can do about it for now.
Attachment #753388 - Flags: review?(jimb) → review+
Attachment #753470 - Flags: review?(jimb) → review+
Just as a reminder: these patches only implement the back-end part of these properties. The front-end part, where we push this information from the browser into the engine, will come in a later patch.
Attachment #753512 - Flags: review?(jimb)
Comment on attachment 753471 [details] [diff] [review]
Implement Debugger.Source.prototype.element (1/3) (v2)

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

Looks good to me.

::: js/src/shell/js.cpp
@@ +959,5 @@
>          }
>  
> +        if (!JS_GetProperty(cx, opts, "element", v.address()))
> +            return false;
> +        if (!JSVAL_IS_PRIMITIVE(v))

This silently skips 'element' properties if they're primitives. I think we want a JSVAL_IS_VOID check here, like the other cases, and then a call to something like js::NonNullObject (which sets the error for you).
Attachment #753471 - Flags: review?(jimb) → review+
Debugger.Source.prototype.element should return an Debugger.Object instance with the element as its referent, not the element itself.
Attachment #753480 - Attachment is obsolete: true
Attachment #753480 - Flags: review?(jimb)
Attachment #753960 - Flags: review?(jimb)
Comment on attachment 753472 [details] [diff] [review]
Implement Debugger.Source.prototype.element (2/3) (v2)

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

This looks good to me; holding review pending ongoing IRC conversation about references to JS objects from ScriptSource.

::: js/src/jsscript.cpp
@@ +913,3 @@
>  Class js::ScriptSourceClass = {
>      "ScriptSource",
>      JSCLASS_HAS_RESERVED_SLOTS(1) | JSCLASS_IS_ANONYMOUS,

I should have caught this in a prior review, but: shouldn't this include JSCLASS_IMPLEMENTS_BARRIERS?
To summarize the IRC conversation with billm:

A source's owning element (and any other gcthings we want to get at from Debugger.Script instances) need to be stored on the ScriptSourceObject, not on the ScriptSource. If it is stored on the ScriptSource, we will not always be able to find it as a root when we need to.

Suppose we store the element reference in the ScriptSource. Here's how things could go awry:

We create a <script> element with a JS reflector, a JSScript, a ScriptSource, and a ScriptSourceObject. The three gcthings in that list are all in one zone.

Then we clone the JSScript into another zone. This clones the ScriptSourceObject along with it; there are now two SSO's pointing to our ScriptSource, from two different zones.

Now we do a GC of the original zone only, in which the original zone's JSScript and ScriptSourceObject are not reachable.

At this point, the ScriptSource is still pointing to the <script> element's JS reflector in the original zone, but that ScriptSource is not reachable from anything within the zone, so tracing will never find the ScriptSource's reference to the reflector, and the reflector will be collected.

Essentially, the path from the cloned ScriptSourceObject through the ScriptSource to the <script> reflector is a cross-zone reference that is never registered in any of the usual places, so collections of that zone only can't find the incoming reference.

If we instead store the <script> reflector reference in ScriptSourceObjects, then cloning a ScriptSourceObject will necessarily store, in the clone's slot, a cross-compartment wrapper referring to the <script> reflector. This is a cross-zone reference that the single-zone GC can find.
Comment on attachment 753472 [details] [diff] [review]
Implement Debugger.Source.prototype.element (2/3) (v2)

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

This looks good to me; holding review pending ongoing IRC conversation about references to JS objects from ScriptSource.

::: js/src/jsscript.cpp
@@ +913,3 @@
>  Class js::ScriptSourceClass = {
>      "ScriptSource",
>      JSCLASS_HAS_RESERVED_SLOTS(1) | JSCLASS_IS_ANONYMOUS,

I should have caught this in a prior review, but: shouldn't this include JSCLASS_IMPLEMENTS_BARRIERS?
Attachment #753472 - Flags: review?(jimb)
I don't know why Splinter is so eager to re-post my reviews. I just meant to clear the review flag...
Moved the element property to the script source object as per discussion on irc
Attachment #753472 - Attachment is obsolete: true
Attachment #754029 - Flags: review?(jimb)
Rebase to account for changes in the previous patch
Attachment #753960 - Attachment is obsolete: true
Attachment #753960 - Flags: review?(jimb)
Attachment #754030 - Flags: review?(jimb)
Adapted this patch to store elementProperty on the ScriptSourceObject as well.
Attachment #753512 - Attachment is obsolete: true
Attachment #753512 - Flags: review?(jimb)
Attachment #754961 - Flags: review?(jimb)
Fix for a minor issue with one of the tests introduced by a previous push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4579d2f68430
After a talk with Terrence on irc, we figured out that we should store a *wrapper* to the original script source object when cloning the script, rather than cloning the script source object as well and wrap on a property by property basis. The latter cannot work, because certain types of properties, such as JSScript *, are inherently not wrappable.
Attachment #754029 - Attachment is obsolete: true
Attachment #754029 - Flags: review?(jimb)
Attachment #755201 - Flags: review?(jimb)
Jim and I decided to store elementProperty on a private slot and trace it explicitly, because string values are not allowed to be NULL, and because we will need to do explicit tracing for introductionScript anyway.

Also added some surface tests.
Attachment #754030 - Attachment is obsolete: true
Attachment #754030 - Flags: review?(jimb)
Attachment #755204 - Flags: review?(jimb)
(In reply to Eddy Bruel [:ejpbruel] from comment #101)
> Created attachment 755204 [details] [diff] [review]
> Implement Debugger.Source.prototype.element (3/3) (v5)
> 
> Jim and I decided to store elementProperty on a private slot and trace it
> explicitly, because string values are not allowed to be NULL, and because we
> will need to do explicit tracing for introductionScript anyway.
> 
> Also added some surface tests.

Right, this comment was meant for the next patch. Sorry.
Attachment #754961 - Attachment is obsolete: true
Attachment #754961 - Flags: review?(jimb)
Attachment #755205 - Flags: review?(jimb)
Comment on attachment 755201 [details] [diff] [review]
Implement Debugger.Source.prototype.element (2/3) (v4)

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

For reasons I cannot fathom, this patch as it stands breaks running chrome mochitests with mach:

./mach mochitest-chrome

The test runner is not to find any of the tests, instead referring me to:

http://www.mochi.test:8888/redirect.html

which leads to a Server not found error.

I have NO idea what's going on here. All the JS tests pass handsomely.
Any idea you could have would be appreciated!
Ah, so, using this wonderful --tbpl flag that Terrence showed me, I figured out my patch actually breaks something in IonMonkey. Somehow, we get into an infinite recursion somewhere.
(In reply to Jim Blandy :jimb from comment #85)
> This silently skips 'element' properties if they're primitives. I think we
> want a JSVAL_IS_VOID check here

Why are we using JSVAL_IS_* methods at all?  We should be using Value::isUndefined() and all the nice built-into-Value methods, and indeed other bugs are actively converting existing code over to those methods.
Depends on: 877995
Depends on: 878000
Attachment #750675 - Flags: review?(jimb)
Comment on attachment 755204 [details] [diff] [review]
Implement Debugger.Source.prototype.element (3/3) (v5)

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

This should be fine, with some changes made:

::: js/src/jit-test/tests/debug/Source-element.js
@@ +9,5 @@
> +assertEq(fw.script.source.element instanceof Debugger.Object, true);
> +assertEq(fw.script.source.element.getOwnPropertyDescriptor("foo").value, "bar");
> +g.evaluate("function f(x) { return 2*x; }");
> +var fw = gw.getOwnPropertyDescriptor('f').value;
> +assertEq(typeof fw.script.source.element, "undefined");

This has been a challenge to get right; let's exercise all the angles:

// Source may not belong to any element.
g.evaluate("function f(x) { return 2*x; }");
var fw = gw.getOwnPropertyDescriptor('f').value;
assertEq(fw.script.source.element, undefined);

// The element to which a given source belongs may be either in the same
// compartment as the source, or in a different compartment; and we may
// pass the element either as a direct reference, or as a cross-compartment
// wrapper. Try all four combinations.

g.eval("let near = { foo: 'near' };");
var far = { foo: 'far' };

var nearW = gw.getOwnPropertyDescriptor('near').value;
var farW = gw.makeDebuggeeValue(far);
g.far = far;

// Source and element in same compartment.
// Specify element as direct reference.
g.eval("evaluate('function f() {}', {element: near});");
var fw = gw.getOwnPropertyDescriptor('f').value;
assertEq(fw.script.source.element, nearW);

// Source and element in same compartment.
// Specify element as cross-compartment wrapper.
g.evaluate('function f() {}', {element: g.near});
var fw = gw.getOwnPropertyDescriptor('f').value;
assertEq(fw.script.source.element, nearW);

// Source and element in different compartments.
// Specify element as direct reference.
g.evaluate("function f() {}", {element: far});
var fw = gw.getOwnPropertyDescriptor('f').value;
assertEq(fw.script.source.element, farW);

// Source and element in different compartments.
// Specify element as cross-compartment wrapper.
g.eval("evaluate('function f() {}', {element: g.far});");
var fw = gw.getOwnPropertyDescriptor('f').value;
assertEq(fw.script.source.element, farW);

::: js/src/vm/Debugger.cpp
@@ +3682,5 @@
> +    if (sourceObject->element()) {
> +        RootedValue v(cx, ObjectValue(*sourceObject->element()));
> +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, &v))
> +            return false;
> +        args.rval().set(v);

I think you can use rval() as the temporary storage:

args.rval().set(ObjectValue(*sourceObject->element()));
if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, args.rval()))
  return false;
Attachment #755204 - Flags: review?(jimb) → review+
Essentially the same patch, but with two fixes in the jsapi-tests to avoid false positives for our recursion check.
Attachment #755201 - Attachment is obsolete: true
Attachment #755201 - Flags: review?(jimb)
Attachment #756762 - Flags: review?(jimb)
Essentially the same patch, with a minor bug fix in jsgc.cpp added
Attachment #755205 - Attachment is obsolete: true
Attachment #755205 - Flags: review?(jimb)
Attachment #756763 - Flags: review?(jimb)
Depends on: 878287
Comment on attachment 755205 [details] [diff] [review]
Implement Debugger.Source.prototype.elementProperty (v3)

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

Things would be much simpler if we simply stored the string as a StringValue in the slot. We wouldn't need any 'trace' function yet (although I understand that we'll need a trace function for introductionScript anyway).

Since all property names are jsids, it should be no proble for CompileOptions to take a HandleId instead of a HandleString, and then everything else gets simpler. Because all ids live in their own special compartment, with special rules, it's fine to have JS::Values that are cross-compartment references to jsids. So there's no need for wrapping or casting back and forth to private values.

::: js/src/jsapi.h
@@ +3894,5 @@
>      bool utf8;
>      const char *filename;
>      unsigned lineno;
>      HandleObject element;
> +    HandleString elementProperty;

This should probably be HandleId, not HandleString.

@@ +3914,5 @@
>      CompileOptions &setFileAndLine(const char *f, unsigned l) {
>          filename = f; lineno = l; return *this;
>      }
>      CompileOptions &setElement(HandleObject e) { element = e; return *this; }
> +    CompileOptions &setElementProperty(HandleString p) { elementProperty = p; return *this; }

HandleId here, too.

::: js/src/jsscript.cpp
@@ +908,5 @@
> +{
> +    ScriptSourceObject *sourceObject = &obj->asScriptSource();
> +    if (sourceObject->elementProperty())
> +        MarkSlot(trc, &obj->getReservedSlotRef(ELEMENT_PROPERTY_SLOT), "elementProperty");
> +}

If we store the element property name as a StringValue, we don't need this yet.

@@ +925,5 @@
> +    NULL,                           /* checkAccess */
> +    NULL,                           /* call        */
> +    NULL,                           /* construct   */
> +    NULL,                           /* hasInstance */
> +    ScriptSourceObject::trace

Nor this.

@@ +931,5 @@
>  
>  ScriptSourceObject *
>  ScriptSourceObject::create(JSContext *cx, ScriptSource *source, CompileOptions options)
>  {
>      RootedObject object(cx, NewObjectWithGivenProto(cx, &ScriptSourceClass, NULL, cx->global()));

This needs something like:

assertSameCompartment(cx, options.element, options.elementProperty);

@@ +938,5 @@
>      JS::RootedScriptSource sourceObject(cx, &object->asScriptSource());
>      source->incref();
>      sourceObject->setSlot(SOURCE_SLOT, PrivateValue(source));
>      sourceObject->setSlot(ELEMENT_SLOT, ObjectOrNullValue(options.element.get()));
> +    sourceObject->setSlot(ELEMENT_PROPERTY_SLOT, PrivateValue(options.elementProperty.get()));

This should just be: ... StringValue(options.elementProperty.get()) ...

::: js/src/jsscript.h
@@ +1085,5 @@
>  
>  class ScriptSourceObject : public JSObject {
>    public:
>      static void finalize(FreeOp *fop, JSObject *obj);
> +    static void trace(JSTracer *trc, JSObject *obj);

This can go.

@@ +1110,5 @@
>          setReservedSlot(ELEMENT_SLOT, ObjectOrNullValue(element));
>      }
>  
> +    JSString *elementProperty() {
> +        return static_cast<JSString *>(getReservedSlot(ELEMENT_PROPERTY_SLOT).toPrivate());

This can simply be: return getReservedSlot(ELEMENT_PROPERTY_SLOT).toString();

@@ +1114,5 @@
> +        return static_cast<JSString *>(getReservedSlot(ELEMENT_PROPERTY_SLOT).toPrivate());
> +    }
> +
> +    void setElementProperty(JSString *elementProperty) {
> +        setReservedSlot(ELEMENT_PROPERTY_SLOT, PrivateValue(elementProperty));

'StringValue'

@@ +1121,4 @@
>    private:
>      static const uint32_t SOURCE_SLOT = 0;
>      static const uint32_t ELEMENT_SLOT = 1;
> +    static const uint32_t ELEMENT_PROPERTY_SLOT = 2;

At this point, you probably want a static const RESERVED_SLOTS that you can cite in the Class initializer.

::: js/src/shell/js.cpp
@@ +926,5 @@
>      bool compileAndGo = true;
>      bool noScriptRval = false;
>      const char *fileName = "@evaluate";
>      RootedObject element(cx);
> +    RootedString elementProperty(cx);

This should be RootedId .

@@ +991,5 @@
>  
> +        if (!JS_GetProperty(cx, opts, "elementProperty", v.address()))
> +            return false;
> +        if (!JSVAL_IS_VOID(v)) {
> +            elementProperty = JS_ValueToString(cx, v);

You probably want JS_ValueToId here.

::: js/src/vm/Debugger.cpp
@@ +3697,5 @@
> +    if (sourceObject->elementProperty()) {
> +        RootedValue v(cx, StringValue(sourceObject->elementProperty()));
> +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, &v))
> +            return false;
> +        args.rval().set(v);

This whole 'then' block can simply be:

{
  args.rval().setString(sourceObject->elementProperty());
  JS_ASSERT(args.rval().toString()->isAtom());
}

(Maybe there's a 'get' needed in there?) But as long as it's an atom, there's no need to rewrap.
Attachment #755205 - Attachment is obsolete: false
Attachment #755205 - Attachment is obsolete: true
(In reply to Jim Blandy :jimb from comment #110)
> Comment on attachment 755205 [details] [diff] [review]
> Implement Debugger.Source.prototype.elementProperty (v3)
> 
> Review of attachment 755205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Things would be much simpler if we simply stored the string as a StringValue
> in the slot. We wouldn't need any 'trace' function yet (although I
> understand that we'll need a trace function for introductionScript anyway).
> 
> Since all property names are jsids, it should be no proble for
> CompileOptions to take a HandleId instead of a HandleString, and then
> everything else gets simpler. Because all ids live in their own special
> compartment, with special rules, it's fine to have JS::Values that are
> cross-compartment references to jsids. So there's no need for wrapping or
> casting back and forth to private values.
> 
> ::: js/src/jsapi.h
> @@ +3894,5 @@
> >      bool utf8;
> >      const char *filename;
> >      unsigned lineno;
> >      HandleObject element;
> > +    HandleString elementProperty;
> 
> This should probably be HandleId, not HandleString.
> 
> @@ +3914,5 @@
> >      CompileOptions &setFileAndLine(const char *f, unsigned l) {
> >          filename = f; lineno = l; return *this;
> >      }
> >      CompileOptions &setElement(HandleObject e) { element = e; return *this; }
> > +    CompileOptions &setElementProperty(HandleString p) { elementProperty = p; return *this; }
> 
> HandleId here, too.
> 
> ::: js/src/jsscript.cpp
> @@ +908,5 @@
> > +{
> > +    ScriptSourceObject *sourceObject = &obj->asScriptSource();
> > +    if (sourceObject->elementProperty())
> > +        MarkSlot(trc, &obj->getReservedSlotRef(ELEMENT_PROPERTY_SLOT), "elementProperty");
> > +}
> 
> If we store the element property name as a StringValue, we don't need this
> yet.
> 
> @@ +925,5 @@
> > +    NULL,                           /* checkAccess */
> > +    NULL,                           /* call        */
> > +    NULL,                           /* construct   */
> > +    NULL,                           /* hasInstance */
> > +    ScriptSourceObject::trace
> 
> Nor this.
> 
> @@ +931,5 @@
> >  
> >  ScriptSourceObject *
> >  ScriptSourceObject::create(JSContext *cx, ScriptSource *source, CompileOptions options)
> >  {
> >      RootedObject object(cx, NewObjectWithGivenProto(cx, &ScriptSourceClass, NULL, cx->global()));
> 
> This needs something like:
> 
> assertSameCompartment(cx, options.element, options.elementProperty);
> 
> @@ +938,5 @@
> >      JS::RootedScriptSource sourceObject(cx, &object->asScriptSource());
> >      source->incref();
> >      sourceObject->setSlot(SOURCE_SLOT, PrivateValue(source));
> >      sourceObject->setSlot(ELEMENT_SLOT, ObjectOrNullValue(options.element.get()));
> > +    sourceObject->setSlot(ELEMENT_PROPERTY_SLOT, PrivateValue(options.elementProperty.get()));
> 
> This should just be: ... StringValue(options.elementProperty.get()) ...
> 
> ::: js/src/jsscript.h
> @@ +1085,5 @@
> >  
> >  class ScriptSourceObject : public JSObject {
> >    public:
> >      static void finalize(FreeOp *fop, JSObject *obj);
> > +    static void trace(JSTracer *trc, JSObject *obj);
> 
> This can go.
> 
> @@ +1110,5 @@
> >          setReservedSlot(ELEMENT_SLOT, ObjectOrNullValue(element));
> >      }
> >  
> > +    JSString *elementProperty() {
> > +        return static_cast<JSString *>(getReservedSlot(ELEMENT_PROPERTY_SLOT).toPrivate());
> 
> This can simply be: return getReservedSlot(ELEMENT_PROPERTY_SLOT).toString();
> 
> @@ +1114,5 @@
> > +        return static_cast<JSString *>(getReservedSlot(ELEMENT_PROPERTY_SLOT).toPrivate());
> > +    }
> > +
> > +    void setElementProperty(JSString *elementProperty) {
> > +        setReservedSlot(ELEMENT_PROPERTY_SLOT, PrivateValue(elementProperty));
> 
> 'StringValue'
> 
> @@ +1121,4 @@
> >    private:
> >      static const uint32_t SOURCE_SLOT = 0;
> >      static const uint32_t ELEMENT_SLOT = 1;
> > +    static const uint32_t ELEMENT_PROPERTY_SLOT = 2;
> 
> At this point, you probably want a static const RESERVED_SLOTS that you can
> cite in the Class initializer.
> 
> ::: js/src/shell/js.cpp
> @@ +926,5 @@
> >      bool compileAndGo = true;
> >      bool noScriptRval = false;
> >      const char *fileName = "@evaluate";
> >      RootedObject element(cx);
> > +    RootedString elementProperty(cx);
> 
> This should be RootedId .
> 
> @@ +991,5 @@
> >  
> > +        if (!JS_GetProperty(cx, opts, "elementProperty", v.address()))
> > +            return false;
> > +        if (!JSVAL_IS_VOID(v)) {
> > +            elementProperty = JS_ValueToString(cx, v);
> 
> You probably want JS_ValueToId here.
> 
> ::: js/src/vm/Debugger.cpp
> @@ +3697,5 @@
> > +    if (sourceObject->elementProperty()) {
> > +        RootedValue v(cx, StringValue(sourceObject->elementProperty()));
> > +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, &v))
> > +            return false;
> > +        args.rval().set(v);
> 
> This whole 'then' block can simply be:
> 
> {
>   args.rval().setString(sourceObject->elementProperty());
>   JS_ASSERT(args.rval().toString()->isAtom());
> }
> 
> (Maybe there's a 'get' needed in there?) But as long as it's an atom,
> there's no need to rewrap.

I tried to refactor this the way you suggested, but there's one critical problem with it. Unlike HandleString, HandleId cannot be NULL, and for most scripts, it should indeed be just that.

I tried adding a flag that indicates whether CompileOption has a HandleId, but that kind of defeats the point of simplifying the API (and still leaves you with the problem of coming up with a default value for the HandleId). You argued that there wasn't a good reason why this should be a HandleString, since all properties are Ids, but unlike most properties, this one can be NULL. The fact that it can be NULL seems like a good reason to me to leave this as is.

I don't want to act unilaterally here, but since I'm leaving on PTO tomorrow, I want to leave the patch in a working state at least. I'll keep this as is for now, unless you have a good suggestion before I leave :-)
Comment on attachment 755204 [details] [diff] [review]
Implement Debugger.Source.prototype.element (3/3) (v5)

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

::: js/src/vm/Debugger.cpp
@@ +3683,5 @@
> +        RootedValue v(cx, ObjectValue(*sourceObject->element()));
> +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, &v))
> +            return false;
> +        args.rval().set(v);
> +    } else

I forgot --- SpiderMonkey style requires braces around all 'if' branches if any 'if' branch has braces.
Comment on attachment 756762 [details] [diff] [review]
Implement Debugger.Source.prototype.element (2/3) (v5)

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

Looks good. Some comments; one necessary fix (with a corresponding test needed); one follow-up bug.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +337,5 @@
>                                                    lazy->strict());
>      if (!pn)
>          return false;
>  
> +    JS::RootedScriptSource sourceObject(cx, ScriptSourceObject::create(cx, lazy->source(), options));

We need to make sure this CompileOptions is properly initialized with the element from lazy->parent. This will apply to pretty much every CompileOptions field we add.

We also need a test that fails until this fix is made. :)

::: js/src/jsscript.h
@@ +635,5 @@
>  
>      js::ScriptSource *scriptSource() const;
>  
>      js::ScriptSourceObject *sourceObject() const {
> +        return &UncheckedUnwrap(sourceObject_)->asScriptSource();;

(Double semicolon.)

This UncheckedUnwrap makes me uncomfortable, but I've checked all its uses and they seem fine. But I do think there's a better way; I've filed a follow-up, bug 878484.

@@ +1096,5 @@
>  class ScriptSourceObject : public JSObject {
>    public:
>      static void finalize(FreeOp *fop, JSObject *obj);
> +
> +    static ScriptSourceObject *create(JSContext *cx, ScriptSource *source, CompileOptions options);

Could we pass CompileOptions by const reference here?

@@ +1116,5 @@
> +    };
> +
> +    void setElement(JSObject *element) {
> +        setReservedSlot(ELEMENT_SLOT, ObjectOrNullValue(element));
> +    }

I don't think these accessors really add much value, especially since in Debugger.Source.prototype.element we're going to do the exact inverse of the type conversions here: "if null, NullValue else ObjectValue". But it's fine for now.
Attachment #756762 - Flags: review?(jimb) → review+
Depends on: 878484
Final patch before I leave on PTO. I've merged (2/3) and (3/3) into a single patch, addressed your comments, and added your tests.
Attachment #755204 - Attachment is obsolete: true
Attachment #756762 - Attachment is obsolete: true
Attachment #757028 - Flags: review?(jimb)
Final patch before I leave on PTO.

I wasn't able to do the refactor you suggested, for reasons detailed above. I wanted to leave you with a working patch before I left, so I kept things the way they were.

Personally, I'd like to see this land as is. However, the intention was not to go behind your back on this, so if you still feel we should use HandleIds and can come up with a way to make it work by all means go for it (or I can do it when I get back, whatever works for you :-)).
Attachment #756763 - Attachment is obsolete: true
Attachment #756763 - Flags: review?(jimb)
Attachment #757029 - Flags: review?(jimb)
(In reply to Eddy Bruel [:ejpbruel] from comment #111)
> I tried to refactor this the way you suggested, but there's one critical
> problem with it. Unlike HandleString, HandleId cannot be NULL, and for most
> scripts, it should indeed be just that.

Oh, I didn't realize that. HandleString it is, then.

> I tried adding a flag that indicates whether CompileOption has a HandleId,
> but that kind of defeats the point of simplifying the API (and still leaves
> you with the problem of coming up with a default value for the HandleId).
> You argued that there wasn't a good reason why this should be a
> HandleString, since all properties are Ids, but unlike most properties, this
> one can be NULL. The fact that it can be NULL seems like a good reason to me
> to leave this as is.

Yes, I agree.

Some suggestions: 

The 'static const RESERVED_SLOTS' suggestion still seems like a good idea.

Given that we're going to need an 'if' to turn the NULL into null, we might as well just store exactly the value that Debugger.Source.prototype.elementProperty is going to return in the SSO's slot. Object slots can't really be typed, and this keeps the accessor as simple as possible.

Also, since we're adding a bunch of GC'd things to CompileOptions, we should have an assertSameCompartment in the frontend::functions, to catch wrapping problems as early as possible.
(In reply to Eddy Bruel [:ejpbruel] from comment #116)
> Created attachment 757029 [details] [diff] [review]
> Implement Debugger.prototype.elementProperty (v5)
> 
> Final patch before I leave on PTO.

Have a great vacation!
(In reply to Jim Blandy :jimb from comment #118)
> (In reply to Eddy Bruel [:ejpbruel] from comment #116)
> > Created attachment 757029 [details] [diff] [review]
> > Implement Debugger.prototype.elementProperty (v5)
> > 
> > Final patch before I leave on PTO.
> 
> Have a great vacation!

Do you want to address these final comments today before I leave? How about we meet up on irc?
Depends on: 877444
Does Debugger::isDebugWrapper() need updating to return true for DebuggerSource objects?  Alternatively, it doesn't seem to be called by anything, so maybe it can go.
Flags: needinfo?(ejpbruel)
Filed bug 879817 to remove isDebugWrapper.
Flags: needinfo?(ejpbruel)
Attachment #752945 - Flags: checkin+
Attachment #752947 - Flags: checkin+
Attachment #752993 - Flags: checkin+
Attachment #753039 - Flags: checkin+
Attachment #753388 - Flags: checkin+
Attachment #753470 - Flags: checkin+
Attachment #753471 - Flags: checkin+
Assignee: ejpbruel → jimb
Attachment #757028 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #757028 - Flags: review?(jimb)
Attachment #763904 - Flags: review?(sphink)
Comment on attachment 763904 [details] [diff] [review]
Implement Debugger.Source.prototype.element (v7)

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

I'm not thinking too hard about the deeper GC issues touched on by this patch, since it seems like that's well in hand now so I won't hurt my pretty little head thinking about it. (Is there a block comment somewhere describing that? I *am* interested, if I don't have to exercise too many brain cells.)

::: js/src/jit-test/tests/debug/Source-element-01.js
@@ +2,5 @@
> +
> +var g = newGlobal();
> +var dbg = new Debugger;
> +var gw = dbg.addDebuggee(g);
> +g.evaluate("function f(x) { return 2*x; }", {element: { foo: "bar" }});

Is this new 'element' property in the evaluate options documented in the help(evaluate) output? I don't see it in this patch or the one that introduces 'element', but maybe it's in another? Please add it in if it isn't already there.

::: js/src/jsapi-tests/testOOM.cpp
@@ +17,4 @@
>  virtual JSRuntime * createRuntime()
>  {
>      JSRuntime *rt = JS_NewRuntime(0, JS_USE_HELPER_THREADS);
> +    JS_SetNativeStackQuota(rt, 50000);

How are these stack quota patches related to this bug? Not that I mind leaving them in even if they aren't. Just wondering.

::: js/src/jsscript.cpp
@@ +532,4 @@
>          options.setVersion(version_)
>                 .setNoScriptRval(!!(scriptBits & (1 << NoScriptRval)))
>                 .setSelfHostingMode(!!(scriptBits & (1 << SelfHosted)));
> +        JS::RootedObject sourceObject(cx);

There's not quite enough context to tell if the NULL value is ever going to be tested, but personally I prefer to explicitly initialize with NULL in that case, rather than relying on the default initialization to NULL. It's implicit documentation.

@@ +787,4 @@
>  js::ScriptSourceObject *
>  JSScript::sourceObject() const
>  {
> +    return &UncheckedUnwrap(sourceObject_)->asScriptSource();

Can this be ->as<ScriptSourceObject*>() yet? (njn has landed a bunch of patches changing everything to this style; asScriptSource should be done this way too if it isn't already.)

@@ +951,2 @@
>      sourceObject->setSlot(SOURCE_SLOT, PrivateValue(source));
> +    sourceObject->setSlot(ELEMENT_SLOT, ObjectOrNullValue(options.element.get()));

initSlot instead of setSlot for both of these, to avoid unneeded barriers.

::: js/src/vm/Debugger.cpp
@@ +3681,5 @@
> +    THIS_DEBUGSOURCE_REFERENT(cx, argc, vp, "(get element)", args, obj, sourceObject);
> +
> +    if (sourceObject->element()) {
> +        args.rval().setObjectOrNull(sourceObject->element());
> +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, args.rval())) 

trailing whitespace

::: js/src/vm/GlobalObject.cpp
@@ +234,4 @@
>          CompileOptions options(cx);
>          options.setNoScriptRval(true)
>                 .setVersion(JSVERSION_DEFAULT);
> +        JS::RootedScriptSource sourceObject(cx, ScriptSourceObject::create(cx, ss, options));

This should be JS::RootedObject, no? Does JS::RootedScriptSource still exist? (Wondering how this compiled.)
Attachment #763904 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #124)
> I'm not thinking too hard about the deeper GC issues touched on by this
> patch, since it seems like that's well in hand now so I won't hurt my pretty
> little head thinking about it. (Is there a block comment somewhere
> describing that? I *am* interested, if I don't have to exercise too many
> brain cells.)

Which "deeper GC issues" do you mean?

We decided to wrap, and not clone ScriptSourceObjects, for reasons not evident in this patch:

Leaving ScriptSourceObjects in their original zone will help us implement Debugger.Source.prototype.introductionScript, the script that *introduced* a given piece of source code to the system: "Here is the call to eval / script element creation / assignment to event handler attribute / etc. that caused us to compile this source code." (There is an accompanying 'introductionScriptOffset' property, which gives the bytecode offset of the introduction call within 'introductionScript'.)

We want to store a pointer to the introducing JSScript * as a private value in a reserved slot of the ScriptSourceObject. But since there is nothing analogous to a cross-compartment wrapper for JSScripts, if the ScriptSourceObject and its introducing JSScript are in different zones, there is no way for a single-zone GC of the zone containing the JSScript to find the ScriptSourceObject referring to it. Thus, ScriptSourceObjects need to stay in the compartment in which they were created --- which is the same compartment as the introducing JSScript. (At least in the common case, that's so; if it is ever not true, we will probably just drop the introducing script reference.)

Do you mean why it's necessary to use a WeakMap with a ScriptSourceObject as a key? That's the same justification as all the other Debugger-related WeakMaps: the magic WeakMap GC behavior is the only way to ensure things Debugger has looked at can get cleaned up when they should be.

> Is this new 'element' property in the evaluate options documented in the
> help(evaluate) output? I don't see it in this patch or the one that
> introduces 'element', but maybe it's in another? Please add it in if it
> isn't already there.

Fixed --- thanks!

> ::: js/src/jsapi-tests/testOOM.cpp
> @@ +17,4 @@
> >  virtual JSRuntime * createRuntime()
> >  {
> >      JSRuntime *rt = JS_NewRuntime(0, JS_USE_HELPER_THREADS);
> > +    JS_SetNativeStackQuota(rt, 50000);
> 
> How are these stack quota patches related to this bug? Not that I mind
> leaving them in even if they aren't. Just wondering.

For reasons I'm not clear on, the self-hosting changes cause us to clone scripts into other zones earlier in the initialization process than we ever have before. In particular, some JSAPI tests never used to clone any scripts at all, but now do. This patch introduces a call to 'JSCompartment::wrap' into js::CloneScript; 'wrap' calls JS_CHECK_CHROME_RECURSION; and those tests never set a stack quota, so they suddenly started failing. Bug 877995 and bug 878000 are fixes for similar problems elsewhere in the engine. (I don't know how those other parts of Firefox managed to run JS without setting a stack quota; perhaps there are more initialization ordering issues here than I know. But setting the quota seems benign.)

> ::: js/src/jsscript.cpp
> @@ +532,4 @@
> >          options.setVersion(version_)
> >                 .setNoScriptRval(!!(scriptBits & (1 << NoScriptRval)))
> >                 .setSelfHostingMode(!!(scriptBits & (1 << SelfHosted)));
> > +        JS::RootedObject sourceObject(cx);
> 
> There's not quite enough context to tell if the NULL value is ever going to
> be tested, but personally I prefer to explicitly initialize with NULL in
> that case, rather than relying on the default initialization to NULL. It's
> implicit documentation.

That declaration is immediately followed by an 'if', both of whose branches initialize sourceObject. So I think this is the same as any of the other RootedObject declarations that don't mention a value. Say, iteratorProto in GlobalObject::initIteratorClasses.

> @@ +787,4 @@
> >  js::ScriptSourceObject *
> >  JSScript::sourceObject() const
> >  {
> > +    return &UncheckedUnwrap(sourceObject_)->asScriptSource();
> 
> Can this be ->as<ScriptSourceObject*>() yet? (njn has landed a bunch of
> patches changing everything to this style; asScriptSource should be done
> this way too if it isn't already.)

Fixed; after bug 880041 (part 10), (attachment 762544 [details] [diff] [review]), isScriptSource and asScriptSource don't even exit any more.

> @@ +951,2 @@
> >      sourceObject->setSlot(SOURCE_SLOT, PrivateValue(source));
> > +    sourceObject->setSlot(ELEMENT_SLOT, ObjectOrNullValue(options.element.get()));
> 
> initSlot instead of setSlot for both of these, to avoid unneeded barriers.

Ah, thanks. Fixed.

> ::: js/src/vm/Debugger.cpp
> @@ +3681,5 @@
> > +    THIS_DEBUGSOURCE_REFERENT(cx, argc, vp, "(get element)", args, obj, sourceObject);
> > +
> > +    if (sourceObject->element()) {
> > +        args.rval().setObjectOrNull(sourceObject->element());
> > +        if (!Debugger::fromChildJSObject(obj)->wrapDebuggeeValue(cx, args.rval())) 
> 
> trailing whitespace

Fixed.

> ::: js/src/vm/GlobalObject.cpp
> @@ +234,4 @@
> >          CompileOptions options(cx);
> >          options.setNoScriptRval(true)
> >                 .setVersion(JSVERSION_DEFAULT);
> > +        JS::RootedScriptSource sourceObject(cx, ScriptSourceObject::create(cx, ss, options));
> 
> This should be JS::RootedObject, no? Does JS::RootedScriptSource still
> exist? (Wondering how this compiled.)

We never removed RootedScriptSource; it's there in RootingAPI.h like everything else. I don't remember why I had to change some of the uses; perhaps I was afraid that making JSScript::Create's sourceObject parameter a HandleObject would cause troubles if I passed a RootedScriptSource? In any case, I've tightened the types back up.

I've also added a 'using JS::RootedScriptSource' to jsapi.h, along with its bretheren, so we can use it in the 'js::' namespace directly, and removed a bunch of unnecessary 'JS::' prefixes.
Try push for .element (v7), reviewed above:
https://tbpl.mozilla.org/?tree=Try&rev=e915ffe16a53
(I'm not going to be able to get this landed before Eddy comes back, but at least I can get it un-bitrotted!)
Unbitrotted.
Attachment #757029 - Attachment is obsolete: true
Attachment #757029 - Flags: review?(jimb)
elementProperty Try push:
https://tbpl.mozilla.org/?tree=Try&rev=380ecd0c4441
(In reply to Jim Blandy :jimb from comment #125)
> For reasons I'm not clear on, the self-hosting changes cause us to clone
> scripts into other zones earlier in the initialization process than we ever
> have before. In particular, some JSAPI tests never used to clone any scripts
> at all, but now do.

Drive-by comment: that should only happen if the tests use some self-hosted builtin, such as many of the Array extras. If it happens under other circumstances, please tell me.
I keep seeing little GC-related crashes in my Try pushes. They're not reproducible. I've been pulling my hair out trying to track it down, and I've come to strongly suspect this line:

http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#l3890

Basically, putting a HandleObject in there (r=me...) is probably bogus. Handles should only be used for parameters; they shouldn't be used in places where the handle might outlive the rooted reference it refers to. For example, I'm pretty sure this CompileOptions does not have a stack-like lifetime:

http://dxr.mozilla.org/mozilla-central/source/js/src/ion/AsmJSModule.h#l301
Depends on: 887077
Comment on attachment 763904 [details] [diff] [review]
Implement Debugger.Source.prototype.element (v7)

Terrence, I'm pretty sure this is causing some intermittent oranges like this one:

https://tbpl.mozilla.org/php/getParsedLog.php?id=24538583&tree=Try

I've been going over this all day, and I haven't been able to figure out anything. Could you give the patch a look?

Note that I've filed bug 887077 for the HandleObject in CompileOptions, but since there are no calls to setElement in the browser, it's hard to see how any CompileOptions::element could ever be anything other than JS::NullPtr::constNullValue when those crashes occurred.
Attachment #763904 - Flags: review+ → review?(terrence)
(In reply to Jim Blandy :jimb from comment #132)

So the crash is happening because it's trying to marking the destination of a cross compartment wrapper black and finding it has already been marked gray.  This is an error because gray marking is only meant to affect things that would not have otherwise been marked black.

Is the new elements slot a cross-compartment reference?  If so I think it needs special tracing to call MarkCrossCompartmentSlot() or similar, and also it needs to somehow be represented in the cross compartment map, unless the referent is always in the atoms compartment.  This is because the GC needs to arrange the order it finishes marking zone groups carefully to allow them to be split up.
Comment on attachment 763904 [details] [diff] [review]
Implement Debugger.Source.prototype.element (v7)

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

Jon answered the question better than I could in comment 133.
Attachment #763904 - Flags: review?(terrence)
Comment on attachment 763904 [details] [diff] [review]
Implement Debugger.Source.prototype.element (v7)

Whoops, I guess this was already r+.
Attachment #763904 - Flags: review+
(In reply to Jon Coppeard (:jonco) from comment #133)

The ELEMENT_SLOT of a ScriptSourceObject should never be a direct cross-compartment reference, although it can be a cross-compartment wrapper. If it is a CCW, then that should be in the cross-compartment map by its nature, and calling MarkCrossCompartmentSlot should be unnecessary, right? 

This patch does change the script->sourceObject_ field from being always same-compartment to being possibly a cross-compartment wrapper. Hmm, I wonder if the call to JSScript::setSourceObject from JSFunction::createScriptForLazilyInterpretedFunction is okay; that doesn't create a wrapper...
And whaddyaknow, jit-tests has absolutely no coverage of that call to JSScript::setSourceObject. Adding 'abort' there causes no jit-tests failures.
(In reply to Jon Coppeard (:jonco) from comment #133)
> (In reply to Jim Blandy :jimb from comment #132)
> 
> So the crash is happening because it's trying to marking the destination of
> a cross compartment wrapper black and finding it has already been marked
> gray.  This is an error because gray marking is only meant to affect things
> that would not have otherwise been marked black.

(It looks like my guess that CompileOptions was to blame was definitely wrong.)

It might help to debug this if we could have the assertion print out the classes of the wrapper's referents. Then we could at least know what type of object is at fault.
Assignee: jimb → ejpbruel
Blocks: 898083
Given the failures from before, I want to approach making JSScript use CCWs to refer to their ScriptSourceObjects a bit more carefully. First patch of two.
Previous patch didn't pass check-style in js/src. Re-try:
https://tbpl.mozilla.org/?tree=Try&rev=c4efc2c73431
Excellent, I guess: Here's the same assertion failure as before, but now with a simpler patch. It doesn't seem to reproduce on Linux, so I'll add some logging and re-try.

Assertion failure: !zone->isCollecting(), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/gc/Marking.cpp:681
TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_SpecialPowersExtension.html | application terminated with exit code 2147483651
PROCESS-CRASH | /tests/Harness_sanity/test_SpecialPowersExtension.html | application crashed [@ ShouldMarkCrossCompartment]
Return code: 1
I added some printfs:

02:41:07     INFO -  JIMB: Bad BLACK->GRAY edge; src 101D2100 class is Proxy; dst 04FB8480 class is ScriptSource
Another Try push with more printfs: https://tbpl.mozilla.org/?tree=Try&rev=e425eeb3ccb5

I've tried to reproduce it locally but not succeeded.
That try push added a new assertion that the script source object the patched js::CloneScript passes to JSCompartment::wrap is never gray. It fails horribly. That was based on this IRC conversation:

<billm> jimb: when the wrap() call happens, I think you should print the color
        of the object being wrapped (black/gray/white) as well as the
        collection state of the src and dst zones
<jimb> billm: Okay. And the referent must be marked black?
<billm> jimb: or white. just not gray.

which I may or may not have fully understood. Bill, does that assertion look to you like it should pass?
Flags: needinfo?(wmccloskey)
The assertion looks correct to me. I suspect we're missing a call to xpc_UnmarkGrayScript somewhere in that call chain. smaug or bz might have some ideas about what to do.

We have a fundamental invariant that the JS engine (excluding the GC and some other corner cases) should never see gray objects. We're not very good about actually ensuring that invariant though. Steve Fink did a big pass a while back to improve the situation, but it's probably regressed since then.
Flags: needinfo?(wmccloskey)
Actually, something else is wrong here. I can't explain why jit-tests would be failing. Nothing should ever be marked gray during jit-tests. Can you try to debug that test locally, Jim?
Oh, I just noticed they're timeouts. Never mind. Although I don't know why that would happen either, but at least the assertions aren't failing.
(In reply to Bill McCloskey (:billm) from comment #147)
> We have a fundamental invariant that the JS engine (excluding the GC and
> some other corner cases) should never see gray objects. We're not very good
> about actually ensuring that invariant though. Steve Fink did a big pass a
> while back to improve the situation, but it's probably regressed since then.

I never got it 100% fixed with our full test suite. Some part of the mccr8/bholley organism made the unmarking more principled since then, I think (I haven't really followed the exposeToJS thing or whatever it's called). But I'd be a little surprised if those particular edges are interfering here.

Would it be useful/possible to call DumpHeapComplete if the assertion is going to fail, so you can examine the full heap in the logs? The heap dump contains the color of each node. It might hit the log size limit, though.
jimb: it appears that we *do* now have a facility for uploading arbitrary files from the try server, for a limited set of platforms and jobs. But ahal on #releng says that it should be available in the case that matters here.

You can check for an environment variable MOZ_UPLOAD_DIR. If it is set, then that directory will already exist, and anything you put in it will get uploaded. The url to your uploads will be printed in the log file. I suggest doing a DumpHeapComplete to a file in that dir if your assertion is going to fail, so you can see the full heap graph.

Besides, it would be a great test of the latest cool toy. ;-)
Sadly, it may be busted at the moment. See bug 938339.
I'm not sure what you mean, Steve. We have calls to JS::ExposeObjectToActiveJS scattered all over the browser. It seems quite possible that we're just missing one. JS::ExposeObjectToActiveJS is just a version of xpc_UnmarkGrayObject that's inlined.

I don't know what a heap dump would tell us. We already know that the sourceObject() is gray. I guess we could check if the script being cloned is gray, but that wouldn't require a heap dump. And it seems almost certain that it will be gray since the marking code for scripts marks the sourceObject.
Oh, sorry. I haven't been following along that closely. s/more principled/better named/. And sure, there's likely a missing unmark gray; I'm just saying that the ones I failed to track down were generally obscure, so it's likely to be one added since then. But that's guesswork, obviously.

As for whether the dump is useful, perhaps not, but it seemed like there was some speculation as to the timing of the sourceObject addition. And (handwaving here) I thought it might be useful to see the whole wad of gray connected to the sourceObject. I mean, if it's something in the stack you already have, then that's way better than scrounging through a heap graph. So yes, I'm probably just adding noise here.

Looking at it... how does the jsMethodObject in nsXBLProtoImplMethod::InstallMember get ungrayed? It's pulled out of a field of nsXBLMaybeCompiled, but shouldn't nsXBLMaybeCompiled::GetJSFunction() unmark it? Or is it supposed to get unmarked when it gets put into nsXBLMaybeCompiled? (I don't know if that's expected to live across a GC or not.) It's expected to be used as a Heap<>. Would it make sense to ExposeObjectToActiveJS in GetJSFunction()?
(In reply to Steve Fink [:sfink] from comment #154)
> Looking at it... how does the jsMethodObject in
> nsXBLProtoImplMethod::InstallMember get ungrayed? It's pulled out of a field
> of nsXBLMaybeCompiled, but shouldn't nsXBLMaybeCompiled::GetJSFunction()
> unmark it? Or is it supposed to get unmarked when it gets put into
> nsXBLMaybeCompiled? (I don't know if that's expected to live across a GC or
> not.) It's expected to be used as a Heap<>. Would it make sense to
> ExposeObjectToActiveJS in GetJSFunction()?

Yeah, I think we need something like that.

Another options is to start adding ExposeObjectToActiveJS calls to the JSAPI. We didn't do that before because the code was in XPConnect. Now that it's in the JS engine, it might make sense to do it that way. We could start with CloneScript.
Is the fix something like this?
Attachment #833140 - Flags: feedback?(sphink)
Comment on attachment 833140 [details] [diff] [review]
Make nsXBLProtoImplMethod handle gray objects properly.

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

Yes, though I was tracing it back one further level to nsXBLMaybeCompiled, since it is the one actually holding a JSObject*. nsXBLProtoImplMethod is just passing it through. I think of the unmark gray stuff as a read barrier, and that's the primordial read.
Attachment #833140 - Flags: feedback?(sphink)
(In reply to Steve Fink [:sfink] from comment #157)
> Yes, though I was tracing it back one further level to nsXBLMaybeCompiled,
> since it is the one actually holding a JSObject*. nsXBLProtoImplMethod is
> just passing it through. I think of the unmark gray stuff as a read barrier,
> and that's the primordial read.

Well, the CC tracing is actually handled by nsXBLProtoImplMethod::Trace, so that seems like the right layer to address this. For example, the call to GetCompiledMethod in that Trace member function should *not*, I assume, cause the thing to be marked black, if that's being called for a gray trace.
Depends on: 941122
I hope that the patch I posted in bug 941122 will resolve the intermittent failures first noted in comment 131 (June 25). Try push:

https://tbpl.mozilla.org/?tree=Try&rev=95e29d89a065
(In reply to Jim Blandy :jimb from comment #158)
> (In reply to Steve Fink [:sfink] from comment #157)
> > Yes, though I was tracing it back one further level to nsXBLMaybeCompiled,
> > since it is the one actually holding a JSObject*. nsXBLProtoImplMethod is
> > just passing it through. I think of the unmark gray stuff as a read barrier,
> > and that's the primordial read.
> 
> Well, the CC tracing is actually handled by nsXBLProtoImplMethod::Trace, so
> that seems like the right layer to address this. For example, the call to
> GetCompiledMethod in that Trace member function should *not*, I assume,
> cause the thing to be marked black, if that's being called for a gray trace.

Ah, I see. Yes, that makes sense. (Sorry for the delay in responding.)
Comment on attachment 833140 [details] [diff] [review]
Make nsXBLProtoImplMethod handle gray objects properly.

This bug and patch are now being dealt with in bug 941122.
Attachment #833140 - Attachment is obsolete: true
Attachment #830527 - Flags: review?(wmccloskey)
Attachment #830528 - Flags: review?(sphink)
Attachment #830527 - Flags: review?(wmccloskey) → review+
Attachment #766502 - Attachment is obsolete: true
Attachment #8336307 - Flags: review?(sphink)
Attachment #8336309 - Flags: review?(sphink)
Attachment #830528 - Flags: review?(sphink) → review+
Attachment #8336307 - Flags: review?(sphink) → review+
Attachment #8336309 - Flags: review?(sphink) → review+
Comment on attachment 830528 [details] [diff] [review]
Have cloned JSScripts refer to their ScriptSourceObjects via a CCW, not by copying them

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

I should have looked, but is there a good place to sneak in an assertion that the scriptSource compartment matches the script's compartment?
(In reply to Steve Fink [:sfink] from comment #164)
> Comment on attachment 830528 [details] [diff] [review]
> Have cloned JSScripts refer to their ScriptSourceObjects via a CCW, not by
> copying them
> 
> Review of attachment 830528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I should have looked, but is there a good place to sneak in an assertion
> that the scriptSource compartment matches the script's compartment?

Yes - and that's covered in an earlier patch.
Attachment #763904 - Flags: checkin+
Attachment #830527 - Flags: checkin+
Attachment #830528 - Flags: checkin+
Attachment #8336307 - Flags: checkin+
Attachment #8336309 - Flags: checkin+
That's an awful lot of patches.

Let's convert this bug into a meta-bug, and make future patches block it.
Blocks: 800200
Depends on: 941876
Summary: [jsdbg2] Implement Debugger.Source, providing script source code and detailed origin information → [jsdbg2] [meta] Implement Debugger.Source, providing script source code and detailed origin information
Depends on: 911721
No longer blocks: 332176
Depends on: 969786
Assignee: ejpbruel → nobody
What's left to do here?
See Also: → 865313, 977255
(In reply to Florian Bender from comment #169)
> What's left to do here?

We're leaving this open as a meta bug, as it says in the title.
See Also: → 957798
Depends on: dbg-source
Just found this bug. Can it be closed? All of its blockers are closed.
As nobody responded to jlongster's request, I think it's safe to close this bug.

Sebastian
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.