Address review comments from brendan (bug 672829 comment 42).
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 03 Aug 2011 19:43:39 -0500
changeset 75232 82545b1e4129e2b5fb970e036aeba492334b1a3d
parent 75231 70955e4b30173a7998ad75621ee2577b79b46cec
child 75233 3a3b3f135185b91b8089d0b596c7e0609c02e0f8
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
bugs672829
milestone8.0a1
Address review comments from brendan (bug 672829 comment 42).
js/src/jscntxt.h
js/src/jscompartment.cpp
js/src/jscompartment.h
js/src/jsscript.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -540,18 +540,25 @@ struct JSRuntime {
     PRLock              *rtLock;
 #ifdef DEBUG
     void *              rtLockOwner;
 #endif
 
     /* Used to synchronize down/up state change; protected by gcLock. */
     PRCondVar           *stateChange;
 
+    /*
+     * Mapping from NSPR thread identifiers to JSThreads.
+     *
+     * This map can be accessed by the GC thread; or by the thread that holds
+     * gcLock, if GC is not running.
+     */
     JSThread::Map       threads;
 #endif /* JS_THREADSAFE */
+
     uint32              debuggerMutations;
 
     /*
      * Security callbacks set on the runtime are used by each context unless
      * an override is set on the context.
      */
     JSSecurityCallbacks *securityCallbacks;
 
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -671,17 +671,17 @@ JSCompartment::setDebugModeFromC(JSConte
     if (enabledBefore != enabledAfter) {
         onStack = hasScriptsOnStack(cx);
         if (b && onStack) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NOT_IDLE);
             return false;
         }
     }
 
