Bug 1444604 - Part 7: Reconcile LiveSavedFrameCache with evalInFramePrev by tweaking SavedFrame parent links. r=jorendorff, a=RyanVM
authorJim Blandy <jimb@mozilla.com>
Wed, 14 Mar 2018 12:54:04 -0700
changeset 776181 c31d3d320dbd76ae231f097150a58f120725ed6e
parent 776180 68469fba24547ba7fd2c7692828a4609606e59c8
child 776182 bdcdf1e0285363c79f8b3cfdec10d903ca76b917
child 780089 d2564a0c25e5084dbd064cf546d98a9f72eed1f3
push id104821
push userbmo:rrosario@mozilla.com
push dateMon, 02 Apr 2018 18:45:53 +0000
reviewersjorendorff, RyanVM
bugs1444604
milestone60.0
Bug 1444604 - Part 7: Reconcile LiveSavedFrameCache with evalInFramePrev by tweaking SavedFrame parent links. r=jorendorff, a=RyanVM
js/src/jit-test/tests/debug/bug-1444604-reduced.js
js/src/jit-test/tests/debug/bug-1444604.js
js/src/vm/SavedStacks.cpp
js/src/vm/SavedStacks.h
js/src/vm/Stack.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/bug-1444604-reduced.js
@@ -0,0 +1,27 @@
+// LiveSavedFrameCache should not be confused by eval-in-frame-prev links.
+
+const g = newGlobal();
+const dbg = new Debugger();
+const gDO = dbg.addDebuggee(g);
+
+g.evaluate(`
+  function inner() { debugger; }
+  function outer() { inner(); }
+`);
+
+dbg.onDebuggerStatement = function handler(frame) {
+  // Capture the JS stack, putting frames for handler, inner, and outer in the
+  // cache. Perform all captures in g's compartment, to avoid flushing the cache
+  // for compartment mismatches.
+  frame.eval('print(`in inner: ${saveStack()}`)');
+
+  // Carry out a capture in outer's context, following the eval-in-frame prev
+  // link and thus skipping over inner, removing it from the cache.
+  frame.older.eval('print(`in outer: ${saveStack()}`)');
+
+  // Capture again. inner's hasCachedSavedFrame bit should be set, but its entry
+  // has been flushed.
+  frame.eval('print(`in inner: ${saveStack()}`)');
+};
+
+g.outer();
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/bug-1444604.js
@@ -0,0 +1,16 @@
+// Fuzz test: LiveSavedFrameCache should not be confused by eval-in-frame-prev links.
+// See bug-144604-reduced.js for a more direct version.
+
+var evalInFrame = (function (global) {
+  var dbgGlobal = newGlobal();
+  var dbg = new dbgGlobal.Debugger();
+  return function evalInFrame(upCount, code) {
+    dbg.addDebuggee(global);
+    var frame = dbg.getNewestFrame().older;
+    for (var i = 0; i < upCount; i++) {
+    }
+    var completion = frame.eval(code);
+  };
+})(this);
+enableTrackAllocations();
+evalInFrame(1, "print(a)");
--- a/js/src/vm/SavedStacks.cpp
+++ b/js/src/vm/SavedStacks.cpp
@@ -1361,17 +1361,34 @@ SavedStacks::insertFrames(JSContext* cx,
     // Accumulate the vector of Lookup objects here, youngest to oldest.
     SavedFrame::AutoLookupVector stackChain(cx);
 
     // If we find an async parent or a cached saved frame, then that supplies
     // the parent of the frames we have placed in stackChain. If we walk the
     // stack all the way to the end, this remains null.
     RootedSavedFrame parent(cx, nullptr);
 
-    FrameIter iter(cx);
+    // Choose the right frame iteration strategy to accomodate both
+    // evalInFramePrev links and the LiveSavedFrameCache. For background, see
+    // the LiveSavedFrameCache comments in Stack.h.
+    //
+    // If we're using the LiveSavedFrameCache, then don't handle evalInFramePrev
+    // links by skipping over the frames altogether; that violates the cache's
+    // assumptions. Instead, traverse the entire stack, but choose each
+    // SavedFrame's parent as directed by the evalInFramePrev link, if any.
+    //
+    // If we're not using the LiveSavedFrameCache, it's hard to recover the
+    // frame to which the evalInFramePrev link refers, so we just let FrameIter
+    // skip those frames. Then each SavedFrame's parent is simply the frame that
+    // follows it in the stackChain vector, even when it has an evalInFramePrev
+    // link.
+    FrameIter iter(cx,
+                   capture.is<JS::AllFrames>()
+                   ? FrameIter::IGNORE_DEBUGGER_EVAL_PREV_LINK
+                   : FrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK);
 
     // Once we've seen one frame with its hasCachedSavedFrame bit set, all its
     // parents (that can be cached) ought to have it set too.
     DebugOnly<bool> seenCached = false;
 
     while (!iter.done()) {
         Activation& activation = *iter.activation();
         Maybe<LiveSavedFrameCache::FramePtr> framePtr = LiveSavedFrameCache::FramePtr::create(iter);
@@ -1486,16 +1503,27 @@ SavedStacks::insertFrames(JSContext* cx,
     }
 
     // Iterate through |stackChain| in reverse order and get or create the
     // actual SavedFrame instances.
     frame.set(parent);
     for (size_t i = stackChain->length(); i != 0; i--) {
         SavedFrame::HandleLookup lookup = stackChain[i-1];
         lookup->parent = frame;
+
+        // If necessary, adjust the parent of a debugger eval frame to point to
+        // the frame in whose scope the eval occurs - if we're using
+        // LiveSavedFrameCache. Otherwise, we simply ask the FrameIter to follow
+        // evalInFramePrev links, so that the parent is always the last frame we
+        // created.
+        if (capture.is<JS::AllFrames>() && lookup->framePtr) {
+            if (!checkForEvalInFramePrev(cx, lookup))
+                return false;
+        }
+
         frame.set(getOrCreateSavedFrame(cx, lookup));
         if (!frame)
             return false;
 
         if (capture.is<JS::AllFrames>() && lookup->framePtr) {
             auto* cache = lookup->activation->getLiveSavedFrameCache(cx);
             if (!cache || !cache->insert(cx, *lookup->framePtr, lookup->pc, frame))
                 return false;
@@ -1565,16 +1593,69 @@ SavedStacks::adoptAsyncStack(JSContext* 
         if (!asyncStack)
             return false;
         stackChain->popBack();
     }
 
     return true;
 }
 
+// Given a |lookup| for which we're about to construct a SavedFrame, if it
+// refers to a Debugger eval frame, adjust |lookup|'s parent to be the frame's
+// evalInFramePrev target.
+//
+// Debugger eval frames run code in the scope of some random older frame on the
+// stack (the 'target' frame). It is our custom to report the target as the
+// immediate parent of the eval frame. The LiveSavedFrameCache requires us not
+// to skip frames, so instead we walk the entire stack, and just give Debugger
+// eval frames the right parents as we encounter them.
+//
+// Call this function only if we are using the LiveSavedFrameCache; otherwise,
+// FrameIter has already taken care of getting us the right parent.
+bool
+SavedStacks::checkForEvalInFramePrev(JSContext* cx, SavedFrame::HandleLookup lookup)
+{
+    MOZ_ASSERT(lookup->framePtr);
+    if (!lookup->framePtr->isInterpreterFrame())
+        return true;
+
+    InterpreterFrame& interpreterFrame = lookup->framePtr->asInterpreterFrame();
+    if (!interpreterFrame.isDebuggerEvalFrame())
+        return true;
+
+    LiveSavedFrameCache::FramePtr target =
+        LiveSavedFrameCache::FramePtr::create(interpreterFrame.evalInFramePrev());
+
+    // If we're caching the frame to which |lookup| refers, then we should
+    // definitely have the target frame in the cache as well.
+    MOZ_ASSERT(target.hasCachedSavedFrame());
+
+    // Search the chain of activations for a LiveSavedFrameCache that has an
+    // entry for target.
+    RootedSavedFrame saved(cx, nullptr);
+    for (Activation* act = lookup->activation; act; act = act->prev()) {
+        // It's okay to force allocation of a cache here; we're about to put
+        // something in the top cache, and all the lower ones should exist
+        // already.
+        auto* cache = act->getLiveSavedFrameCache(cx);
+        if (!cache)
+            return false;
+
+        cache->findWithoutInvalidation(target, &saved);
+        if (saved)
+            break;
+    }
+
+    // Since |target| has its cached bit set, we should have found it.
+    MOZ_ALWAYS_TRUE(saved);
+
+    lookup->parent = saved;
+    return true;
+}
+
 SavedFrame*
 SavedStacks::getOrCreateSavedFrame(JSContext* cx, SavedFrame::HandleLookup lookup)
 {
     const SavedFrame::Lookup& lookupInstance = lookup.get();
     DependentAddPtr<SavedFrame::Set> p(cx, frames, lookupInstance);
     if (p) {
         MOZ_ASSERT(*p);
         return *p;
--- a/js/src/vm/SavedStacks.h
+++ b/js/src/vm/SavedStacks.h
@@ -220,16 +220,17 @@ class SavedStacks {
         }
     };
 
     MOZ_MUST_USE bool insertFrames(JSContext* cx, MutableHandleSavedFrame frame,
                                    JS::StackCapture&& capture);
     MOZ_MUST_USE bool adoptAsyncStack(JSContext* cx, MutableHandleSavedFrame asyncStack,
                                       HandleAtom asyncCause,
                                       const Maybe<size_t>& maxFrameCount);
+    MOZ_MUST_USE bool checkForEvalInFramePrev(JSContext* cx, SavedFrame::HandleLookup lookup);
     SavedFrame* getOrCreateSavedFrame(JSContext* cx, SavedFrame::HandleLookup lookup);
     SavedFrame* createFrameFromLookup(JSContext* cx, SavedFrame::HandleLookup lookup);
 
     // Cache for memoizing PCToLineNumber lookups.
 
     struct PCKey {
         PCKey(JSScript* script, jsbytecode* pc) : script(script), pc(pc) { }
 
--- a/js/src/vm/Stack.h
+++ b/js/src/vm/Stack.h
@@ -1231,16 +1231,44 @@ struct DefaultHasher<AbstractFramePtr> {
 //   LiveSavedFrameCache simply records whichever SavedFrames were created most
 //   recently. When we find a cache hit, we check the entry's SavedFrame's
 //   compartment against the current compartment; if they do not match, we flush
 //   the entire cache. This means that it is not always true that, if a frame's
 //   bit it set, it must have an entry in the cache. But we can still assert
 //   that, if a frame's bit is set and the cache is not completely empty, the
 //   frame will have an entry. When the cache is flushed, it will be repopulated
 //   immediately with the new capture's frames.
+//
+// - When the Debugger API evaluates an expression in some frame (the 'target
+//   frame'), it's SpiderMonkey's convention that the target frame be treated as
+//   the parent of the eval frame. In reality, of course, the eval frame is
+//   pushed on the top of the stack like any other frame, but stack captures
+//   simply jump straight over the intervening frames, so that the '.parent'
+//   property of a SavedFrame for the eval is the SavedFrame for the target.
+//   This is arranged by giving the eval frame an 'evalInFramePrev` link
+//   pointing to the target, which an ordinary FrameIter will notice and
+//   respect.
+//
+//   If the LiveSavedFrameCache were presented with stack traversals that
+//   skipped frames in this way, it would cause havoc. First, with no debugger
+//   eval frames present, capture the stack, populating the cache. Then push a
+//   debugger eval frame and capture again; the skipped frames to appear to be
+//   absent from the stack. Now pop the debugger eval frame, and capture a third
+//   time: the no-longer-skipped frames seem to reappear on the stack, with
+//   their cached bits still set.
+//
+//   The LiveSavedFrameCache assumes that the stack it sees is used in a
+//   stack-like fashion: if a frame has its bit set, it has never left the
+//   stack. To support this assumption, when the cache is in use, we do not skip
+//   the frames between a debugger eval frame an its target; we always traverse
+//   the entire stack, invalidating and populating the cache in the usual way.
+//   Instead, when we construct a SavedFrame for a debugger eval frame, we
+//   select the appropriate parent at that point: rather than the next-older
+//   frame, we find the SavedFrame for the eval's target frame. The skip appears
+//   in the SavedFrame chains, even as the traversal covers all the frames.
 class LiveSavedFrameCache
 {
   public:
     // The address of a live frame for which we can cache SavedFrames: it has a
     // 'hasCachedSavedFrame' bit we can examine and set, and can be converted to
     // a Key to index the cache.
     class FramePtr {
         // We use jit::CommonFrameLayout for both Baseline frames and Ion