Address review comments from brendan (bug 627829 comment 35).
authorJason Orendorff <jorendorff@mozilla.com>
Fri, 22 Jul 2011 10:54:36 -0500
changeset 75227 8c3a259cccf38ff521ae78f2f044e00ec701db85
parent 75226 8998a25f3887f8038426aaa4a050368d75dd499b
child 75228 4ebca9c44e56db78fddf6ada10d86a048ade8a99
push id2
push userbsmedberg@mozilla.com
push dateFri, 19 Aug 2011 14:38:13 +0000
bugs627829
milestone8.0a1
Address review comments from brendan (bug 627829 comment 35).
js/src/jscntxt.h
js/src/jscompartment.cpp
js/src/jscompartment.h
js/src/jsdbg.cpp
js/src/jsdbg.h
js/src/jstracer.cpp
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -501,19 +501,17 @@ struct JSRuntime {
     JSFlatString        *emptyString;
 
     /* List of active contexts sharing this runtime; protected by gcLock. */
     JSCList             contextList;
 
     /* Per runtime debug hooks -- see jsprvtd.h and jsdbgapi.h. */
     JSDebugHooks        globalDebugHooks;
 
-    /*
-     * Right now, we only support runtime-wide debugging.
-     */
+    /* If true, new compartments are initially in debug mode. */
     bool                debugMode;
 
 #ifdef JS_TRACER
     /* True if any debug hooks not supported by the JIT are enabled. */
     bool debuggerInhibitsJIT() const {
         return (globalDebugHooks.interruptHook ||
                 globalDebugHooks.callHook);
     }
@@ -546,20 +544,19 @@ struct JSRuntime {
 #ifdef DEBUG
     void *              rtLockOwner;
 #endif
 
     /* Used to synchronize down/up state change; protected by gcLock. */
     PRCondVar           *stateChange;
 
     /*
-     * 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.
+     * 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.
      */
     PRLock              *debuggerLock;
 
     JSThread::Map       threads;
 #endif /* JS_THREADSAFE */
     uint32              debuggerMutations;
 
     /*
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -746,30 +746,31 @@ JSCompartment::getBreakpointSite(jsbytec
     return p ? p->value : NULL;
 }
 
 BreakpointSite *
 JSCompartment::getOrCreateBreakpointSite(JSContext *cx, JSScript *script, jsbytecode *pc, JSObject *scriptObject)
 {
     JS_ASSERT(script->code <= pc);
     JS_ASSERT(pc < script->code + script->length);
+    JS_ASSERT_IF(scriptObject, scriptObject->isScript() || scriptObject->isFunction());
+    JS_ASSERT_IF(scriptObject && scriptObject->isFunction(),
+                 scriptObject->getFunctionPrivate()->script() == script);
+    JS_ASSERT_IF(scriptObject && scriptObject->isScript(), scriptObject->getScript() == script);
+
     BreakpointSiteMap::AddPtr p = breakpointSites.lookupForAdd(pc);
     if (!p) {
         BreakpointSite *site = cx->runtime->new_<BreakpointSite>(script, pc);
         if (!site || !breakpointSites.add(p, pc, site)) {
             js_ReportOutOfMemory(cx);
             return NULL;
         }
     }
 
     BreakpointSite *site = p->value;
-    JS_ASSERT_IF(scriptObject, scriptObject->isScript() || scriptObject->isFunction());
-    JS_ASSERT_IF(scriptObject && scriptObject->isFunction(),
-                 scriptObject->getFunctionPrivate()->script() == script);
-    JS_ASSERT_IF(scriptObject && scriptObject->isScript(), scriptObject->getScript() == script);
     if (site->scriptObject)
         JS_ASSERT_IF(scriptObject, site->scriptObject == scriptObject);
     else
         site->scriptObject = scriptObject;
 
     return site;
 }
 
@@ -777,19 +778,19 @@ void
 JSCompartment::clearBreakpointsIn(JSContext *cx, js::Debugger *dbg, JSScript *script,
                                   JSObject *handler)
 {
     JS_ASSERT_IF(script, script->compartment == this);
 
     for (BreakpointSiteMap::Enum e(breakpointSites); !e.empty(); e.popFront()) {
         BreakpointSite *site = e.front().value;
         if (!script || site->script == script) {
-            Breakpoint *next;
-            for (Breakpoint *bp = site->firstBreakpoint(); bp; bp = next) {
-                next = bp->nextInSite();
+            Breakpoint *nextbp;
+            for (Breakpoint *bp = site->firstBreakpoint(); bp; bp = nextbp) {
+                nextbp = bp->nextInSite();
                 if ((!dbg || bp->debugger == dbg) && (!handler || bp->getHandler() == handler))
                     bp->destroy(cx, &e);
             }
         }
     }
 }
 
 void
@@ -816,17 +817,17 @@ JSCompartment::markBreakpointsIterativel
                 markedAny = true;
             MarkValue(trc, site->trapClosure, "trap closure");
         }
 
         // Mark jsdbg breakpoints. If either the debugger or the script is
         // collected, then the breakpoint is collected along with it. So do not
         // mark the handler in that case.
         //
-        // If scriptObject is nonnull, examine it to see if the script will be
+        // If scriptObject is non-null, examine it to see if the script will be
         // collected. If scriptObject is null, then site->script is an eval
         // script on the stack, so it is definitely live.
         //
         if (!site->scriptObject || site->scriptObject->isMarked()) {
             for (Breakpoint *bp = site->firstBreakpoint(); bp; bp = bp->nextInSite()) {
                 if (bp->debugger->toJSObject()->isMarked() &&
                     bp->handler &&
                     !bp->handler->isMarked())
@@ -841,18 +842,25 @@ JSCompartment::markBreakpointsIterativel
 }
 
 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);
         }
     }
 }
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -483,17 +483,19 @@ struct JS_FRIEND_API(JSCompartment) {
      * dictionary mode). But because all the initial properties are
      * non-configurable, they will always map to fixed slots.
      */
     const js::Shape              *initialRegExpShape;
     const js::Shape              *initialStringShape;
 
   private:
     enum { DebugFromC = 1, DebugFromJS = 2 };
+
     uintN                        debugModeBits;  // see debugMode() below
+
   public:
     JSCList                      scripts;        // scripts in this compartment
 
     js::NativeIterCache          nativeIterCache;
 
     typedef js::Maybe<js::ToSourceCache> LazyToSourceCache;
     LazyToSourceCache            toSourceCache;
 
@@ -574,17 +576,17 @@ struct JS_FRIEND_API(JSCompartment) {
     size_t backEdgeCount(jsbytecode *pc) const;
     size_t incBackEdgeCount(jsbytecode *pc);
 
     /*
      * There are dueling APIs for debug mode. It can be enabled or disabled via
      * JS_SetDebugModeForCompartment. It is automatically enabled and disabled
      * by Debugger objects. Therefore debugModeBits has the DebugFromC bit set
      * if the C API wants debug mode and the DebugFromJS bit set if debuggees
-     * is nonempty.
+     * is non-empty.
      */
     bool debugMode() const { return !!debugModeBits; }
 
     /*
      * True if any scripts from this compartment are on the JS stack in the
      * calling thread. cx is a context in the calling thread, and it is assumed
      * that no other thread is using this compartment.
      */
--- a/js/src/jsdbg.cpp
+++ b/js/src/jsdbg.cpp
@@ -99,20 +99,20 @@ ReportMoreArgsNeeded(JSContext *cx, cons
     char s[2];
     s[0] = '0' + (required - 1);
     s[1] = '\0';
     JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                          name, s, required == 1 ? "" : "s");
     return false;
 }
 
-#define REQUIRE_ARGC(name, n) \
-    JS_BEGIN_MACRO \
-        if (argc < (n)) \
-            return ReportMoreArgsNeeded(cx, name, n); \
+#define REQUIRE_ARGC(name, n)                                                 \
+    JS_BEGIN_MACRO                                                            \
+        if (argc < (n))                                                       \
+            return ReportMoreArgsNeeded(cx, name, n);                         \
     JS_END_MACRO
 
 bool
 ReportObjectRequired(JSContext *cx)
 {
     JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
     return false;
 }
@@ -150,18 +150,18 @@ CheckThisClass(JSContext *cx, Value *vp,
     if (!thisobj)                                                            \
         return false;                                                        \
     js::classname *private = (classname *) thisobj->getPrivate();
 
 
 /*** Breakpoints *********************************************************************************/
 
 BreakpointSite::BreakpointSite(JSScript *script, jsbytecode *pc)
-    : script(script), pc(pc), realOpcode(JSOp(*pc)), scriptObject(NULL), enabledCount(0),
-      trapHandler(NULL), trapClosure(UndefinedValue())
+  : script(script), pc(pc), realOpcode(JSOp(*pc)), scriptObject(NULL), enabledCount(0),
+    trapHandler(NULL), trapClosure(UndefinedValue())
 {
     JS_ASSERT(realOpcode != JSOP_TRAP);
     JS_INIT_CLIST(&breakpoints);
 }
 
 /*
  * Precondition: script is live, meaning either it is a non-held script that is
  * on the stack or a held script that hasn't been GC'd.
--- a/js/src/jsdbg.h
+++ b/js/src/jsdbg.h
@@ -371,16 +371,17 @@ class BreakpointSite {
     Value trapClosure;
 
     bool recompile(JSContext *cx, bool forTrap);
 
   public:
     BreakpointSite(JSScript *script, jsbytecode *pc);
     Breakpoint *firstBreakpoint() const;
     bool hasBreakpoint(Breakpoint *bp);
+    bool hasTrap() const { return !!trapHandler; }
 
     bool inc(JSContext *cx);
     void dec(JSContext *cx);
     bool setTrap(JSContext *cx, JSTrapHandler handler, const Value &closure);
     void clearTrap(JSContext *cx, BreakpointSiteMap::Enum *e = NULL,
                    JSTrapHandler *handlerp = NULL, Value *closurep = NULL);
     void destroyIfEmpty(JSRuntime *rt, BreakpointSiteMap::Enum *e);
 };
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -14104,17 +14104,17 @@ MethodReadBarrier(JSContext* cx, JSObjec
     return &tvr.value().toObject();
 }
 JS_DEFINE_CALLINFO_4(static, OBJECT_FAIL, MethodReadBarrier, CONTEXT, OBJECT, SHAPE, OBJECT,
                      0, ACCSET_STORE_ANY)
 
 /*
  * Get a property. The current opcode has JOF_ATOM.
  *
- * There are two modes. The caller must pass nonnull pointers for either outp
+ * There are two modes. The caller must pass non-null pointers for either outp
  * or both slotp and v_insp. In the latter case, we require a plain old
  * property with a slot; if the property turns out to be anything else, abort
  * tracing (rather than emit a call to a native getter or GetAnyProperty).
  */
 JS_REQUIRES_STACK AbortableRecordingStatus
 TraceRecorder::prop(JSObject* obj, LIns* obj_ins, uint32 *slotp, LIns** v_insp, Value *outp)
 {
     /*