Bug 676937 - Make entering a compartment and pushing a dummy frame an atomic stack operation (r=mrbkap)
authorLuke Wagner <luke@mozilla.com>
Fri, 05 Aug 2011 14:06:33 -0700
changeset 75574 e4b0baf5db728876dc26c44034b4ab7fc55e764a
parent 75573 df2aebf58b689b9b820587aacb57a712c86f7f93
child 75575 9f28a8fec3cbd789ea67237e5780f382077967f1
push id3
push userfelipc@gmail.com
push dateFri, 30 Sep 2011 20:09:13 +0000
reviewersmrbkap
bugs676937
milestone9.0a1
Bug 676937 - Make entering a compartment and pushing a dummy frame an atomic stack operation (r=mrbkap)
js/src/jswrapper.cpp
js/src/vm/Stack-inl.h
js/src/vm/Stack.cpp
js/src/vm/Stack.h
--- a/js/src/jswrapper.cpp
+++ b/js/src/jswrapper.cpp
@@ -404,21 +404,22 @@ bool
 ForceFrame::enter()
 {
     frame = context->new_<DummyFrameGuard>();
     if (!frame)
        return false;
     LeaveTrace(context);
 
     JS_ASSERT(context->compartment == target->compartment());
+    JSCompartment *destination = context->compartment;
 
     JSObject *scopeChain = target->getGlobal();
     JS_ASSERT(scopeChain->isNative());
 
-    return context->stack.pushDummyFrame(context, REPORT_ERROR, *scopeChain, frame);
+    return context->stack.pushDummyFrame(context, destination, *scopeChain, frame);
 }
 
 AutoCompartment::AutoCompartment(JSContext *cx, JSObject *target)
     : context(cx),
       origin(cx->compartment),
       target(target),
       destination(target->getCompartment()),
       entered(false)
@@ -437,31 +438,18 @@ AutoCompartment::enter()
     JS_ASSERT(!entered);
     if (origin != destination) {
         LeaveTrace(context);
 
         JSObject *scopeChain = target->getGlobal();
         JS_ASSERT(scopeChain->isNative());
 
         frame.construct();
-
-        /*
-         * Set the compartment eagerly so that pushDummyFrame associates the
-         * resource allocation request with 'destination' instead of 'origin'.
-         * (This is important when content has overflowed the stack and chrome
-         * is preparing to run JS to throw up a slow script dialog.) However,
-         * if an exception is thrown, we need it to be in origin's compartment
-         * so be careful to only report after restoring.
-         */
-        context->compartment = destination;
-        if (!context->stack.pushDummyFrame(context, DONT_REPORT_ERROR, *scopeChain, &frame.ref())) {
-            context->compartment = origin;
-            js_ReportOverRecursed(context);
+        if (!context->stack.pushDummyFrame(context, destination, *scopeChain, &frame.ref()))
             return false;
-        }
 
         if (context->isExceptionPending())
             context->wrapPendingException();
     }
     entered = true;
     return true;
 }
 
--- a/js/src/vm/Stack-inl.h
+++ b/js/src/vm/Stack-inl.h
@@ -383,25 +383,26 @@ StackSpace::ensureEnoughSpaceToEnterTrac
     ptrdiff_t needed = TraceNativeStorage::MAX_NATIVE_STACK_SLOTS +
                        TraceNativeStorage::MAX_CALL_STACK_ENTRIES * VALUES_PER_STACK_FRAME;
     return ensureSpace(cx, DONT_REPORT_ERROR, firstUnused(), needed);
 }
 #endif
 
 STATIC_POSTCONDITION(!return || ubound(from) >= nvals)
 JS_ALWAYS_INLINE bool
-StackSpace::ensureSpace(JSContext *cx, MaybeReportError report, Value *from, ptrdiff_t nvals) const
+StackSpace::ensureSpace(JSContext *cx, MaybeReportError report, Value *from, ptrdiff_t nvals,
+                        JSCompartment *dest) const
 {
     assertInvariants();
     JS_ASSERT(from >= firstUnused());
 #ifdef XP_WIN
     JS_ASSERT(from <= commitEnd_);
 #endif
     if (JS_UNLIKELY(conservativeEnd_ - from < nvals))
-        return ensureSpaceSlow(cx, report, from, nvals);
+        return ensureSpaceSlow(cx, report, from, nvals, dest);
     return true;
 }
 
 inline Value *
 StackSpace::getStackLimit(JSContext *cx, MaybeReportError report)
 {
     FrameRegs &regs = cx->regs();
     uintN nvals = regs.fp()->numSlots() + VALUES_PER_STACK_FRAME;
--- a/js/src/vm/Stack.cpp
+++ b/js/src/vm/Stack.cpp
@@ -409,23 +409,26 @@ StackSpace::mark(JSTracer *trc)
             slotsEnd = (Value *)fp;
         }
         MarkStackRangeConservatively(trc, seg->slotsBegin(), slotsEnd);
         nextSegEnd = (Value *)seg;
     }
 }
 
 JS_FRIEND_API(bool)