-    debugModeBits = (debugModeBits & ~uintN(DebugFromC)) | (b * DebugFromC);
+    debugModeBits = (debugModeBits & ~uintN(DebugFromC)) | (b ? DebugFromC : 0);
     JS_ASSERT(debugMode() == enabledAfter);
     if (enabledBefore != enabledAfter && !onStack)
         updateForDebugMode(cx);
     return true;
 }
 
 void
 JSCompartment::updateForDebugMode(JSContext *cx)
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -553,16 +553,17 @@ struct JS_FRIEND_API(JSCompartment) {
      */
     js::GlobalObjectSet              debuggees;
 
   public:
     js::BreakpointSiteMap            breakpointSites;
 
   private:
     JSCompartment *thisForCtor() { return this; }
+
   public:
     js::MathCache *getMathCache(JSContext *cx) {
         return mathCache ? mathCache : allocMathCache(cx);
     }
 
 #ifdef JS_TRACER
     bool hasTraceMonitor() {
         return !!traceMonitor_;
@@ -607,16 +608,17 @@ struct JS_FRIEND_API(JSCompartment) {
     bool setDebugModeFromC(JSContext *cx, bool b);
 
     js::BreakpointSite *getBreakpointSite(jsbytecode *pc);
     js::BreakpointSite *getOrCreateBreakpointSite(JSContext *cx, JSScript *script, jsbytecode *pc,
                                                   JSObject *scriptObject);
     void clearBreakpointsIn(JSContext *cx, js::Debugger *dbg, JSScript *script, JSObject *handler);
     void clearTraps(JSContext *cx, JSScript *script);
     bool markBreakpointsIteratively(JSTracer *trc);
+
   private:
     void sweepBreakpoints(JSContext *cx);
 
   public:
     js::WatchpointMap *watchpointMap;
 };
 
 #define JS_SCRIPTS_TO_GC(cx)    ((cx)->compartment->scriptsToGC)
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -516,22 +516,16 @@ struct JSScript {
          * A script object of class js_ScriptClass, to ensure the script is GC'd.
          * - All scripts returned by JSAPI functions (JS_CompileScript,
          *   JS_CompileFile, etc.) have these objects.
          * - Function scripts never have script objects; such scripts are owned
          *   by their function objects.
          * - Temporary scripts created by obj_eval, JS_EvaluateScript, and
          *   similar functions never have these objects; such scripts are
          *   explicitly destroyed by the code that created them.
-         * Debugging API functions (JSDebugHooks::newScriptHook;
-         * JS_GetFunctionScript) may reveal sans-script-object Function and
-         * temporary scripts to clients, but clients must never call
-         * JS_NewScriptObject on such scripts: doing so would double-free them,
-         * once from the explicit call to js_DestroyScript, and once when the
-         * script object is garbage collected.
          */
         JSObject    *object;
         JSScript    *nextToGC;  /* next to GC in rt->scriptsToGC list */
     } u;
 
 #ifdef CHECK_SCRIPT_OWNER
     JSThread        *owner;     /* for thread-safe life-cycle assertions */
 #endif
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -287,17 +287,16 @@ bool
 BreakpointSite::hasBreakpoint(Breakpoint *bp)
 {
     for (Breakpoint *p = firstBreakpoint(); p; p = p->nextInSite())
         if (p == bp)
             return true;
     return false;
 }
 
-
 Breakpoint::Breakpoint(Debugger *debugger, BreakpointSite *site, JSObject *handler)
     : debugger(debugger), site(site), handler(handler)
 {
     JS_APPEND_LINK(&debuggerLinks, &debugger->breakpoints);
     JS_APPEND_LINK(&siteLinks, &site->breakpoints);
 }
 
 Breakpoint *
@@ -912,23 +911,23 @@ Debugger::onTrap(JSContext *cx, Value *v
     vp->setInt32(op);
     return JSTRAP_CONTINUE;
 }
 
 
 /*** Debugger JSObjects **************************************************************************/
 
 void
-Debugger::markKeysInCompartment(JSTracer *tracer, ObjectWeakMap &map)
+Debugger::markKeysInCompartment(JSTracer *tracer, const ObjectWeakMap &map)
 {
     JSCompartment *comp = tracer->context->runtime->gcCurrentCompartment;
     JS_ASSERT(comp);
 
     typedef HashMap<JSObject *, JSObject *, DefaultHasher<JSObject *>, RuntimeAllocPolicy> Map;
-    Map &storage = map;
+    const Map &storage = map;
     for (Map::Range r = storage.all(); !r.empty(); r.popFront()) {
         JSObject *key = r.front().key;
         if (key->compartment() == comp && IsAboutToBeFinalized(tracer->context, key))
             js::gc::MarkObject(tracer, *key, "cross-compartment WeakMap key");
     }
 }
 
 /*
@@ -1472,25 +1471,25 @@ Debugger::construct(JSContext *cx, uintN
             return false;
     }
 
     vp->setObject(*obj);
     return true;
 }
 
 bool
-Debugger::addDebuggeeGlobal(JSContext *cx, GlobalObject *obj)
+Debugger::addDebuggeeGlobal(JSContext *cx, GlobalObject *global)
 {
-    JSCompartment *debuggeeCompartment = obj->compartment();
+    JSCompartment *debuggeeCompartment = global->compartment();
 
     /*
-     * Check for cycles. If obj's compartment is reachable from this Debugger
-     * object's compartment by following debuggee-to-debugger links, then
-     * adding obj would create a cycle. (Typically nobody is debugging the
-     * debugger, in which case we zip through this code without looping.)
+     * Check for cycles. If global's compartment is reachable from this
+     * Debugger object's compartment by following debuggee-to-debugger links,
+     * then adding global would create a cycle. (Typically nobody is debugging
+     * the debugger, in which case we zip through this code without looping.)
      */
     Vector<JSCompartment *> visited(cx);
     if (!visited.append(object->compartment()))
         return false;
     for (size_t i = 0; i < visited.length(); i++) {
         JSCompartment *c = visited[i];
         if (c == debuggeeCompartment) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_LOOP);
@@ -1516,39 +1515,37 @@ Debugger::addDebuggeeGlobal(JSContext *c
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NOT_IDLE);
         return false;
     }
 
     /*
      * Each debugger-debuggee relation must be stored in up to three places.
      * JSCompartment::addDebuggee enables debug mode if needed.
      */
-    AutoCompartment ac(cx, obj);
+    AutoCompartment ac(cx, global);
     if (!ac.enter())
         return false;
-    GlobalObject::DebuggerVector *v = obj->getOrCreateDebuggers(cx);
+    GlobalObject::DebuggerVector *v = global->getOrCreateDebuggers(cx);
     if (!v || !v->append(this)) {
         js_ReportOutOfMemory(cx);
-        goto fail1;
-    }
-    if (!debuggees.put(obj)) {
-        js_ReportOutOfMemory(cx);
-        goto fail2;
+    } else {
+        if (!debuggees.put(global)) {
+            js_ReportOutOfMemory(cx);
+        } else {
+            if (global->getDebuggers()->length() > 1)
+                return true;
+            if (debuggeeCompartment->addDebuggee(cx, global))
+                return true;
+
+            /* Maintain consistency on error. */
+            debuggees.remove(global);
+        }
+        JS_ASSERT(v->back() == this);
+        v->popBack();
     }
-    if (obj->getDebuggers()->length() == 1 && !debuggeeCompartment->addDebuggee(cx, obj))
-        goto fail3;
-    return true;
-
-    /* Maintain consistency on error. */
-fail3:
-    debuggees.remove(obj);
-fail2:
-    JS_ASSERT(v->back() == this);
-    v->popBack();
-fail1:
     return false;
 }
 
 void
 Debugger::removeDebuggeeGlobal(JSContext *cx, GlobalObject *global,
                                GlobalObjectSet::Enum *compartmentEnum,
                                GlobalObjectSet::Enum *debugEnum)
 {
@@ -1876,35 +1873,18 @@ DebuggerScript_getStartLine(JSContext *c
     return true;
 }
 
 static JSBool
 DebuggerScript_getLineCount(JSContext *cx, uintN argc, Value *vp)
 {
     THIS_DEBUGSCRIPT_LIVE_SCRIPT(cx, vp, "get lineCount", obj, script);
 
-    /*
-     * A script's line count is not stored, so we calculate it by reading all
-     * the source notes for the whole script.
-     */
-    size_t line = script->lineno, maxLine = line;
-    for (jssrcnote *sn = script->notes(); !SN_IS_TERMINATOR(sn); sn = SN_NEXT(sn)) {
-        JSSrcNoteType type = (JSSrcNoteType) SN_TYPE(sn);
-        if (type == SRC_SETLINE)
-            line = size_t(js_GetSrcNoteOffset(sn, 0));
-        else if (type == SRC_NEWLINE)
-            line++;
-        else
-            continue;
-
-        if (line > maxLine)
-            maxLine = line;
-    }
-
-    vp->setNumber(jsdouble(maxLine + 1 - script->lineno));
+    uintN maxLine = js_GetScriptLineExtent(script);
+    vp->setNumber(jsdouble(maxLine));
     return true;
 }
 
 static JSBool
 DebuggerScript_getLive(JSContext *cx, uintN argc, Value *vp)
 {
     THIS_DEBUGSCRIPT_SCRIPT(cx, vp, "get live", obj, script);
     vp->setBoolean(!!script);
@@ -2111,17 +2091,16 @@ class FlowGraphSummary : public Vector<s
                     pc += step;
                 }
             }
 
             prevOp = op;
             prevLine = lineno;
         }
 
