Bug 810169 - Extra marking for JSContext::global() (r=luke)

--HG--
extra : rebase_source : 573ec5d6efb8ba5e5b4ecb0c459ab6160543d1bc

js/src/jscntxt.h

@@ -1393,33 +1393,8 @@ struct JSContext : js::ContextFriendFields,
         return enterCompartmentDepth_ > 0;
     }
 
-    void enterCompartment(JSCompartment *c) {
-        enterCompartmentDepth_++;
-        compartment = c;
-        if (throwing)
-            wrapPendingException();
-    }
-
-    inline void leaveCompartment(JSCompartment *oldCompartment) {
-        JS_ASSERT(hasEnteredCompartment());
-        enterCompartmentDepth_--;
-
-        /*
-         * Before we entered the current compartment, 'compartment' was
-         * 'oldCompartment', so we might want to simply set it back. However, we
-         * currently have this terrible scheme whereby defaultCompartmentObject_
-         * can be updated while enterCompartmentDepth_ > 0. In this case,
-         * oldCompartment != defaultCompartmentObject_->compartment and we must
-         * ignore oldCompartment.
-         */
-        if (hasEnteredCompartment() || !defaultCompartmentObject_)
-            compartment = oldCompartment;
-        else
-            compartment = defaultCompartmentObject_->compartment();
-
-        if (throwing)
-            wrapPendingException();
-    }
+    inline void enterCompartment(JSCompartment *c);
+    inline void leaveCompartment(JSCompartment *oldCompartment);
 
     /* See JS_SaveFrameChain/JS_RestoreFrameChain. */
   private:
@@ -1450,7 +1425,10 @@ struct JSContext : js::ContextFriendFields,
     /* Current execution stack. */
     js::ContextStack    stack;
 
-    /* Current global. */
+    /*
+     * Current global. This is only safe to use within the scope of the
+     * AutoCompartment from which it's called.
+     */
     inline js::Handle<js::GlobalObject*> global() const;
 
     /* ContextStack convenience functions */

js/src/jscntxtinlines.h

@@ -583,4 +583,39 @@ JSContext::setDefaultCompartmentObjectIfUnset(JSObject *obj)
         setDefaultCompartmentObject(obj);
 }
 
+inline void
+JSContext::enterCompartment(JSCompartment *c)
+{
+    enterCompartmentDepth_++;
+    compartment = c;
+    c->enter();
+    if (throwing)
+        wrapPendingException();
+}
+
+inline void
+JSContext::leaveCompartment(JSCompartment *oldCompartment)
+{
+    JS_ASSERT(hasEnteredCompartment());
+    enterCompartmentDepth_--;
+
+    compartment->leave();
+
+    /*
+     * Before we entered the current compartment, 'compartment' was
+     * 'oldCompartment', so we might want to simply set it back. However, we
+     * currently have this terrible scheme whereby defaultCompartmentObject_ can
+     * be updated while enterCompartmentDepth_ > 0. In this case, oldCompartment
+     * != defaultCompartmentObject_->compartment and we must ignore
+     * oldCompartment.
+     */
+    if (hasEnteredCompartment() || !defaultCompartmentObject_)
+        compartment = oldCompartment;
+    else
+        compartment = defaultCompartmentObject_->compartment();
+
+    if (throwing)
+        wrapPendingException();
+}
+
 #endif /* jscntxtinlines_h___ */

js/src/jscompartment.cpp

@@ -47,6 +47,7 @@ JSCompartment::JSCompartment(JSRuntime *rt)
   : rt(rt),
     principals(NULL),
     global_(NULL),
+    enterCompartmentDepth(0),
 #ifdef JSGC_GENERATIONAL
     gcStoreBuffer(&gcNursery),
 #endif
@@ -526,6 +527,13 @@ JSCompartment::mark(JSTracer *trc)
     if (ionCompartment_)
         ionCompartment_->mark(trc, this);
 #endif
+
+    /*
+     * If a compartment is on-stack, we mark its global so that
+     * JSContext::global() remains valid.
+     */
+    if (enterCompartmentDepth && global_)
+        MarkObjectRoot(trc, global_.unsafeGet(), "on-stack compartment global");
 }
 
 void
@@ -631,7 +639,7 @@ JSCompartment::sweep(FreeOp *fop, bool releaseTypes)
         sweepNewTypeObjectTable(lazyTypeObjects);
         sweepBreakpoints(fop);
 
