Jason Orendorff [:jorendorff] Assignee
	Description • 13 years ago

The jsdbg2 branch contains a bunch of work on a Debugger object, currently accessible only in the shell.

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/

https://wiki.mozilla.org/Debugger#Properties_of_the_Debugger.Object_prototype

Plan:
  - get reviews on everything up to jsdbg2 rev f630b2a5add1
  - address urgent review comments
  - file followup bugs to address any non-urgent review comments
  - merge and push to mozilla-central

Development after rev f630b2a5add1 will use bugzilla and the normal review process. The jsdbg2 repo will be retired.

We have been tracking mozilla-central closely, so the merge won't be hard. Reviews are the main thing remaining.

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 13 years ago

Unlike most of what we've done so far, the Debugger object has a ton of accessor properties in it. Add some JSAPI support to define such properties.

I actually had a question for Luke about this one. I think this question only makes sense after you look at the patch.

If I have a template

  template <int X>
  JSBool NativeTemplate(JSContext *cx, uintN argc, Value *vp)
  { ... }

and I try to use NativeTemplate<0> as an argument to JS_PSGS as defined in this patch, I get compile errors in non-debug builds only. This is the opposite of what I intended. I actually had to expand such a template by hand (there were only 4 instances, so working around it with a class template would have been even uglier). Pretty gross. Can it be fixed?

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 13 years ago

You can look at the "everything" patch to see where this gets used. Two places.

I could look up the constructor property, but it seems a waste since js_InitClass has the JSObject in question handy.

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 13 years ago

r?dvander because it seems like there are slight variants of this scattered around in the jits.

Incidentally there's also a BytecodeRangeWithLineNumbers in the "everything" patch.

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 13 years ago

The debugger does cross-compartment Object introspection by entering the compartment, looking around, and then leaving the compartment. But an error can occur while you're in there.

We don't want to throw to the debugger a wrapper of a debuggee-compartment Error object, because just printing the exception would implicitly cause things to happen in the debuggee, and we very much want to avoid that. (Plus, it's not obvious what kind of wrapper the debugger would get.)

I really want to make exceptions impossible in these situations, but until I get around to that, we need the error-copying code.

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 13 years ago

This actually also adds support for a same-compartment weakmap with JSObject * keys and values. But the CrossCompartmentMarkPolicy is what wants scrutiny.

	Jason Orendorff [:jorendorff] Assignee
	Comment 7 • 13 years ago