-
         return true;
     }
 };
 
 static JSBool
 DebuggerScript_getAllOffsets(JSContext *cx, uintN argc, Value *vp)
 {
     THIS_DEBUGSCRIPT_LIVE_SCRIPT(cx, vp, "getAllOffsets", obj, script);
@@ -2246,26 +2225,23 @@ DebuggerScript_setBreakpoint(JSContext *
 
     JSObject *handler = NonNullObject(cx, vp[3]);
     if (!handler)
         return false;
 
     JSCompartment *comp = script->compartment;
     jsbytecode *pc = script->code + offset;
     BreakpointSite *site = comp->getOrCreateBreakpointSite(cx, script, pc, holder);
-    if (!site->inc(cx))
-        goto fail1;
-    if (!cx->runtime->new_<Breakpoint>(dbg, site, handler))
-        goto fail2;
-    vp->setUndefined();
-    return true;
-
-fail2:
-    site->dec(cx);
-fail1:
+    if (site->inc(cx)) {
+        if (cx->runtime->new_<Breakpoint>(dbg, site, handler)) {
+            vp->setUndefined();
+            return true;
+        }
+        site->dec(cx);
+    }
     site->destroyIfEmpty(cx->runtime, NULL);
     return false;
 }
 
 static JSBool
 DebuggerScript_getBreakpoints(JSContext *cx, uintN argc, Value *vp)
 {
     THIS_DEBUGSCRIPT_LIVE_SCRIPT(cx, vp, "getBreakpoints", obj, script);
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -137,18 +137,18 @@ class Debugger {
     bool addDebuggeeGlobal(JSContext *cx, GlobalObject *obj);
     void removeDebuggeeGlobal(JSContext *cx, GlobalObject *global,
                               GlobalObjectSet::Enum *compartmentEnum,
                               GlobalObjectSet::Enum *debugEnum);
 
     /*
      * Cope with an error or exception in a debugger hook.
      *
-     * If callHook is true, then call the uncaughtExceptionHook, if any. If
-     * additionally vp is non-null, then parse the value returned by
+     * 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.
      *
      * If there is no uncaughtExceptionHook, or if it fails, report and clear
      * the pending exception on ac.context and return JSTRAP_ERROR.
      *
      * 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.
@@ -156,17 +156,17 @@ class Debugger {
     JSTrapStatus handleUncaughtException(AutoCompartment &ac, Value *vp, bool callHook);
     JSTrapStatus parseResumptionValue(AutoCompartment &ac, bool ok, const Value &rv, Value *vp,
                                       bool callHook = true);
     JSObject *unwrapDebuggeeArgument(JSContext *cx, Value *vp);
 
     static void traceObject(JSTracer *trc, JSObject *obj);
     void trace(JSTracer *trc);
     static void finalize(JSContext *cx, JSObject *obj);
-    static void markKeysInCompartment(JSTracer *tracer, ObjectWeakMap &map);
+    static void markKeysInCompartment(JSTracer *tracer, const ObjectWeakMap &map);
 
     static Class jsclass;
 
     static JSBool getEnabled(JSContext *cx, uintN argc, Value *vp);
     static JSBool setEnabled(JSContext *cx, uintN argc, Value *vp);
     static JSBool getHookImpl(JSContext *cx, uintN argc, Value *vp, Hook which);
     static JSBool setHookImpl(JSContext *cx, uintN argc, Value *vp, Hook which);
     static JSBool getOnDebuggerStatement(JSContext *cx, uintN argc, Value *vp);
@@ -246,19 +246,19 @@ class Debugger {
      *   * it is in the middle of dispatching an event (the event dispatching
      *     code roots it in this case); OR
      *   * it is enabled, and it is debugging at least one live compartment,
      *     and at least one of the following is true:
      *       - it has a debugger hook installed
      *       - it has a breakpoint set on a live script
      *       - it has a watchpoint set on a live object.
      *
-     * The last case is handled by the mark() method. If it finds any Debugger
-     * objects that are definitely live but not yet marked, it marks them and
-     * returns true. If not, it returns false.
+     * Debugger::mark handles the last case. If it finds any Debugger objects
+     * that are definitely live but not yet marked, it marks them and returns
+     * true. If not, it returns false.
      */
     static void markCrossCompartmentDebuggerObjectReferents(JSTracer *tracer);
     static bool mark(GCMarker *trc, JSGCInvocationKind gckind);
     static void sweepAll(JSContext *cx);
     static void detachAllDebuggersFromGlobal(JSContext *cx, GlobalObject *global,
                                              GlobalObjectSet::Enum *compartmentEnum);
 
     static inline void onEnterFrame(JSContext *cx);
@@ -273,49 +273,65 @@ class Debugger {
     /************************************* Functions for use by Debugger.cpp. */
 
     inline bool observesEnterFrame() const;
     inline bool observesNewScript() const;
     inline bool observesScope(JSObject *obj) const;
     inline bool observesFrame(StackFrame *fp) const;
 
     /*
-     * Precondition: *vp is a value from a debuggee compartment and cx is in
-     * the debugger's compartment.
+     * Like cx->compartment->wrap(cx, vp), but for the debugger compartment.
      *
-     * Wrap *vp for the debugger compartment, wrap it in a Debugger.Object if it's
-     * an object, store the result in *vp, and return true.
+     * 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);
 
     /*
-     * NOT the inverse of wrapDebuggeeValue.
+     * Unwrap a Debug.Object, without rewrapping it for any particular debuggee
+     * compartment.
      *
-     * Precondition: cx is in the debugger compartment. *vp is a value in that
-     * compartment. (*vp is a "debuggee value", meaning it is the debugger's
-     * reflection of a value in the debuggee.)
+     * 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.
      *
-     * The value is not rewrapped for any debuggee compartment.
+     * 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);
 
     /* Store the Debugger.Frame object for the frame fp in *vp. */
     bool getScriptFrame(JSContext *cx, StackFrame *fp, Value *vp);
 
     /*
      * Precondition: we are in the debuggee compartment (ac is entered) and ok
      * is true if the operation in the debuggee compartment succeeded, false on
      * error or exception.
      *
-     * Postcondition: we are in the debugger compartment (ac is not entered)
-     * whether creating the new completion value succeeded or not.
+     * Postcondition: we are in the debugger compartment, having called
+     * ac.leave() even if an error occurred.
      *
      * On success, a completion value is in vp and ac.context does not have a
      * pending exception. (This ordinarily returns true even if the ok argument
      * is false.)
      */
     bool newCompletionValue(AutoCompartment &ac, bool ok, Value val, Value *vp);
 
     /*
@@ -410,17 +426,16 @@ class Breakpoint {
 
 Debugger *
 Debugger::fromLinks(JSCList *links)
 {
     unsigned char *p = reinterpret_cast<unsigned char *>(links);
     return reinterpret_cast<Debugger *>(p - offsetof(Debugger, link));
 }
 
-
 Breakpoint *
 Debugger::firstBreakpoint() const
 {
     if (JS_CLIST_IS_EMPTY(&breakpoints))
         return NULL;
     return Breakpoint::fromDebuggerLinks(JS_NEXT_LINK(&breakpoints));
 }