-        if (global_ && IsObjectAboutToBeFinalized(&global_))
+        if (global_ && IsObjectAboutToBeFinalized(global_.unsafeGet()))
             global_ = NULL;
 
 #ifdef JS_ION

js/src/jscompartment.h

@@ -126,22 +126,26 @@ struct JSCompartment : public js::gc::GraphNodeBase<JSCompartment>
   private:
     friend struct JSRuntime;
     friend struct JSContext;
-    js::GlobalObject             *global_;
+    js::ReadBarriered<js::GlobalObject> global_;
+
+    unsigned                     enterCompartmentDepth;
+
   public:
-    // Nb: global_ might be NULL, if (a) it's the atoms compartment, or (b) the
-    // compartment's global has been collected.  The latter can happen if e.g.
-    // a string in a compartment is rooted but no object is, and thus the
-    // global isn't rooted, and thus the global can be finalized while the
-    // compartment lives on.
-    //
-    // In contrast, JSObject::global() is infallible because marking a JSObject
-    // always marks its global as well.
-    // TODO: add infallible JSScript::global()
-    //
-    js::GlobalObject *maybeGlobal() const {
-        JS_ASSERT_IF(global_, global_->compartment() == this);
-        return global_;
-    }
+    void enter() { enterCompartmentDepth++; }
+    void leave() { enterCompartmentDepth--; }
+
+    /*
+     * Nb: global_ might be NULL, if (a) it's the atoms compartment, or (b) the
+     * compartment's global has been collected.  The latter can happen if e.g.
+     * a string in a compartment is rooted but no object is, and thus the global
+     * isn't rooted, and thus the global can be finalized while the compartment
+     * lives on.
+     *
+     * In contrast, JSObject::global() is infallible because marking a JSObject
+     * always marks its global as well.
+     * TODO: add infallible JSScript::global()
+     */
+    inline js::GlobalObject *maybeGlobal() const;
 
     void initGlobal(js::GlobalObject &global) {
         JS_ASSERT(global.compartment() == this);
@@ -555,7 +559,13 @@ JSContext::typeInferenceEnabled() const
 inline js::Handle<js::GlobalObject*>
 JSContext::global() const
 {
-    return js::Handle<js::GlobalObject*>::fromMarkedLocation(&compartment->global_);
+    /*
+     * It's safe to use |unsafeGet()| here because any compartment that is
+     * on-stack will be marked automatically, so there's no need for a read
+     * barrier on it. Once the compartment is popped, the handle is no longer
+     * safe to use.
+     */
+    return js::Handle<js::GlobalObject*>::fromMarkedLocation(compartment->global_.unsafeGet());
 }
 
 namespace js {
@@ -582,16 +592,8 @@ class AutoCompartment
     JSCompartment * const origin_;
 
   public:
-    AutoCompartment(JSContext *cx, JSObject *target)
-      : cx_(cx),
-        origin_(cx->compartment)
-    {
-        cx_->enterCompartment(target->compartment());
-    }
-
-    ~AutoCompartment() {
-        cx_->leaveCompartment(origin_);
-    }
+    inline AutoCompartment(JSContext *cx, JSObject *target);
+    inline ~AutoCompartment();
 
     JSContext *context() const { return cx_; }
     JSCompartment *origin() const { return origin_; }

js/src/jscompartmentinlines.h

@@ -0,0 +1,30 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+ * vim: set ts=4 sw=4 et tw=79:
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef jscompartment_inlines_h___
+#define jscompartment_inlines_h___
+
+js::GlobalObject *
+JSCompartment::maybeGlobal() const
+{
+    JS_ASSERT_IF(global_, global_->compartment() == this);
+    return global_;
+}
+
+js::AutoCompartment::AutoCompartment(JSContext *cx, JSObject *target)
+  : cx_(cx),
+    origin_(cx->compartment)
+{
+    cx_->enterCompartment(target->compartment());
+}
+
+js::AutoCompartment::~AutoCompartment()
+{
+    cx_->leaveCompartment(origin_);
+}
+
+#endif /* jscompartment_inlines_h___ */

js/src/jsobjinlines.h

@@ -39,6 +39,7 @@
 #include "vm/StringObject.h"
 
 #include "jsatominlines.h"
+#include "jscompartmentinlines.h"
 #include "jsfuninlines.h"
 #include "jsgcinlines.h"
 #include "jsinferinlines.h"