I guess I can't assign every review to a different JS hacker. Oh well.

  - Expose the JSObject methods needed by Debugger.Object introspection
    methods. Property descriptor stuff, mainly, but also a few odds and ends
    like isSealed().

    As a result of the property descriptor refactoring, a bug in proxies
    (previously reported, actually, bug 636789) was magically fixed. This
    required fixing a silly test that wanted to see the buggy behavior (pfft).

  - infallible JSObject::setReservedSlot (see comment -- use with care,
    it'll assert)

  - JSObject::arrayGetOwnDataElement. This has a rather narrow use; see
    jsdbg.cpp:DebuggerScript_getAllOffsets in the "everything" patch,
    where it is used in building an array of arrays.

	Jason Orendorff [:jorendorff] Assignee
	Comment 8 • 13 years ago

Direct eval scripts store the calling function. Instead of script->getFunction(0), how about a method, getCallerFunction?

Also, initialize savedCallerFun earlier: in NewScriptFromCG rather than its caller, Compiler::compileScript. Rationale: Now that the onNewScript hook is actually functional enough that you can use it and test it, it turns out we were passing it scripts that weren't fully initialized. Debugger.Script objects have a method, Debugger.Script.prototype.getChildScripts, that needs to know to skip function 0 in direct eval scripts. The debugger needs to be able to call this method from the onNewScript hook, so the savedCallerFun bit had better be set.

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 13 years ago

This patch wouldn't compile on its own; I just tried to split the everything patch into reviewable chunks.

For a working js engine, either
  hg clone http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2
or see the "everything" patch.


Main, part 1

Brand-new code in jsdbg.{h,cpp}, and the necessary per-compartment data structures in jscntxt.{h,cpp} and jscompartment.{h,cpp}.

This is complicated by the fact that JSD1 must remain working for now.

This includes a whole new implementation of breakpoints, so the C entry points in jsdbgapi.cpp (JS_SetTrap, JS_ClearTrap, etc.) are reimplemented on top of that. Same goes for JS_SetDebugModeForCompartment.

GC integration code is included in this part too.

JS_HandleTrap and JS_ClearAllTraps are gone (the latter replaced by JS_ClearAllTrapsForCompartment).

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 13 years ago

In case your gut reaction is "debuggers, plural?!?!" see the api spec at https://wiki.mozilla.org/Debugger#Properties_of_the_Debugger.Object_prototype

The idea is that you should be able to run a hypothetical jsdbg2-based Firebug and the Firefox Web Console without the two having to fight over anything. Addons will be able to use the Debugger API too; there are legitimate reasons to use it other than "you're writing a debugger". Careful introspection of content objects, for example. So multiple Debugger objects can coexist peacefully; events will fire to all of them.

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 13 years ago

You might have to look at jsdbg2 tip to make sense of some of this...

Major bits:

 - Most importantly, JS_SetDebugModeForCompartment changed API significantly.
   It can now report an error. This means that the shell debugMode() function
   now throws if it tries to enable debug mode and can't.

 - Bug fix: TrampolineCompiler::generateForceReturn wasn't emitting
   a call to ScriptDebugEpilogue. I added it.

 - Bug fix: js_InternalThrow(VMFrame &f) wasn't handling the throw hook
   the same way as the interpreter. It should be called just before each time
   exception handling has to deal with a frame (whether it's going to jump to
   handler code or pop the frame). js_InternalThrow was only calling it once.

 - The Debugger.onNewScript hook needs the script JSObject to exist earlier, so
   move the creation of that object from jsapi.cpp to JSScript::NewScriptFromCG.
   This makes more sense anyway: the new JSScript is really finished at the end.

Minor stuff:

 - JSCompartment::debugMode became a member function.

 - The shell clone() function can now clone functions across compartments.
   This was used for testing.

 - The shell now calls JS_DefineDebuggerObject. \o/

 - JS_HandleTrap is now called Debugger::onTrap (the new method dispatches
   traps to both JSD1 and JSD2 handlers). Other debug hook dispatch stuff
   was similarly renamed.

 - Because there's now a class js::Debugger, stubs::Debugger was renamed to
   stubs::DebuggerStatement.

	Jason Orendorff [:jorendorff] Assignee
	Comment 13 • 13 years ago

I changed JS_SetTrap, and thus the trap() shell builtin, to assert that the
offset is valid, and that broke a bunch of tests.

This fixes them, for now.

(They could parse the output of disassemble() instead, which might be more robust—but I'm hoping we delete JSD1 before that would pay off.)

	Jason Orendorff [:jorendorff] Assignee
	Comment 14 • 13 years ago

The biggest patch of the bunch: tests for the new code.

I don't expect anyone to review this. It's pretty awesome though.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 15 • 13 years ago

Comment on attachment 547153 [details] [diff] [review]
Support, part 6 - Cross-compartment weak map support.

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

The same-compartment stuff looks fine. However, I think it will probably work across compartments as well, so that we don't need CrossCompartmentMarkPolicy. Namely: if you call IsAboutToBeFinalized on an object that's in a different compartment than the one being GCed, then it returns false. So it seems like the extra isMarked check is unnecessary. And MarkObject will ignore objects that are not in a compartment being GCed, so I think those checks are also not needed.

Perhaps I'm missing the point, though.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 16 • 13 years ago

Comment on attachment 547146 [details] [diff] [review]
Support, part 3 - Add js::BytecodeRange! \o/

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

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 17 • 13 years ago

Comment on attachment 547167 [details] [diff] [review]
Main, part 3 - Changes to how the rest of the engine interacts with debug mode.

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

::: js/src/jsscript.cpp
@@ +1232,5 @@
>      /* Tell the debugger about this compiled script. */
>      js_CallNewScriptHook(cx, script, fun);
> +    if (!cg->parent) {
> +        Debugger::onNewScript(cx, script,
> +                              fun ? fun : script->u.object ? script->u.object : cg->scopeChain(),

Could this be parenthesized so the association is a little clearer?

	Andrew Drake [:adrake]
	Comment 18 • 13 years ago

Comment on attachment 547172 [details] [diff] [review]
Tests - part 1, Fixes for trap tests that have probably been busted for some time.

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

Looks good; I'm amazed the insertion of the trap somewhere invalid wasn't breaking something else long before now.

	Luke Wagner [:luke]
	Comment 19 • 13 years ago

(In reply to comment #1)
> Pretty gross. Can it be fixed?

I wasn't able to reproduce the compile error locally (using cset 695f93acc095), but I did notice that sometimes errors with instantiation get incorrectly reported as "insufficient context".  I think I even see one such error (the enumerator I believe should be prefixed by Debugger::).  In particular, do you still see the error with this change (x3):

-    JS_PSGS("onDebuggerStatement", Debugger::getHook<OnDebuggerStatement>,
-            Debugger::setHook<OnDebuggerStatement>, 0),
+    JS_PSGS("onDebuggerStatement", Debugger::getHook<Debugger::OnDebuggerStatement>,
+            Debugger::setHook<Debugger::OnDebuggerStatement>, 0),

	Jason Orendorff [:jorendorff] Assignee
	Comment 20 • 13 years ago

I get the same errors with or without the change suggested in comment 19. Maybe it's a bug in the Mac version of GCC that has since been fixed.

$ g++-4.2 -v
Using built-in specs.
Target: i686-apple-darwin10
Configured with: /var/tmp/gcc/gcc-5666.3~123/src/configure --disable-checking --enable-werror --prefix=/usr --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.2/ --with-slibdir=/usr/lib --build=i686-apple-darwin10 --program-prefix=i686-apple-darwin10- --host=x86_64-apple-darwin10 --target=i686-apple-darwin10 --with-gxx-include-dir=/include/c++/4.2.1
Thread model: posix
gcc version 4.2.1 (Apple Inc. build 5666) (dot 3)

$ make jsdbg.o
jsdbg.cpp
g++-4.2 -o jsdbg.o -c  -fvisibility=hidden -DOSTYPE=\"Darwin10.8.0\" -DOSARCH=Darwin -DEXPORT_JS_API -DIMPL_MFBT -D__STDC_LIMIT_MACROS  -I.. -I. -I./dist/include -I./dist/include/nsprpub     -I.. -I../assembler -I../yarr  -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -ggdb3 -fno-strict-aliasing -fpascal-strings -fno-common -pthread -pipe  -DNDEBUG -DTRIMMED -g -O3 -fstrict-aliasing -fno-stack-protector -fno-omit-frame-pointer -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1   -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MF .deps/jsdbg.pp /Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1500: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1500: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1502: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1502: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1504: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1504: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1506: error: insufficient contextual information to determine type
/Users/jorendorff/dev/jsdbg2/js/src/jsdbg.cpp:1506: error: insufficient contextual information to determine type
make: *** [jsdbg.o] Error 1

	Luke Wagner [:luke]
	Comment 21 • 13 years ago

(In reply to comment #20)
Bummer.  Incidentally I get a different (linker) error on Linux gcc 4.5 that requires explicitly instantiating the set/get templates, so clearly this is twitchy logic in gcc.  I'm happy to leave it as is if you are :)

	Jason Orendorff [:jorendorff] Assignee
	Comment 22 • 13 years ago

(In reply to comment #21)
> I'm happy to leave it as is if you are :)

Sold.

	Jeff Walden [:Waldo]
	Comment 23 • 13 years ago

Comment on attachment 547166 [details] [diff] [review]
Main, part 2 - Each global object gains a slot for a list of debuggers.

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

::: js/src/vm/GlobalObject.h
@@ +41,5 @@
>  #ifndef GlobalObject_h___
>  #define GlobalObject_h___
>  
>  #include "jsfun.h"
> +#include "jsprvtd.h"

If jsprvtd.h is only needed here for the js::Debugger class-or-struct, just forward-declare it and avoid the not-really-necessary #include.  The jsprvtd.h add-everything-special model appears to me to often create longer compile times than necessary when something nigglingly internal is changed, and in an ideal world I'd eventually like to move away from it.  When I removed the dozen-odd exposed headers a couple few weeks ago, I'd have had to make a bit fewer changes if people had more often forward-declared structs used only by pointer than #included the headers that fully elaborated them.

@@ +174,5 @@
> +    typedef js::Vector<js::Debugger *, 0, js::SystemAllocPolicy> DebuggerVector;
> +
> +    // The collection of Debug objects debugging this global. If this global is
> +    // not a debuggee, this returns either NULL or an empty vector.
> +    DebuggerVector *getDebuggers();

No horse in this race, but this file so far has used /*\n*\n*/ for method comments, seems like this comment and the one below it should do that, for consistency with this file and with most others.

::: js/src/vm/GlobalObject.cpp
@@ +287,5 @@
> +GlobalObject::DebuggerVector *
> +GlobalObject::getOrCreateDebuggers(JSContext *cx)
> +{
> +    assertSameCompartment(cx, this);
> +    DebuggerVector *vec = getDebuggers();

|debuggers| seems a better name (here and elsewhere) than the nondescript |vec|.

@@ +291,5 @@
> +    DebuggerVector *vec = getDebuggers();
> +    if (vec)
> +        return vec;
> +
> +    JSObject *obj = NewNonFunction<WithProto::Given>(cx, &GlobalDebuggees_class, NULL, NULL);

Vague paranoia, but passing this as the parent might be better-slash-less prone to potential error.

@@ +299,5 @@
> +    if (!vec)
> +        return NULL;
> +    obj->setPrivate(vec);
> +    if (!js_SetReservedSlot(cx, this, DEBUGGERS, ObjectValue(*obj)))
> +        return NULL;

setSlot(DEBUGGERS, ObjectValue(*obj));

@@ +314,5 @@
> +    for (Debugger **p = vec->begin(); p != vec->end(); p++)
> +        JS_ASSERT(*p != dbg);
> +#endif
> +    if (vec->empty() && !compartment()->addDebuggee(cx, this))
> +        return false;

This bit with multiple globals being debugged per compartment will be nicer with compartment-per-global, because you only need to keep a single vector around, right?

	Jeff Walden [:Waldo]
	Comment 24 • 13 years ago

Comment on attachment 547154 [details] [diff] [review]
Support, part 7 - JSObject changes.

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

::: js/src/jsobj.h
@@ +146,2 @@
>      /* 8.10.5 ToPropertyDescriptor(Obj) */
> +    bool initialize(JSContext* cx, const js::Value &v, bool checkAccessors=true);

checkAccessors = true

You should describe what |checkAccessors| means.  Reading the patch from top to bottom, I have no idea.  :-)

@@ +147,5 @@
> +    bool initialize(JSContext* cx, const js::Value &v, bool checkAccessors=true);
> +
> +    /* 8.10.4 FromPropertyDescriptor(Desc) */
> +    void initFromPropertyDescriptor(const PropertyDescriptor &desc);
> +    bool makeObject(JSContext *cx);

makeObject could use a description.

@@ +198,5 @@
>          return js::CastAsStrictPropertyOp(setterObject());
>      }
> +
> +    inline bool checkGetter(JSContext *cx);
> +    inline bool checkSetter(JSContext *cx);

These also need descriptions.  Although, since they're only used one place each, I'd just inline them and not have separate methods for them, personally.  Or are they used elsewhere in subsequent patches?

@@ +1651,5 @@
>  
> +
> +/*
> + * Define a property as specified by the ES5 [[DefineOwnProperty]] algorithms,
> + * sections 8.12.9 and (for arrays) 15.4.5.1.

Well, really this calls the [[DefineProperty]] implementation for the provided object.  It does whatever should happen for the object, which might well be proxy behavior, say.

@@ +1763,5 @@
> +
> +bool
> +NewPropertyDescriptorObject(JSContext *cx, const PropertyDescriptor *desc, Value *vp);
> +
> +}

/* namespace js */ for slightly more readability, particularly if this section grows more stuff.

::: js/src/jsobjinlines.h
@@ +287,5 @@
>  
> +inline void
> +JSObject::setReservedSlot(uintN index, const js::Value &v)
> +{
> +    setSlot(index, v);

This seems pretty nice, could be used a bit in the GlobalObject implementation and elsewhere (and in the DebuggerVector patch I just reviewed here, too).  But presumably it should start with |JS_ASSERT(index < JSSLOT_FREE(getClass()));|?

@@ +1381,5 @@
>      global->setSlot(key + JSProto_LIMIT * 2, ObjectValue(*ctor));
>      return true;
>  }
>  
> +bool PropDesc::checkGetter(JSContext *cx) {

Style nit:

..)
{

@@ +1390,5 @@
> +    }
> +    return true;
> +}
> +
> +bool PropDesc::checkSetter(JSContext *cx) {

Here too.

::: js/src/jsobj.cpp
@@ +2478,5 @@
>  
>      return DefinePropertyOnObject(cx, obj, id, desc, throwError, rval);
>  }
>  
> +}

/* namespace js */

	Jason Orendorff [:jorendorff] Assignee
	Comment 25 • 13 years ago

(In reply to comment #23)
> > +#include "jsprvtd.h"
> 
> If jsprvtd.h is only needed here for the js::Debugger class-or-struct, just
> forward-declare it and avoid the not-really-necessary #include.

Done.

> No horse in this race, but this file so far has used /*\n*\n*/ for method
> comments, seems like this comment and the one below it should do that, for
> consistency with this file and with most others.

Just a mistake on my part. Too much late-night hacking. Fixed.

> |debuggers| seems a better name (here and elsewhere) than the nondescript
> |vec|.

Changed.

> > +    JSObject *obj = NewNonFunction<WithProto::Given>(cx, &GlobalDebuggees_class, NULL, NULL);
> 
> Vague paranoia, but passing this as the parent might be better-slash-less
> prone to potential error.

Oh, of course that's better. Good catch.

> > +    if (!js_SetReservedSlot(cx, this, DEBUGGERS, ObjectValue(*obj)))
> > +        return NULL;
> 
> setSlot(DEBUGGERS, ObjectValue(*obj));

This makes me uneasy, but ok. I'll followup to the js-internals mailing list about what the actual contract is here.

> @@ +314,5 @@
> > +    for (Debugger **p = vec->begin(); p != vec->end(); p++)
> > +        JS_ASSERT(*p != dbg);
> > +#endif
> > +    if (vec->empty() && !compartment()->addDebuggee(cx, this))
> > +        return false;
> 
> This bit with multiple globals being debugged per compartment will be nicer
> with compartment-per-global, because you only need to keep a single vector
> around, right?

Yes, *any* collapsing of the sprawling E-R diagram that is JSRuntime / JSContext / thread / JSCompartment / js::GlobalObject will help.

Of course, we're adding to that diagram by connecting Debuggers to GlobalObjects in an N-M relationship, but I think that's OK. First of all, the notion of debuggers isn't nearly as pervasive as all these other things. The extra complexity is well isolated (a few methods here, a few more in JSCompartment, and all the rest in jsdbg.cpp). And second, this is definitely the right thing to do for consumers of the Debugger API.

	Jeff Walden [:Waldo]
	Comment 26 • 13 years ago

Comment on attachment 547157 [details] [diff] [review]
Tidying, part 2 - Miscellaneous cleanups.

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

::: js/src/jit-test/README
@@ +66,5 @@
>      slow         Test runs slowly. Do not run if the --no-slow option is given.
>      allow-oom    If the test runs out of memory, it counts as passing.
>      valgrind     Run test under valgrind.
> +    mjitalways   Run js with -a, whether --jitflags says to or not
> +    debug        Run js with -d, whether --jitflags says to or not

I'd prefer if the changes to this file went in whichever patch actually made them, but if that's not easy to do, never mind.

::: js/src/jsobj.cpp
@@ +878,5 @@
> +           v.isNumber() ? "number" :
> +           v.isBoolean() ? "boolean" :
> +           v.isNull() ? "null" :
> +           v.isUndefined() ? "undefined" :
> +           "value";

Um.  This is insane.  :-)  How about a sequence of if-returns here, please?

@@ +885,1 @@
>  }

I'm guessing without looking at context that this should have a /* namespace js */ by it.

	Jeff Walden [:Waldo]
	Comment 27 • 13 years ago

Comment on attachment 547144 [details] [diff] [review]
Support, part 2 - Make js_InitClass optionally return the new constructor object.

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

If I weren't cleaning this up elsewhere; and if I thought it would be much pain beyond what I've already gone through to rebase through this; or if it weren't part of, and thus holding up, a mega-branch landing; I'd probably complain about more barnacles here.   But none of those (apologies for some double-negatives) are the case, so I'm okay letting you run with this.

	Jason Orendorff [:jorendorff] Assignee
	Comment 28 • 13 years ago

(In reply to comment #24)
> checkAccessors = true
> 
> You should describe what |checkAccessors| means.  Reading the patch from top
> to bottom, I have no idea.  :-)

Done:

    /*
     * 8.10.5 ToPropertyDescriptor(Obj)
     *
     * If checkAccessors is false, skip steps 7.b and 8.b, which throw a
     * TypeError if .get or .set is neither a callable object nor undefined.
     *
     * (DebuggerObject_defineProperty uses this: the .get and .set properties
     * are expected to be Debugger.Object wrappers of functions, which are not
     * themselves callable.)
     */
    bool initialize(JSContext* cx, const js::Value &v, bool checkAccessors=true);

> makeObject could use a description.

    /*
     * 8.10.4 FromPropertyDescriptor(Desc)
     *
     * initFromPropertyDescriptor sets pd to undefined and populates all the
     * other fields of this PropDesc from desc.
     *
     * makeObject populates pd based on the other fields of *this, creating a
     * new property descriptor JSObject and defining properties on it.
     */
    void initFromPropertyDescriptor(const PropertyDescriptor &desc);
    bool makeObject(JSContext *cx);

> > +    inline bool checkGetter(JSContext *cx);
> > +    inline bool checkSetter(JSContext *cx);
> 
> These also need descriptions.  Although, since they're only used one place
> each, I'd just inline them and not have separate methods for them,
> personally.  Or are they used elsewhere in subsequent patches?

Yes, DebuggerObject_defineProperty uses them, via UnwrapPropDesc.

    /*
     * Throw a TypeError if a getter/setter is present and is neither callable
     * nor undefined. These methods do exactly the type checks that are skipped
     * by passing false as the checkAccessors parameter of initialize.
     */
    inline bool checkGetter(JSContext *cx);
    inline bool checkSetter(JSContext *cx);

> @@ +1651,5 @@
> > +
> > +/*
> > + * Define a property as specified by the ES5 [[DefineOwnProperty]] algorithms,
> > + * sections 8.12.9 and (for arrays) 15.4.5.1.
> 
> Well, really [...]

I definitely wanted to keep the relevant ES5 section numbers in the comment, so I rewrote it like this. Which is maybe too thorough, but we'll live.

/*
 * Call the [[DefineOwnProperty]] internal method of obj.
 *
 * If obj is an array, this follows ES5 15.4.5.1.
 * If obj is any other native object, this follows ES5 8.12.9.
 * If obj is a proxy, this calls the proxy handler's defineProperty method.
 * Otherwise, this reports an error and returns false.
 */
extern bool
DefineProperty(JSContext *cx, JSObject *obj, const jsid &id, const PropDesc &desc, bool throwError,
               bool *rval);

> > +JSObject::setReservedSlot(uintN index, const js::Value &v)
> 
> This seems pretty nice, could be used a bit in the GlobalObject
> implementation and elsewhere (and in the DebuggerVector patch I just
> reviewed here, too).  But presumably it should start with |JS_ASSERT(index <
> JSSLOT_FREE(getClass()));|?

Added; and I changed the DebuggerVector code to use this.

> > +bool PropDesc::checkGetter(JSContext *cx) {
> 
> Style nit:

Yep. Fixed both places.

> /* namespace js */

Added both places.

	Jeff Walden [:Waldo]
	Comment 29 • 13 years ago

jorendorff rocks my world.  That is all.

	Jason Orendorff [:jorendorff] Assignee
	Comment 30 • 13 years ago

(In reply to comment #26)
> ::: js/src/jit-test/README
> @@ +66,5 @@
> >      slow         Test runs slowly. Do not run if the --no-slow option is given.
> >      allow-oom    If the test runs out of memory, it counts as passing.
> >      valgrind     Run test under valgrind.
> > +    mjitalways   Run js with -a, whether --jitflags says to or not
> > +    debug        Run js with -d, whether --jitflags says to or not
> 
> I'd prefer if the changes to this file went in whichever patch actually made
> them, but if that's not easy to do, never mind.

Heh! So would I, but that stuff was added back in February. I just updated the README.

FWIW, jsdbg2 contains hundreds of changesets; the history doesn't look much like the reviews in this patch, though they add up to the same total. I just took one massive diff of "what's gonna land" and carved it up into hunks for reviews.

> ::: js/src/jsobj.cpp
> @@ +878,5 @@
> > +           v.isNumber() ? "number" :
> > +           v.isBoolean() ? "boolean" :
> > +           v.isNull() ? "null" :
> > +           v.isUndefined() ? "undefined" :
> > +           "value";
> 
> Um.  This is insane.  :-)  How about a sequence of if-returns here, please?

Yeah, ok.

> @@ +885,1 @@
> >  }
> 
> I'm guessing without looking at context that this should have a /* namespace
> js */ by it.

Added.

	Jason Orendorff [:jorendorff] Assignee
	Comment 31 • 13 years ago

(In reply to comment #15)
> The same-compartment stuff looks fine. However, I think it will probably
> work across compartments as well, so that we don't need
> CrossCompartmentMarkPolicy. [...]

Yes, you're completely right. New patch coming.

	Jason Orendorff [:jorendorff] Assignee
	Comment 32 • 13 years ago

How about this?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 33 • 13 years ago

Comment on attachment 547551 [details] [diff] [review]
Support, part 6 - Cross-compartment weak map support, v2

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 34 • 13 years ago

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/6ff6297bebb6
Address review comments from dvander (bug 672829 comment 17).

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/1af365c2b3fc
Address review comments from Waldo (bug 672829 comment 23).

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/501052c6263b
Address review comments from Waldo (bug 672829 comment 24).

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/46a7638aeb4d
Address review comments from Waldo (bug 672829 comment 26).

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/8998a25f3887
Address review comments from billm (bug 672829 comment 15).

	Brendan Eich [:brendan]
	Comment 35 • 13 years ago

Comment on attachment 547161 [details] [diff] [review]
Main, part 1 - jsdbg.cpp and data structures

     * Right now, we only support runtime-wide debugging.

s/only support/support only/

Nit: instead of:

    /*
     * Lock serializing watchPointList accesses, and count of all
     * mutations to watchPointList made by debugger threads.  To
     * keep the code simple, we define debuggerMutations for the thread-unsafe
     * case too.
     */

wrap to:

    /*
     * Lock serializing watchPointList accesses, and count of all mutations to
     * watchPointList made by debugger threads.  To keep the code simple, we
     * define debuggerMutations for the thread-unsafe case too.
     */

A couple of blank 'tween lines would help here:

    enum { DebugFromC = 1, DebugFromJS = 2 };
    uintN                        debugModeBits;  // see debugMode() below
  public:

Nit: non-empty here:

     * is nonempty.

This is too clever:

    (b * DebugFromC)

Why not (b & DebugFromC)?

Can't the assertions starting with JS_ASSERT_IF(scriptObject, ...) in JSCompartment::getOrCreateBreakpointSite come first and cover the early return path too?

nonnull => non-null

Put \ in column 79 per house style:

#define REQUIRE_ARGC(name, n) \

Half-indent overflow member/super init clause:

    : script(script), pc(pc), realOpcode(JSOp(*pc)), scriptObject(NULL), enabledCount(0),
      trapHandler(NULL), trapClosure(UndefinedValue())

Isn't recompile going to report OOM?

        recompile(cx, false);  // errors ignored

Breakpoint::destroy calls BreakpointSite::destroyIfEmpty which destroys this iff !trapHandler, but only BreakpointSite::clearTrap nulls trapHandler, AFAICS. What if you don't clear before you try to destroy a bp?

Stopping at Debugger::getScriptFrame(JSContext *cx, StackFrame *fp, Value *vp)

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 36 • 13 years ago

(In reply to comment #35)
> Comment on attachment 547161 [details] [diff] [review] [review]
> Main, part 1 - jsdbg.cpp and data structures
> 
>      * Right now, we only support runtime-wide debugging.
> 
> s/only support/support only/

Yep. It's surprising to me that I still miss these, even knowing what the rule is.

For everyone following along at home: the comment as written might mean

 we only *support* runtime-wide debugging, we aren't actively developing it
 we only support *runtime-wide* debugging, not per-compartment
 we only support runtime-wide *debugging*, not runtime-wide profiling

As it happens we mean the second thing. This kind of ambiguity is common with the word "only". In cases where it's grammatical to put "only" just before the word it modifies, that's more precise.

However this particular comment is a complete lie, so I'll replace it with:

    /* If true, new compartments are initially in debug mode. */
    bool                debugMode;

Not very useful! When JSD1 is removed, we'll kill this runtime-wide bit too.


> This is too clever:
> 
>     (b * DebugFromC)
> 
> Why not (b & DebugFromC)?

That would rely on the int value of DebugFromC being 1, which seems precarious (worse than clever?). I'll change it to (b ? DebugFromC : 0).

(I was curious, so I checked, and GCC doesn't emit an extra branch or cmove for that, at least on x86-64. Anyway, it's the JSCompartment constructor—if that's hot code, something's wrong!)

> Isn't recompile going to report OOM?
> 
>         recompile(cx, false);  // errors ignored


> Breakpoint::destroy calls BreakpointSite::destroyIfEmpty which destroys this
> iff !trapHandler, but only BreakpointSite::clearTrap nulls trapHandler,
> AFAICS. What if you don't clear before you try to destroy a bp?

In the case of JSCompartment::sweepBreakpoints, we would retain a BreakpointSite pointing to a dead script. Good catch. I changed it like this:

> void
> JSCompartment::sweepBreakpoints(JSContext *cx)
> {
>     for (BreakpointSiteMap::Enum e(breakpointSites); !e.empty(); e.popFront()) {
>         BreakpointSite *site = e.front().value;
>         if (site->scriptObject) {
>+            // clearTrap and nextbp are necessary here to avoid possibly
>+            // reading *site or *bp after destroying it.
>             bool scriptGone = IsAboutToBeFinalized(cx, site->scriptObject);
>+            bool clearTrap = scriptGone && site->hasTrap();
>+
>             Breakpoint *nextbp;
>             for (Breakpoint *bp = site->firstBreakpoint(); bp; bp = nextbp) {
>                 nextbp = bp->nextInSite();
>                 if (scriptGone || IsAboutToBeFinalized(cx, bp->debugger->toJSObject()))
>                     bp->destroy(cx, &e);
>             }
>+
>+            if (clearTrap)
>+                site->clearTrap(cx, &e);
>         }
>     }
> }

In all the other call sites, we really just want to clear a particular jsdbg2 breakpoint and leave JSD1 alone.

	Brendan Eich [:brendan]
	Comment 37 • 13 years ago

(In reply to comment #36)
> I'll change it to (b ? DebugFromC : 0).

Of course, thanks.

> (I was curious, so I checked, and GCC doesn't emit an extra branch or cmove
> for that, at least on x86-64. Anyway, it's the JSCompartment constructor—if
> that's hot code, something's wrong!)

Right, this was confirmed by someone else (Waldo?) the other year. Good to know.

> > Isn't recompile going to report OOM?
> > 
> >         recompile(cx, false);  // errors ignored

Did your reply get cut here?

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 38 • 13 years ago

Sorry about that. Yes, recompilation can fail with OOM. It would be reported, then ignored.

But we really do want to attempt recompilation, and if it fails we really do not care. So I think the best thing is to remove the error reporter from cx temporarily. How does that sound?

	Brendan Eich [:brendan]
	Comment 39 • 13 years ago

(In reply to comment #38)
> Sorry about that. Yes, recompilation can fail with OOM. It would be
> reported, then ignored.
> 
> But we really do want to attempt recompilation, and if it fails we really do
> not care. So I think the best thing is to remove the error reporter from cx
> temporarily. How does that sound?

Alternative is to make recompile use a non-reporting alloc policy and push the reporting up to its callers.

Refreshed patch for me to restart on? No worries if it's too much trouble.

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 41 • 13 years ago

Like the previous patch, this started out as
  hg diff -r 819a2ffc4f0e -r tip
and then I manually cut out everything else. Since that was a manual step, and also because I changed all the comments from // to /**/, interdiffing the two will be pretty much impossible. Instead consult the repo:
  http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2

	Brendan Eich [:brendan]
	Comment 42 • 13 years ago

Comment on attachment 548278 [details] [diff] [review]
Main, part 1 - jsdbg.cpp and data structures - v2

Isn't vm/Debug.cpp or even vm/Debugger.cpp the right name, not jsdbg.cpp? At some point, if not now (but why not now?).

    JSThread::Map       threads;

is uncommented amid the other commented members and an #endif.

Nit: public: before getMathCache inline definition wants a newline before it, to match surrounding. Same nit for private: before sweepBreakpoints at end.

    debugModeBits(rt->debugMode * DebugFromC),

is still too cleverly dependent on value of DebugFromC, right? Same for

    debugModeBits = (debugModeBits & ~uintN(DebugFromC)) | (b * DebugFromC);

NewNonHeldScript -> NewUnheldScript?

or does the Non usefully connote "never"?

Is there a deeper issue here where an API client could JS_NewScriptObject on a JSScript * it discovers that was recorded as NonHeld by the Debugger implementation?

    /* An ephemeral map from script-holding objects to Debugger.Script */

has a funny

    /*
     * instances.
     */

major comment right after it.

     * additionally vp is non-null, then parse the value returned by

s/additionally/in addition,/

    JSTrapStatus handleUncaughtException(AutoCompartment &ac, Value *vp, bool callHook);

Mutable refs, have you changed your thinking, or is this going with a non-null type convention?

     * The last case is handled by the mark() method. If it finds any Debugger

Grammar nit: "it" refers to "The last case", not "the mark() method" -- best to repeat minimally, e.g. "If mark finds ...."

     * NOT the inverse of wrapDebuggeeValue.

So rename from unwrap to something more precise and not the inverse of wrap? Is this really getDebuggee (get- because fallible) or getDebuggeeIfDebugger or some such?

     * Postcondition: we are in the debugger compartment (ac is not entered)

s/not entered/exited/ or "left"?

Debugger::fromLinks would not need ugly reinterpret_casts and offsetof if you derived Debugger from JSCList, just a static cast.

Double newline at 417 in jsdbg.h -- ditto for 57 in jsdbg.cpp.

DebuggerScript_setBreakpoint could be goto free with some if nesting.

BreakpointSite::inc recompiles after planting JSOP_TRAP on first inc if !trapHandler -- worth a comment on why this is necessary (setEnabled toggling after you first get a JSOP_TRAP recompile via JS_SetTrap).

Nit: in Debugger::addDebuggeeGlobal(JSContext *cx, GlobalObject *obj) perhaps s/obj/global/g -- would help readers see what's going on, I bet.

Nit there about goto fail[123] being easily avoided at the price of some indentation. I'm easy.

Can't you use js_GetScriptLineExtent instead of rewriting it in the form of DebuggerScript_getLineCount?

At FlowGraphSummary, out of time till later today, and I don't want to hoard these comments.

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 43 • 13 years ago

(In reply to comment #42)
> NewNonHeldScript -> NewUnheldScript?
> 
> or does the Non usefully connote "never"?

Well, neither one is an established word. I slightly prefer "non" here, for the reason you suggest -- we mean "this script is not held" (logical inverse) rather than "this script was held but we unheld it" (procedural inverse). "Non" can't mean the latter, but "un" could theoretically mean either one (cf. "undepend", "unlock", "uneval", "unwatch"). Admittedly there is little chance of actual confusion.

In any case, I want to make all scripts "held" (that is, make the GC manage all script lifetimes, with no exception for eval/JS_Evaluate* scripts) and remove this distinction. That would make all this moot. See bug 676281.

> Is there a deeper issue here where an API client could JS_NewScriptObject on
> a JSScript * it discovers that was recorded as NonHeld by the Debugger
> implementation?

Heh. A comment on JSScript::u.script documents that as an API no-no, but actually JS_NewScriptObject is gone! And good riddance. :)
  hg.mozilla.org/mozilla-central/rev/c919a7271ac1

I'll delete the obsolete comment.

>     JSTrapStatus handleUncaughtException(AutoCompartment &ac, Value *vp,
> bool callHook);
> 
> Mutable refs, have you changed your thinking, or is this going with a
> non-null type convention?

OK, this is going to get long, please skip it if you don't have time.

I'm
  * strongly opposed to using references for plain old out parameters;
  * somewhat opposed to using them for JSObject parameters and the like
    (because it's very common for me to pass a pointer and thus get a
    compiler error, which seems to me like pointless friction we never had
    before); and
  * somewhat in favor of references for stack-only RAII-ish types like
    AutoCompartment, since that means we always use ".", never "->", with that
    type. We don't have a lot of precedent either way, looks like.

So that's why I used references. I'll switch to pointers if you like.

It's not about nullability. Unfortunately references are a very poor (worse than useless, IMO) substitute for a real non-nullable pointer type. The compiler doesn't even try to enforce the non-nullability of references. The behavior of chasing a null reference is exactly the same as chasing a null pointer, only the compiler is allowed to do insane things (undefined behavior) in more situations if you use a reference. The other difference is that dereferencing a reference happens without any * or -> or [] in the source code to indicate what's happening. EIBTI, for my money.


Btw, I also had written this:

>+    static void markKeysInCompartment(JSTracer *tracer, ObjectWeakMap &map);

Oops! I changed it to a const reference and it worked fine.


>      * The last case is handled by the mark() method. If it finds any
> Debugger
> 
> Grammar nit: "it" refers to "The last case", not "the mark() method" -- best
> to repeat minimally, e.g. "If mark finds ...."

Instead I reversed the sentence: "Debugger::mark handles the last case. If it finds..."

>      * NOT the inverse of wrapDebuggeeValue.
> 
> So rename from unwrap to something more precise and not the inverse of wrap?
> Is this really getDebuggee (get- because fallible) or getDebuggeeIfDebugger
> or some such?

What it really means to say is "almost but not quite the inverse of wrapDebuggeeValue; read carefully!"

I rewrote both comments in the new patch:

    /*
     * Like cx->compartment->wrap(cx, vp), but for the debugger compartment.
     *
     * Preconditions: *vp is a value from a debuggee compartment; cx is in the
     * debugger's compartment.
     *
     * If *vp is an object, this produces a (new or existing) Debugger.Object
     * wrapper for it. Otherwise this is the same as JSCompartment::wrap.
     */
    bool wrapDebuggeeValue(JSContext *cx, Value *vp);

    /*
     * Unwrap a Debug.Object, without rewrapping it for any particular debuggee
     * compartment.
     *
     * Preconditions: cx is in the debugger compartment. *vp is a value in that
     * compartment. (*vp should be a "debuggee value", meaning it is the
     * debugger's reflection of a value in the debuggee.)
     *
     * If *vp is a Debugger.Object, store the referent in *vp. Otherwise, if *vp
     * is an object, throw a TypeError, because it is not a debuggee
     * value. Otherwise *vp is a primitive, so leave it alone.
     *
     * When passing values from the debuggee to the debugger:
     *     enter debugger compartment;
     *     call wrapDebuggeeValue;  // compartment- and debugger-wrapping
     *
     * When passing values from the debugger to the debuggee:
     *     call unwrapDebuggeeValue;  // debugger-unwrapping
     *     enter debuggee compartment;
     *     call cx->compartment->wrap;  // compartment-rewrapping
     *
     * (Extreme nerd sidebar: Unwrapping happens in two steps because there are
     * two different kinds of symmetry at work: regardless of which direction
     * we're going, we want any exceptions to be created and thrown in the
     * debugger compartment--mirror symmetry. But compartment wrapping always
     * happens in the target compartment--rotational symmetry.)
     */
    bool unwrapDebuggeeValue(JSContext *cx, Value *vp);


>      * Postcondition: we are in the debugger compartment (ac is not entered)
> 
> s/not entered/exited/ or "left"?

Fixed:

     * Postcondition: we are in the debugger compartment, having called
     * ac.leave() even if an error occurred.

> Debugger::fromLinks would not need ugly reinterpret_casts and offsetof if
> you derived Debugger from JSCList, just a static cast.

True, but that adds an implicit dependency on Debugger not having virtual methods and JSCList being the first base class. I'd rather keep offsetof. It's uglier, but more robust against change.

> BreakpointSite::inc recompiles after planting JSOP_TRAP on first inc if
> !trapHandler -- worth a comment on why this is necessary (setEnabled
> toggling after you first get a JSOP_TRAP recompile via JS_SetTrap).

Well, the real reason is that any time you change bytecode, we recompile. So immediately after the two places where we assign to *pc, we recompile.

We can get here via setEnabled; that's because we try to eliminate all overhead when you disable a debugger. But you also get here in the normal case where you're just calling Script.prototype.setBreakpoint.

> Can't you use js_GetScriptLineExtent instead of rewriting it in the form of
> DebuggerScript_getLineCount?

D'oh. Yes. Didn't see that there.

	Jason Orendorff [:jorendorff] Assignee
	Comment 44 • 13 years ago

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/4ebca9c44e56
Rename jsdbg.{h,cpp} to vm/Debugger.{h,cpp}. This addresses a review comment from
brendan in bug 627829 comment 42. 

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/82545b1e4129
Address review comments from brendan (bug 672829 comment 42).

	Brendan Eich [:brendan]
	Comment 45 • 13 years ago

> So that's why I used references. I'll switch to pointers if you like.

No worries, we have references IN THE BUILDING to quote Basil Fawlty ("The Germans").

FWIW I agree with your views on references and EIBTI for C-level languages, but I think we are in the minority around here ;-).

Reviewing again.

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 47 • 13 years ago

I'm splitting attachment 548278 [details] [diff] [review] ("Main, part 1 - jsdbg.cpp and data structures") into 14 parts to spread out the review load.

Again, these parts are not in chronological order or any other logical order. Understanding some bits may require a peek at the jsdbg2 repo, particularly vm/Debugger.h:

hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/file/tip/js/src/vm/Debugger.h

	Jason Orendorff [:jorendorff] Assignee
	Comment 48 • 13 years ago

The new debugger requires the user to specify which globals to debug. Debug mode is automatically turned on for the target compartments.

At the same time we have to keep JSD working, which requires a little cruft that we can delete later.

	Jason Orendorff [:jorendorff] Assignee
	Comment 49 • 13 years ago

See https://wiki.mozilla.org/Debugger#Debuggee_Values

	Jason Orendorff [:jorendorff] Assignee
	Comment 50 • 13 years ago

Completion values, resumption values, and error handling.

https://wiki.mozilla.org/Debugger#Completion_Values
https://wiki.mozilla.org/Debugger#Resumption_Values

The spec for uncaughtExceptionHook is under:
https://wiki.mozilla.org/Debugger#Accessor_Properties_of_the_Debugger_Prototype_Object

Note in particular that uncaughtExceptionHook is different from all the other
hooks. The others are fired in response to debuggee behavior. This one is
called when the debugger screws up.

The spec for uncaughtExceptionHook has changed slightly since we implemented it
(in particular, what happens in the clownshoes case where the
uncaughtExceptionHook itself throws an exception), and the implementation
hasn't caught up yet.

	Jason Orendorff [:jorendorff] Assignee
	Comment 51 • 13 years ago

Debugging hook dispatch code, to fire onDebuggerStatement, onEnterFrame, onExceptionUnwind, and onNewScript. See the corresponding inlines in Debugger.h.

https://wiki.mozilla.org/Debugger#Debugging_hooks

(The "onError" hook in the spec isn't implemented yet.)

	Jason Orendorff [:jorendorff] Assignee
	Comment 52 • 13 years ago

Everything the debugger does is cross-compartment, and as a result the GC code is complicated.

Originally the plan was to use cross-compartment wrappers for everything. But this had a steady stream of drawbacks: wrappers are not transparent for all the inspection operations the debugger should support; you can't make a wrapper of a wrapper, so if the debuggee object you want to look at happens to be wrapper, you risk implicitly creating a new wrapper with different behavior; while it would work OK for objects, wrappers are not transparent to *any* of the operations we would need for scripts; wrapper policy is the embedding's responsibility, so it could be dangerous for the debugger to make any assumptions about the wrappers... just keeping direct pointers to the objects and scripts made for a complicated GC story but it was simpler in other ways.

	Jason Orendorff [:jorendorff] Assignee
	Comment 53 • 13 years ago

https://wiki.mozilla.org/Debugger#The_Debugger_Object

	Jason Orendorff [:jorendorff] Assignee
	Comment 54 • 13 years ago

See https://wiki.mozilla.org/Debugger#Debugger.Script

The "mirabile dictu" comment is being deleted in bug 665167. More to the point,
see bug 674164. Trying to hide the existence of the eval cache from the
debugger was a mistake. It can't be hidden altogether.

	Jason Orendorff [:jorendorff] Assignee
	Comment 55 • 13 years ago

See https://wiki.mozilla.org/Debugger#Debugger.Script

	Jason Orendorff [:jorendorff] Assignee
	Comment 56 • 13 years ago

We still use JSOP_TRAP and adrake's methodjit breakpoint code. This is a rewrite of the high-level data structure.

In the new breakpoint implementation, breakpoints are per-compartment, like the
new watchpoints, not runtime-wide. So they do not need a mutex. Part 11 removes
JSRuntime::debuggerLock.

Again, some cruft is taken on board in order to continue supporting JSD for
now.

	Jason Orendorff [:jorendorff] Assignee
	Comment 57 • 13 years ago

Reimplement jsdbgapi.h trap APIs in terms of new breakpoints. Remove debuggerLock.

	Jason Orendorff [:jorendorff] Assignee
	Comment 58 • 13 years ago

See https://wiki.mozilla.org/Debugger#Debugger.Frame

	Jason Orendorff [:jorendorff] Assignee
	Comment 59 • 13 years ago

See https://wiki.mozilla.org/Debugger#Debugger.Object

The main sad thing here is 
>+    /* Bug: This can cause the debuggee to run! */

In corner cases, mostly involving script Proxy objects, the object
introspection methods can cause debuggee code to run, which is not what jimb
intended when he designed the API. This isn't catastrophic; it just means
debuggee code runs, and it could reenter the debugger or throw exceptions at it.

It would be better to throw an exception in these cases, if the operation would
reenter or do anything fallible (other than allocating memory).

	Jason Orendorff [:jorendorff] Assignee
	Comment 60 • 13 years ago

The end.

	Jason Orendorff [:jorendorff] Assignee
	Comment 61 • 13 years ago

The above 14 patches add up to (approximately) attachment 548278 [details] [diff] [review] ("Main, part 1 - jsdbg.cpp and data structures") — the remaining un-reviewed patch from the first wave. I'll request reviews on some of these now and the rest tomorrow.

	Jason Orendorff [:jorendorff] Assignee
	Comment 62 • 13 years ago

Comment on attachment 552020 [details] [diff] [review]
Main, part 12 - Debugger.Frame

Debugger.Frame is going to evolve; see bug 674165.

	Jason Orendorff [:jorendorff] Assignee
	Comment 63 • 13 years ago

Comment on attachment 552016 [details] [diff] [review]
Main, part 8 - Script internals

Igor, I think a lot of this stuff will be simplified, but it'll be a big merge with your patch in bug 674251 (which I'm reviewing as fast as I can). If you have any suggestions about that, let me know.

	Igor Bukanov
	Comment 64 • 13 years ago

> Igor, I think a lot of this stuff will be simplified, but it'll be a big
> merge with your patch in bug 674251 (which I'm reviewing as fast as I can).
> If you have any suggestions about that, let me know.

JSScript as a GC thing is a useful test case for variable-length GC thing allocator, but I can live without it. So unless that simplifies a lot, lets put bug 674251 on hold until you land your debugger changes.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 65 • 13 years ago

Comment on attachment 552013 [details] [diff] [review]
Main, part 6 - GC

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

I noticed that in JSCompartment::markBreakpointsIteratively, you call isMarked. It would be nice if all the isMarked calls could be converted to IsAboutToBeFinalized.

::: js/src/jsgc.cpp
@@ +2184,5 @@
>  
>      while (read < end) {
>          JSCompartment *compartment = *read++;
>  
> +        if (compartment->isAboutToBeCollected(gckind)) {

Given my comment below about isAboutToBeCollected, as well as the fact that its definition is sort of GC-internal, I'd rather this not be factored out like this. It's very easy to misuse.

::: js/src/vm/Debugger.cpp
@@ +611,5 @@
> +    JS_ASSERT(comp);
> +
> +    typedef HashMap<JSObject *, JSObject *, DefaultHasher<JSObject *>, RuntimeAllocPolicy> Map;
> +    const Map &storage = map;
> +    for (Map::Range r = storage.all(); !r.empty(); r.popFront()) {

Why can't you just iterate over map directly?

@@ +666,5 @@
> + *   1. Mark Debugger objects that are unreachable except for debugger hooks that
> + *      may yet be called.
> + *   2. Mark breakpoint handlers.
> + *
> + * This happens during the incremental long tail of the GC mark phase. This

The phrase "incremental long tail" sounds confusing. How about "the end"?

@@ +692,5 @@
> +        /*
> +         * If this is a single-compartment GC, no compartment can debug itself, so skip
> +         * |comp|. If it's a global GC, then search every live compartment.
> +         */
> +        if (comp ? dc != comp : !dc->isAboutToBeCollected(gckind)) {

I don't think isAboutToBeCollected does what you want it to do. It works by checking if the compartment has any objects in it. This can only ever return false after we've run the sweep phase. So at this point in the GC, isAboutToBeCollected will always return true. Luckily, I think that test was just an optimization, right? I think it would be nicer just to replace this with the following:
    if (comp && dc == comp)
        continue;
Then you don't have to indent so much.

@@ +713,5 @@
> +                     *   - dbg is actually in a compartment being GC'd
> +                     *   - it isn't already marked
> +                     *   - it actually has hooks that might be called
> +                     */
> +                    if ((!comp || obj->compartment() == comp) && !obj->isMarked()) {

I think you can replace the entire condition with IsAboutToBeFinalized.

@@ +777,5 @@
> +}
> +
> +void
> +Debugger::sweepAll(JSContext *cx)
> +{

It looks like this routine is intended to be used only for full GCs, so it would be good to assert !gcCurrentCompartment here.

@@ +782,5 @@
> +    JSRuntime *rt = cx->runtime;
> +    for (JSCList *p = &rt->debuggerList; (p = JS_NEXT_LINK(p)) != &rt->debuggerList;) {
> +        Debugger *dbg = Debugger::fromLinks(p);
> +
> +        if (!dbg->object->isMarked()) {

If you can, it's cleaner to use IsAboutToBeFinalized than to test isMarked. It has some extra checks that are nice to have, and it's easier to grep for.

@@ +802,5 @@
> +        /* For each debuggee being GC'd, detach it from all its debuggers. */
> +        GlobalObjectSet &debuggees = (*c)->getDebuggees();
> +        for (GlobalObjectSet::Enum e(debuggees); !e.empty(); e.popFront()) {
> +            GlobalObject *global = e.front();
> +            if (!global->isMarked())

Again, IsAboutToBeFinalized would be preferable.

@@ +821,5 @@
> +
> +void
> +Debugger::finalize(JSContext *cx, JSObject *obj)
> +{
> +    Debugger *dbg = (Debugger *) obj->getPrivate();

Use fromJSObject?

	Steve Fink [:sfink] [:s:]
	Comment 66 • 13 years ago

Comment on attachment 552009 [details] [diff] [review]
Main, part 3 - Debuggee values

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

I also noticed that js.msg in what I just pulled from http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2 still refers to "Debug.Object" instead of "Debugger.Object".

::: js/src/vm/Debugger.cpp
@@ +250,5 @@
> +        Value owner = dobj->getReservedSlot(JSSLOT_DEBUGOBJECT_OWNER);
> +        if (owner.toObjectOrNull() != object) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                                 owner.isNull()
> +                                 ? JSMSG_DEBUG_OBJECT_PROTO

Is there any other way a Debugger.Object could lose its owner other than by being the proto? I wonder if it would at least be useful to JS_ASSERT_IF(owner.isNull(), dobj == &object->getReservedSlot(JSSLOT_DEBUG_OBJECT_PROTO).toObject())

	Luke Wagner [:luke]
	Comment 67 • 13 years ago

Comment on attachment 552014 [details] [diff] [review]
Main, part 7 - The Debugger constructor

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

In general, vm/Debugger looks really clean; nice job!

I noticed no use of StackIter or iterating over native callees yet, is that for later?

	David Mandelin [:dmandelin]
	Comment 68 • 13 years ago

Comment on attachment 552007 [details] [diff] [review]
Main, part 1 - basics and utils

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

I guess the main things to review here are the top-level API and its comments, independent of the functionality. I have only a few comments/questions below. It looks very good. I particularly like the detailed explanations of the preconditions.

::: js/src/vm/Debugger.h
@@ +92,5 @@
> +    /*
> +     * Weak references to stack frames that are currently on the stack and thus
> +     * necessarily alive. We drop them as soon as they leave the stack (see
> +     * slowPathOnLeaveFrame) and in removeDebuggee.
> +     */

Just to be sure I get it, "weak references" means that the trace function doesn't mark through this set?

@@ +143,5 @@
> +     * Cope with an error or exception in a debugger hook.
> +     *
> +     * If callHook is true, then call the uncaughtExceptionHook, if any. If, in
> +     * addition, vp is non-null, then parse the value returned by
> +     * uncaughtExceptionHook as a resumption value.

Is "resumption value" defined somewhere? (Or is it a well-known term in this context?)

@@ +218,5 @@
> +     * Receive a "new script" event from the engine. A new script was compiled
> +     * or deserialized. If kind is NewHeldScript, obj is the holder
> +     * object. Otherwise kind is NewNonHeldScript and obj is an arbitrary
> +     * object in the same global as the scope in which the script is being
> +     * evaluated.

I think this reads better as:

* Receive a "new script (compiled or deserialized)" event from the 
* engine. If kind is NewHeldScript, obj must be the script holder. 
* Otherwise, kind must be NewNonHeldScript and we must have
* |obj->getGlobal() == script->global()|.

(assuming the last condition really is the correct translation.)

	Luke Wagner [:luke]
	Comment 69 • 13 years ago

Comment on attachment 552020 [details] [diff] [review]
Main, part 12 - Debugger.Frame

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

Very readable, thanks.

::: js/src/vm/Debugger.cpp
@@ +393,5 @@
> +    JS_ASSERT(fp->isScriptFrame());
> +    FrameMap::AddPtr p = frames.lookupForAdd(fp);
> +    if (!p) {
> +        /* Create and populate the Debugger.Frame object. */
> +        JSObject *proto = &object->getReservedSlot(JSSLOT_DEBUG_FRAME_PROTO).toObject();

As a general comment, perhaps you could make your own subclass of JSObject (in the StringObject/GlobalObject style) that provides named accessors instead of manual getReservedSlot use?  I think, in general, we want to move everything in this direction.

@@ +2474,5 @@
> +/* The getter used for each element of frame.arguments. See DebuggerFrame_getArguments. */
> +static JSBool
> +DebuggerArguments_getArg(JSContext *cx, uintN argc, Value *vp)
> +{
> +    JSObject *callee = &CallArgsFromVp(argc, vp).callee();

You do this here, which is nice, but then use vp below.  Perhaps store the CallArgs by value and use throughout the function.  In general, it would be nice if new natives exclusively used CallArgs, easing the possible/eventual work for bug 580337.

@@ +2502,5 @@
> +     * there is no guarantee this object has an ith argument.
> +     */
> +    JS_ASSERT(i >= 0);
> +    if (uintN(i) < fp->numActualArgs())
> +        *vp = fp->actualArgs()[i];

I think you want canonicalActualArg here.

@@ +2522,5 @@
> +
> +    JSObject *argsobj;
> +    if (fp->hasArgs()) {
> +        /* Create an arguments object. */
> +        GlobalObject *global = CallArgsFromVp(argc, vp).callee().getGlobal();

ditto re: CallArgs.

@@ +2661,5 @@
> +{
> +    REQUIRE_ARGC(mode == WithBindings ? "Debugger.Frame.evalWithBindings" : "Debugger.Frame.eval",
> +                 uintN(mode == WithBindings ? 2 : 1));
> +    THIS_FRAME(cx, vp, mode == WithBindings ? "evalWithBindings" : "eval", thisobj, fp);
> +    Debugger *dbg = Debugger::fromChildJSObject(&vp[1].toObject());

CallArgs would look nice here, they could even be passed as a parameter!

	Steve Fink [:sfink] [:s:]
	Comment 70 • 13 years ago

Comment on attachment 552010 [details] [diff] [review]
Main, part 4 - Completion, resumption, and error handling

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

I found the use of AutoCompartment slightly confusing because I immediately expected it to leave the compartment via RAII. I had to trace through the logic to see if it was really right to leave() when an error was detected, for example. But that appears to be the only mechanism we have, and it does exactly what you want.

I wasn't sure from the spec whether the resumption values needed to take *exactly* the forms allowed here (eg it's illegal to use { return: 42, sponsor: "Tesla Motors" }). But this behavior seems best until there's a justification for changing it.

	Jason Orendorff [:jorendorff] Assignee
	Comment 71 • 13 years ago

(In reply to Bill McCloskey (:billm) from comment #65)
> I noticed that in JSCompartment::markBreakpointsIteratively, you call
> isMarked. It would be nice if all the isMarked calls could be converted to
> IsAboutToBeFinalized.

Done. I also changed the isMarked calls in jswatchpoint.cpp, which are my fault as well.

I have a question about this code:

        // Mark jsdbgapi state if any. [...]
        [...]
            if (site->trapClosure.isObject() &&
                IsAboutToBeFinalized(cx, site->trapClosure.toObject()))
            {
                markedAny = true;
            }
            MarkValue(trc, site->trapClosure, "trap closure");

What if site->trapClosure is a string that IsAboutToBeFinalized? Do I need to set markedAny = true in that case?

> Given my comment below about isAboutToBeCollected, as well as the fact that
> its definition is sort of GC-internal, I'd rather this not be factored out
> like this. It's very easy to misuse.

Good eye! I changed it back, as suggested.

> ::: js/src/vm/Debugger.cpp
> @@ +611,5 @@
> > +    JS_ASSERT(comp);
> > +
> > +    typedef HashMap<JSObject *, JSObject *, DefaultHasher<JSObject *>, RuntimeAllocPolicy> Map;
> > +    const Map &storage = map;
> > +    for (Map::Range r = storage.all(); !r.empty(); r.popFront()) {
> 
> Why can't you just iterate over map directly?

HashMap<>::Range is private in WeakMap<>. Bizarre but true. And intentional. You're really not supposed to iterate over WeakMaps. Yeah, this exact think I'm doing in this code. Don't do that.

I dunno, maybe Range should be public, since apparently some users think they're so smart. Or maybe this markKeysInCompartment function should be a public member of cross-compartment WeakMaps (but I'm not sure this algorithm generalizes that well). If you like, I can file a follow-up bug.

> @@ +666,5 @@
> > + *   1. Mark Debugger objects that are unreachable except for debugger hooks that
> > + *      may yet be called.
> > + *   2. Mark breakpoint handlers.
> > + *
> > + * This happens during the incremental long tail of the GC mark phase. This
> 
> The phrase "incremental long tail" sounds confusing. How about "the end"?

That doesn't quite say what I want. I changed it to "the iterative part".

I took the rest of your suggestions.

  Address review comments from billm (bug 672829 comment 65). 
  http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/746e5c170b36

	Jason Orendorff [:jorendorff] Assignee
	Comment 72 • 13 years ago

(In reply to Steve Fink [:sfink] from comment #66)
> Is there any other way a Debugger.Object could lose its owner other than by
> being the proto?

No, that slot never ever changes.

> I wonder if it would at least be useful to
> JS_ASSERT_IF(owner.isNull(), dobj ==
> &object->getReservedSlot(JSSLOT_DEBUG_OBJECT_PROTO).toObject())

I don't think so. I'm so confident in it I'd rather not assert it.

(In case that seems insane: To me, asserting something suggests that it's not totally trivial, that there's at least a some small reason for asserting it. It may sound ironic and backwards, but it's true: we really do only assert things that might be false someday. If you were to come across something like:

    if (x > 0) {
        ...
    } else {
        JS_ASSERT(x <= 0);
        ...
    }

you'd be surprised, right? You might even go check the type of x to make sure the assertion is really saying that. I don't want to give anyone that "what--that can happen?!" feeling.)

  Fix error messages to address review comments from sfink (comment 66).
  http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/d714c406a71c

	Jason Orendorff [:jorendorff] Assignee
	Comment 73 • 13 years ago

(In reply to Luke Wagner [:luke] from comment #67)
> In general, vm/Debugger looks really clean; nice job!
> 
> I noticed no use of StackIter or iterating over native callees yet, is that
> for later?

Yes. It'll happen in bug 674165.

	Jason Orendorff [:jorendorff] Assignee
	Comment 74 • 13 years ago

(In reply to David Mandelin from comment #68)
> ::: js/src/vm/Debugger.h
> @@ +92,5 @@
> > +    /*
> > +     * Weak references to stack frames that are currently on the stack and thus
> > +     * necessarily alive. We drop them as soon as they leave the stack (see
> > +     * slowPathOnLeaveFrame) and in removeDebuggee.
> > +     */
> 
> Just to be sure I get it, "weak references" means that the trace function
> doesn't mark through this set?

Hmmm. Good question. Certainly the Debugger's trace function never marks JSStackFrames (the keys of this table); they're either on the stack or they're gone. But it actually does mark Debugger.Frame objects (the values) through this set. Like a WeakMap (but the implementation has to be different since JSStackFrames aren't gc things).

The comment should be improved. I'll look at it tomorrow morning.

> @@ +143,5 @@
> > +     * Cope with an error or exception in a debugger hook.
> > +     *
> > +     * If callHook is true, then call the uncaughtExceptionHook, if any. If, in
> > +     * addition, vp is non-null, then parse the value returned by
> > +     * uncaughtExceptionHook as a resumption value.
> 
> Is "resumption value" defined somewhere? (Or is it a well-known term in this
> context?)

Yes, it's a very specific technical term:
  https://wiki.mozilla.org/Debugger#Resumption_Values

parseResumptionValue should have its own comment pointing this out.

> @@ +218,5 @@
> > +     * Receive a "new script" event from the engine. A new script was compiled
> > +     * or deserialized. If kind is NewHeldScript, obj is the holder
> > +     * object. Otherwise kind is NewNonHeldScript and obj is an arbitrary
> > +     * object in the same global as the scope in which the script is being
> > +     * evaluated.
> 
> I think this reads better as:
> 
> * Receive a "new script (compiled or deserialized)" event from the 
> * engine. If kind is NewHeldScript, obj must be the script holder. 
> * Otherwise, kind must be NewNonHeldScript and we must have
> * |obj->getGlobal() == script->global()|.
> 
> (assuming the last condition really is the correct translation.)

Yes, that is much better! I don't see JSScript::global(), though. Is there such a thing?

	Jason Orendorff [:jorendorff] Assignee
	Comment 75 • 13 years ago

(In reply to David Mandelin from comment #68)
> I think this reads better as:
> 
> * Receive a "new script (compiled or deserialized)" event from the 
> * engine. If kind is NewHeldScript, obj must be the script holder. 
> * Otherwise, kind must be NewNonHeldScript and we must have
> * |obj->getGlobal() == script->global()|.
> 
> (assuming the last condition really is the correct translation.)

After bug 665167 this will read:

    /*
     * Receive a "new script" event from the engine. A new script was compiled
     * or deserialized.
     */
    void fireNewScript(JSContext *cx, JSScript *script);

The obj and NewScriptKind parameters are gone.

Address review comments from dmandelin (bug 672829 comment 68).
http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/cd0e3abdaed5

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 76 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #71)
> I have a question about this code:
> 
>         // Mark jsdbgapi state if any. [...]
>         [...]
>             if (site->trapClosure.isObject() &&
>                 IsAboutToBeFinalized(cx, site->trapClosure.toObject()))
>             {
>                 markedAny = true;
>             }
>             MarkValue(trc, site->trapClosure, "trap closure");
> 
> What if site->trapClosure is a string that IsAboutToBeFinalized? Do I need
> to set markedAny = true in that case?

Yeah, I guess so. IsAboutToBeFinalized correctly handles static strings, so you should just be able to test if isMarkable() and, if true, pass toGCThing() to IsAboutToBeFinalized.

> > Why can't you just iterate over map directly?
> 
> HashMap<>::Range is private in WeakMap<>. Bizarre but true. And intentional.
> You're really not supposed to iterate over WeakMaps. Yeah, this exact think
> I'm doing in this code. Don't do that.

I see. It seems like you'd have to go through some contortions to make that code look like it belongs inside WeakMap. Maybe just put a comment about Range being private?

	Andrew Drake [:adrake]
	Comment 77 • 13 years ago

Comment on attachment 552019 [details] [diff] [review]
Main, part 11 - Old breakpoints

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

If I could r+ this one twice I would.

	Igor Bukanov
	Comment 78 • 13 years ago

Comment on attachment 552016 [details] [diff] [review]
Main, part 8 - Script internals

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

With JSScript as a GC things this will be simplified, but that is orthogonal to the review here.

	Jason Orendorff [:jorendorff] Assignee
	Comment 79 • 13 years ago

(In reply to Luke Wagner [:luke] from comment #69)
> As a general comment, perhaps you could make your own subclass of JSObject
> (in the StringObject/GlobalObject style) that provides named accessors
> instead of manual getReservedSlot use?

Filed bug 678236.

> In general, it would be
> nice if new natives exclusively used CallArgs, easing the possible/eventual
> work for bug 580337.

Filed bug 678201 with patches for this and all the other CallArgs comments.

> > +    if (uintN(i) < fp->numActualArgs())
> > +        *vp = fp->actualArgs()[i];
> 
> I think you want canonicalActualArg here.

Fixed:
http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/e80ea91176e0

	Jason Orendorff [:jorendorff] Assignee
	Comment 80 • 13 years ago

(In reply to Steve Fink [:sfink] from comment #70)
> I found the use of AutoCompartment slightly confusing because I immediately
> expected it to leave the compartment via RAII. I had to trace through the
> logic to see if it was really right to leave() when an error was detected,
> for example. But that appears to be the only mechanism we have, and it does
> exactly what you want.

Yeah. It's clear as mud. The comment in Debugger.h says,

     ...
     * This always calls ac.leave(); ac is a parameter because this method must
     * do some things in the debugger compartment and some things in the
     * debuggee compartment.
     */
    JSTrapStatus handleUncaughtException(AutoCompartment &ac, Value *vp, bool
        callHook);

Suggestions welcome. :-|

	Jason Orendorff [:jorendorff] Assignee
	Comment 81 • 13 years ago

(In reply to Bill McCloskey (:billm) from comment #76)
> (In reply to Jason Orendorff [:jorendorff] from comment #71)
> > What if site->trapClosure is a string that IsAboutToBeFinalized? Do I need
> > to set markedAny = true in that case?
> 
> Yeah, I guess so. IsAboutToBeFinalized correctly handles static strings, so
> you should just be able to test if isMarkable() and, if true, pass
> toGCThing() to IsAboutToBeFinalized.
...
> [...] Maybe just put a comment about Range being private?

These two changes are in:
  Address review comments from billm in bug 672829 comment 76.
  hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/718fd6b1d535

	Brian Hackett [Laid off!]
	Comment 82 • 13 years ago

Comment on attachment 552021 [details] [diff] [review]
Main, part 13 - Debugger.Object

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

Looks great.

::: js/src/vm/Debugger.cpp
@@ +2975,5 @@
> +        /* Rewrap the debuggee values in desc for the debugger. */
> +        if (!dbg->wrapDebuggeeValue(cx, &desc.value))
> +            return false;
> +        if (desc.attrs & JSPROP_GETTER) {
> +            Value get = ObjectOrNullValue(CastAsObject(desc.getter));

Isn't it impossible for desc.getter (or desc.setter below) to be NULL here?

@@ +3067,5 @@
> +}
> +
> +/*
> + * Rewrap *idp and the fields of *desc for the current compartment.  Also:
> + * defining a property on a proxy requiers the pd field to contain a descriptor

typo

@@ +3198,5 @@
> +        ok = obj->freeze(cx);
> +    } else {
> +        JS_ASSERT(op == PreventExtensions);
> +        if (!obj->isExtensible())
> +            return true;

vp hasn't been set yet.

	Jason Orendorff [:jorendorff] Assignee
	Comment 83 • 13 years ago

(In reply to Brian Hackett from comment #82)
> > +        if (desc.attrs & JSPROP_GETTER) {
> > +            Value get = ObjectOrNullValue(CastAsObject(desc.getter));
> 
> Isn't it impossible for desc.getter (or desc.setter below) to be NULL here?

Nope.

var obj = {};
Object.defineProperty(obj, 'x', {get: undefined, set: function (x) {}});
dumpObject(obj);

object 0x101811028
class 0x100489c20 Object
flags: none
proto <Object at 0x1018000d0>
parent <global object at 0x101800040>
properties:
    ((Shape *) 0x10180f840) readonly permanent shared getterValue=0x0 setterValue=0x101806190 "x": slot -1

The output "getterValue=0x0" means that the JSPROP_GETTER bit is set. If you omit "get: undefined" then the bit isn't set. I don't know if this is intentional, but that's how it is.

Good catch about vp.

Address review comments from bhackett (bug 672829 comment 82).
hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/8944653199f1

	Brian Hackett [Laid off!]
	Comment 84 • 13 years ago

Comment on attachment 552017 [details] [diff] [review]
Main, part 9 - Debugger.Script

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

Stealing.

Nice.  The only thing that bugs me is the O(n)-ness of some operations --- IsValidBytecodeOffset and the natives which use it, and getLineOffsets.  I think these could have problems if someone is debugging one of the gigantic scripts which show up from time to time.  After JM merges it will be simple to make IsValidBytecodeOffset O(1), by using the script->analysis() info cleared on each GC and regenerated on demand, and a similar thing could be done to cache line offset data.  Though the latter should only be done if it does turn out to be a problem.

::: js/src/vm/Debugger.cpp
@@ +1904,5 @@
> +
> +    JSCompartment *comp = script->compartment;
> +    jsbytecode *pc = script->code + offset;
> +    BreakpointSite *site = comp->getOrCreateBreakpointSite(cx, script, pc, holder);
> +    if (site->inc(cx)) {

Needs a NULL check.

	Jason Orendorff [:jorendorff] Assignee
	Comment 85 • 13 years ago

(In reply to Brian Hackett from comment #84)
> Nice.  The only thing that bugs me is the O(n)-ness of some operations ---
> IsValidBytecodeOffset and the natives which use it, and getLineOffsets.  I
> think these could have problems if someone is debugging one of the gigantic
> scripts which show up from time to time.  After JM merges it will be simple
> to make IsValidBytecodeOffset O(1), by using the script->analysis() info
> cleared on each GC and regenerated on demand, and a similar thing could be
> done to cache line offset data.  Though the latter should only be done if it
> does turn out to be a problem.

All that makes sense to me.

> Needs a NULL check.

Sure does. Thanks.

hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2/rev/afe34d24652c

	Brendan Eich [:brendan]
	Comment 86 • 13 years ago

Comment on attachment 552017 [details] [diff] [review]
Main, part 9 - Debugger.Script

Looks great. One question: how sparse is FlowGraphSummary as a vector? More general q: how's the memory overhead of jsdbg2 when enabled?

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 87 • 13 years ago

(In reply to Brendan Eich [:brendan] from comment #86)
> Looks great. One question: how sparse is FlowGraphSummary as a vector? More
> general q: how's the memory overhead of jsdbg2 when enabled?

FlowGraphSummary is pretty sparse. Only a third or so of the bytes in a typical JSScript are opcodes (the rest being operands), and most opcodes aren't jump targets. But it was quick to write and the code is simple.

Memory overhead should be low. When jsdbg2 is enabled, the target compartment is put into debug mode. That causes all its jitcode to be discarded; the re-compiled jitcode is a bit larger, but probably not much. I haven't measured it. If the debugger does nothing, there's no further overhead--but a realistic debugger will at least track each debuggee script. That's one Debugger.Script object per JSScript the debuggee compiles.

	Jason Orendorff [:jorendorff] Assignee
	Comment 88 • 13 years ago

Also: the FlowGraphSummary is discarded once getAllOffsets/getLineOffsets has built the result Array--which will be sparse or dense as the Array code chooses.

	Jason Orendorff [:jorendorff] Assignee
	Comment 89 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/080fece621e4

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 90 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/d25610a7d260

I'm not going through and marking the 800-something changesets with their m-c changeset links.  In the future, please land project branches directly on mozilla-central instead of on mozilla-inbound.

	Jason Orendorff [:jorendorff] Assignee
	Comment 91 • 13 years ago

khuey, thanks for landing this. I asked about it on #developers twice, and the impression I got was that it was OK with everyone. mbrubeck and rnewman had no objection. I said I'd mark the bugs myself.

	Jim Blandy :jimb
	Comment 92 • 13 years ago

Note that this API is documented here:
https://wiki.mozilla.org/Debugger

However, that documentation is half spec --- it includes stuff we haven't implemented yet, and what's done is not clearly delimited from what's yet to be done. But naturally, feel free to copy anything that's useful.

	Eric Shepherd [:sheppy]
	Comment 93 • 13 years ago

We have docs for this now, although more example type material is needed in the Guide section. That said, the stuff we have should be pretty good, and we'll worry about adding more examples once there's some stuff on which to base that, so I'm marking this as done to get it out of my face for now. :)

https://developer.mozilla.org/en/SpiderMonkey/JS_Debugger_API_Reference
https://developer.mozilla.org/en/SpiderMonkey/JS_Debugger_API_Guide