-StackSpace::ensureSpaceSlow(JSContext *cx, MaybeReportError report,
-                            Value *from, ptrdiff_t nvals) const
+StackSpace::ensureSpaceSlow(JSContext *cx, MaybeReportError report, Value *from, ptrdiff_t nvals,
+                            JSCompartment *dest) const
 {
     assertInvariants();
 
-    bool trusted = !cx->compartment ||
-                   cx->compartment->principals == cx->runtime->trustedPrincipals();
+    /* See CX_COMPARTMENT comment. */
+    if (dest == (JSCompartment *)CX_COMPARTMENT)
+        dest = cx->compartment;
+
+    bool trusted = !dest || dest->principals == cx->runtime->trustedPrincipals();
     Value *end = trusted ? trustedEnd_ : defaultEnd_;
 
     /*
      * conservativeEnd_ must stay below defaultEnd_: if conservativeEnd_ were
      * to be bumped past defaultEnd_, untrusted JS would be able to consume the
      * buffer space at the end of the stack reserved for trusted JS.
      */
 
@@ -543,27 +546,27 @@ ContextStack::containsSlow(const StackFr
  * pushing a StackSegment. The 'pushedSeg' outparam indicates whether such a
  * segment was pushed (and hence whether the caller needs to call popSegment).
  *
  * Additionally, to minimize calls to ensureSpace, ensureOnTop ensures that
  * there is space for nvars slots on top of the stack.
  */
 Value *
 ContextStack::ensureOnTop(JSContext *cx, MaybeReportError report, uintN nvars,
-                          MaybeExtend extend, bool *pushedSeg)
+                          MaybeExtend extend, bool *pushedSeg, JSCompartment *dest)
 {
     Value *firstUnused = space().firstUnused();
 
     if (onTop() && extend) {
-        if (!space().ensureSpace(cx, report, firstUnused, nvars))
+        if (!space().ensureSpace(cx, report, firstUnused, nvars, dest))
             return NULL;
         return firstUnused;
     }
 
-    if (!space().ensureSpace(cx, report, firstUnused, VALUES_PER_STACK_SEGMENT + nvars))
+    if (!space().ensureSpace(cx, report, firstUnused, VALUES_PER_STACK_SEGMENT + nvars, dest))
         return NULL;
 
     FrameRegs *regs;
     CallArgsList *calls;
     if (seg_ && extend) {
         regs = seg_->maybeRegs();
         calls = seg_->maybeCalls();
     } else {
@@ -691,28 +694,30 @@ ContextStack::pushExecuteFrame(JSContext
 
     efg->prevRegs_ = seg_->pushRegs(efg->regs_);
     JS_ASSERT(space().firstUnused() == efg->regs_.sp);
     efg->setPushed(*this);
     return true;
 }
 
 bool
-ContextStack::pushDummyFrame(JSContext *cx, MaybeReportError report, JSObject &scopeChain,
-                             DummyFrameGuard *dfg)
+ContextStack::pushDummyFrame(JSContext *cx, JSCompartment *dest, JSObject &scopeChain, DummyFrameGuard *dfg)
 {
+    JS_ASSERT(dest == scopeChain.compartment());
+
     uintN nvars = VALUES_PER_STACK_FRAME;
-    Value *firstUnused = ensureOnTop(cx, report, nvars, CAN_EXTEND, &dfg->pushedSeg_);
+    Value *firstUnused = ensureOnTop(cx, REPORT_ERROR, nvars, CAN_EXTEND, &dfg->pushedSeg_, dest);
     if (!firstUnused)
         return NULL;
 
     StackFrame *fp = reinterpret_cast<StackFrame *>(firstUnused);
     fp->initDummyFrame(cx, scopeChain);
     dfg->regs_.initDummyFrame(*fp);
 
+    cx->compartment = dest;
     dfg->prevRegs_ = seg_->pushRegs(dfg->regs_);
     JS_ASSERT(space().firstUnused() == dfg->regs_.sp);
     dfg->setPushed(*this);
     return true;
 }
 
 void
 ContextStack::popFrame(const FrameGuard &fg)
@@ -785,34 +790,21 @@ ContextStack::popGeneratorFrame(const Ge
 
     /* ~FrameGuard/popFrame will finish the popping. */
     JS_ASSERT(ImplicitCast<const FrameGuard>(gfg).pushed());
 }
 
 bool
 ContextStack::saveFrameChain()
 {
-    /*
-     * The StackSpace uses the context's current compartment to determine
-     * whether to allow access to the privileged end-of-stack buffer.
-     * However, we always want saveFrameChain to have access to this privileged
-     * buffer since it gets used to prepare calling trusted JS. To force this,
-     * we clear the current compartment (which is interpreted by ensureSpace as
-     * 'trusted') and either restore it on OOM or let resetCompartment()
-     * clobber it.
-     */
-    JSCompartment *original = cx_->compartment;
-    cx_->compartment = NULL;
+    JSCompartment *dest = NULL;
 
     bool pushedSeg;
-    if (!ensureOnTop(cx_, DONT_REPORT_ERROR, 0, CANT_EXTEND, &pushedSeg)) {
-        cx_->compartment = original;
-        js_ReportOverRecursed(cx_);
+    if (!ensureOnTop(cx_, REPORT_ERROR, 0, CANT_EXTEND, &pushedSeg, dest))
         return false;
-    }
 
     JS_ASSERT(pushedSeg);
     JS_ASSERT(!hasfp());
     JS_ASSERT(onTop() && seg_->isEmpty());
 
     cx_->resetCompartment();
     return true;
 }
--- a/js/src/vm/Stack.h
+++ b/js/src/vm/Stack.h
@@ -39,16 +39,17 @@
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef Stack_h__
 #define Stack_h__
 
 #include "jsfun.h"
 
 struct JSContext;
+struct JSCompartment;
 
 namespace js {
 
 class StackFrame;
 class FrameRegs;
 class StackSegment;
 class StackSpace;
 class ContextStack;
@@ -1331,20 +1332,33 @@ class StackSpace
     static void staticAsserts() {
         JS_STATIC_ASSERT(CAPACITY_VALS % COMMIT_VALS == 0);
     }
 
     friend class AllFramesIter;
     friend class ContextStack;
     friend class StackFrame;
 
+    /*
+     * Except when changing compartment (see pushDummyFrame), the 'dest'
+     * parameter of ensureSpace is cx->compartment. Ideally, we'd just pass
+     * this directly (and introduce a helper that supplies cx->compartment when
+     * no 'dest' is given). For some compilers, this really hurts performance,
+     * so, instead, a trivially sinkable magic constant is used to indicate
+     * that dest should be cx->compartment.
+     */
+    static const size_t CX_COMPARTMENT = 0xc;
+
     inline bool ensureSpace(JSContext *cx, MaybeReportError report,
-                            Value *from, ptrdiff_t nvals) const;
+                            Value *from, ptrdiff_t nvals,
+                            JSCompartment *dest = (JSCompartment *)CX_COMPARTMENT) const;
     JS_FRIEND_API(bool) ensureSpaceSlow(JSContext *cx, MaybeReportError report,
-                                        Value *from, ptrdiff_t nvals) const;
+                                        Value *from, ptrdiff_t nvals,
+                                        JSCompartment *dest) const;
+
     StackSegment &findContainingSegment(const StackFrame *target) const;
 
   public:
     StackSpace();
     bool init();
     ~StackSpace();
 
     /*
@@ -1423,17 +1437,18 @@ class ContextStack
 #else
     void assertSpaceInSync() const {}
 #endif
 
     /* Implementation details of push* public interface. */
     StackSegment *pushSegment(JSContext *cx);
     enum MaybeExtend { CAN_EXTEND = true, CANT_EXTEND = false };
     Value *ensureOnTop(JSContext *cx, MaybeReportError report, uintN nvars,
-                       MaybeExtend extend, bool *pushedSeg);
+                       MaybeExtend extend, bool *pushedSeg,
+                       JSCompartment *dest = (JSCompartment *)StackSpace::CX_COMPARTMENT);
 
     inline StackFrame *
     getCallFrame(JSContext *cx, MaybeReportError report, const CallArgs &args,
                  JSFunction *fun, JSScript *script, StackFrame::Flags *pflags) const;
 
     /* Make pop* functions private since only called by guard classes. */
     void popSegment();
     friend class InvokeArgsGuard;
@@ -1504,19 +1519,26 @@ class ContextStack
     /*
      * Called by SendToGenerator to resume a yielded generator. In addition to
      * pushing a frame onto the VM stack, this function copies over the
      * floating frame stored in 'gen'. When 'gfg' is destroyed, the destructor
      * will copy the frame back to the floating frame.
      */
     bool pushGeneratorFrame(JSContext *cx, JSGenerator *gen, GeneratorFrameGuard *gfg);
 
-    /* Pushes a "dummy" frame; should be removed one day. */
-    bool pushDummyFrame(JSContext *cx, MaybeReportError report, JSObject &scopeChain,
-                        DummyFrameGuard *dfg);
+    /*
+     * When changing the compartment of a cx, it is necessary to immediately
+     * change the scope chain to a global in the right compartment since any
+     * amount of general VM code can run before the first scripted frame is
+     * pushed (if at all). This is currently and hackily accomplished by
+     * pushing a "dummy frame" with the correct scope chain. On success, this
+     * function will change the compartment to 'scopeChain.compartment()' and
+     * push a dummy frame for 'scopeChain'. On failure, nothing is changed.
+     */
+    bool pushDummyFrame(JSContext *cx, JSCompartment *dest, JSObject &scopeChain, DummyFrameGuard *dfg);
 
     /*
      * An "inline frame" may only be pushed from within the top, active
      * segment. This is the case for calls made inside mjit code and Interpret.
      * The 'stackLimit' overload updates 'stackLimit' if it changes.
      */
     bool pushInlineFrame(JSContext *cx, FrameRegs &regs, const CallArgs &args,
                          JSObject &callee, JSFunction *fun, JSScript *script,