Bug 1516514: Clear the hasCachedSavedFrame bit on frames on compartment mismatch. r=jorendorff
authorJim Blandy <jimb@mozilla.com>
Thu, 17 Jan 2019 21:09:04 +0000
changeset 511446 a3092a304863b66c609e77e8b4aefb271b527eb8
parent 511445 e5d2509fe3b22c0da2a0adf8b86e6a58a66140c2
child 511447 a0be6d91b30235fd5ad25b7133df9ffdb9cd05f5
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1516514
milestone66.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 1516514: Clear the hasCachedSavedFrame bit on frames on compartment mismatch. r=jorendorff The code that manages the LiveSavedFrameCache would very much like to assert that, if a frame has its hasCachedSavedFrame bit set, then it actually does have an entry in the LiveSavedFrameCache. However, in the presence of compartment mismatches, this becomes temporarily untrue, and OOMs can make 'temporarily' longer than expected. This patch more aggressively clears frames' hasCachedSavedFrame bits, so that when we do purge the cache for a compartment mismatch, all frames get their bits cleared before we start repopulating the cache. Differential Revision: https://phabricator.services.mozilla.com/D16661
js/src/jit-test/tests/saved-stacks/oom-in-save-stack-02.js
js/src/vm/SavedStacks.cpp
js/src/vm/Stack.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/saved-stacks/oom-in-save-stack-02.js
@@ -0,0 +1,28 @@
+// |jit-test| --no-ion; --no-baseline; skip-if: !('oomAtAllocation' in this)
+// This shouldn't assert (bug 1516514).
+//
+// Disabled for ion and baseline because those introduce OOMs at some point that
+// we don't seem to be able to catch, and they're not relevant to the bug.
+
+let g = newGlobal();
+
+function oomTest() {
+  let done = false;
+  for (let j = 1; !done; j++) {
+    saveStack();
+
+    oomAtAllocation(j);
+
+    try {
+      g.saveStack();
+    } catch {}
+
+    done = !resetOOMFailure();
+
+    try {
+      g.saveStack();
+    } catch {}
+  }
+}
+
+oomTest();
--- a/js/src/vm/SavedStacks.cpp
+++ b/js/src/vm/SavedStacks.cpp
@@ -94,16 +94,21 @@ bool LiveSavedFrameCache::insert(JSConte
 }
 
 void LiveSavedFrameCache::find(JSContext* cx, FramePtr& framePtr,
                                const jsbytecode* pc,
                                MutableHandleSavedFrame frame) const {
   MOZ_ASSERT(initialized());
   MOZ_ASSERT(framePtr.hasCachedSavedFrame());
 
+  // The assertions here check that either 1) frames' hasCachedSavedFrame flags
+  // accurately indicate the presence of a cache entry for that frame (ignoring
+  // pc mismatches), or 2) the cache is completely empty, having been flushed
+  // for a realm mismatch.
+
   // If we flushed the cache due to a realm mismatch, then we shouldn't
   // expect to find any frames in the cache.
   if (frames->empty()) {
     frame.set(nullptr);
     return;
   }
 
   // All our SavedFrames should be in the same realm. If the last
@@ -120,44 +125,33 @@ void LiveSavedFrameCache::find(JSContext
     frame.set(nullptr);
     return;
   }
 
   Key key(framePtr);
   while (key != frames->back().key) {
     MOZ_ASSERT(frames->back().savedFrame->realm() == cx->realm());
 
-    // We know that the cache does contain an entry for frameIter's frame,
-    // since its bit is set. That entry must be below this one in the stack,
-    // so frames->back() must correspond to a frame younger than
-    // frameIter's. If frameIter is the youngest frame with its bit set,
-    // then its entry is the youngest that is valid, and we can pop this
-    // entry. Even if frameIter is not the youngest frame with its bit set,
-    // since we're going to push new cache entries for all frames younger
-    // than frameIter, we must pop it anyway.
+    // framePtr must have an entry, but apparently it's below this one on the
+    // stack; frames->back() must correspond to a frame younger than framePtr's.
+    // SavedStacks::insertFrames is going to push new cache entries for
+    // everything younger than framePtr, so this entry should be popped.
     frames->popBack();
 
-    // If the frame's bit was set, the frame should always have an entry in
-    // the cache. (If we purged the entire cache because its SavedFrames had
-    // been captured for a different compartment, then we would have
-    // returned early above.)
+    // Since the cache does contain an entry for framePtr's frame somewhere,
+    // popping this younger frame had better not empty the cache.
     MOZ_ALWAYS_TRUE(!frames->empty());
   }
 
   // The youngest valid frame may have run some code, so its current pc may
   // not match its cache entry's pc. In this case, just treat it as a miss. No
   // older frame has executed any code; it would have been necessary to pop
   // this frame for that to happen, but this frame's bit is set.
   if (pc != frames->back().pc) {
     frames->popBack();
-
-    // Since we've removed this entry from the cache, clear the bit on its
-    // frame. Usually we'll repopulate the cache anyway, but if there's an
-    // OOM that might not happen.
-    framePtr.clearHasCachedSavedFrame();
     frame.set(nullptr);
     return;
   }
 
   frame.set(frames->back().savedFrame);
 }
 
 void LiveSavedFrameCache::findWithoutInvalidation(
@@ -1317,27 +1311,29 @@ bool SavedStacks::insertFrames(JSContext
     if (capture.is<JS::AllFrames>() && framePtr &&
         framePtr->hasCachedSavedFrame()) {
       auto* cache = activation.getLiveSavedFrameCache(cx);
       if (!cache) {
         return false;
       }
       cache->find(cx, *framePtr, iter.pc(), &parent);
 
-      // Even though iter.hasCachedSavedFrame() was true, we can't
-      // necessarily stop walking the stack here. We can get cache misses
-      // for two reasons:
-      // 1) This is the youngest valid frame in the cache, and it has run
-      //    code and advanced to a new pc since it was cached.
-      // 2) The cache was populated with SavedFrames captured for a
-      //    different compartment, and got purged completely. We will
-      //    repopulate it from scratch.
+      // Even though iter.hasCachedSavedFrame() was true, we may still get a
+      // cache miss, if the frame's pc doesn't match the cache entry's, or if
+      // the cache was emptied due to a realm mismatch.
       if (parent) {
         break;
       }
+
+      // This frame doesn't have a cache entry, despite its hasCachedSavedFrame
+      // flag being set. If this was due to a pc mismatch, we can clear the flag
+      // here and set things right. If the cache was emptied due to a realm mismatch,
+      // we should clear all the frames' flags as we walk to the bottom of the stack,
+      // so that they are all clear before we start pushing any new entries.
+      framePtr->clearHasCachedSavedFrame();
     }
 
     // We'll be pushing this frame onto stackChain. Gather the information
     // needed to construct the SavedFrame::Lookup.
     Rooted<LocationValue> location(cx);
     {
       AutoRealmUnchecked ar(cx, iter.realm());
       if (!cx->realm()->savedStacks().getLocation(cx, iter, &location)) {
--- a/js/src/vm/Stack.h
+++ b/js/src/vm/Stack.h
@@ -1143,19 +1143,20 @@ namespace js {
 //
 // - When we find a cache entry whose frame address matches our frame F, we know
 //   that F has never left the stack, but it may certainly be the case that
 //   execution took place in that frame, and that the current source position
 //   within F's function has changed. This means that the entry's SavedFrame,
 //   which records the source line and column as well as the function, is not
 //   correct. To detect this case, when we push a cache entry, we record the
 //   frame's pc. When consulting the cache, if a frame's address matches but its
-//   pc does not, then we pop the cache entry and continue walking the stack.
-//   The next stack frame will definitely hit: since its callee frame never left
-//   the stack, the calling frame never got the chance to execute.
+//   pc does not, then we pop the cache entry, clear the frame's bit, and
+//   continue walking the stack. The next stack frame will definitely hit: since
+//   its callee frame never left the stack, the calling frame never got the
+//   chance to execute.
 //
 // - Generators, at least conceptually, have long-lived stack frames that
 //   disappear from the stack when the generator yields, and reappear on the
 //   stack when the generator's 'next' method is called. When a generator's
 //   frame is placed again atop the stack, its bit must be cleared - for the
 //   purposes of the cache, treating the frame as a new frame - to respect the
 //   invariants we used to justify the algorithm above. Async function
 //   activations usually appear atop empty stacks, since they are invoked as a
@@ -1173,24 +1174,30 @@ namespace js {
 // - We actually break up the cache into one cache per Activation. Popping an
 //   activation invalidates all its cache entries, simply by freeing the cache
 //   altogether.
 //
 // - The entire chain of SavedFrames for a given stack capture is created in the
 //   compartment of the code that requested the capture, *not* in that of the
 //   frames it represents, so in general, different compartments may have
 //   different SavedFrame objects representing the same actual stack frame. The
-//   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 is 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.
+//   LiveSavedFrameCache simply records whichever SavedFrames were used in the
+//   most recent captures. When we find a cache hit, we check the entry's
+//   SavedFrame's compartment against the current compartment; if they do not
+//   match, we clear the entire cache.
+//
+//   This means that it is not always true that, if a frame's
+//   hasCachedSavedFrame bit is set, it must have an entry in the cache. The
+//   actual invariant is: either the cache is completely empty, or the frames'
+//   bits are trustworthy. This invariant holds even though capture can be
+//   interrupted at many places by OOM failures. Clearing the cache is a single,
+//   uninterruptible step. When we try to look up a frame whose bit is set and
+//   find an empty cache, we clear the frame's bit. And we only add the first
+//   frame to an empty cache once we've walked the stack all the way, so we know
+//   that all frames' bits are cleared by that point.
 //
 // - 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