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 76885 e4b0baf5db728876dc26c44034b4ab7fc55e764a
parent 76884 df2aebf58b689b9b820587aacb57a712c86f7f93
child 76886 9f28a8fec3cbd789ea67237e5780f382077967f1
push id78
push userclegnitto@mozilla.com
push dateFri, 16 Dec 2011 17:32:24 +0000
treeherdermozilla-release@79d24e644fdd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs676937
milestone9.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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,