Bug 1443592: Part 2: Move hasCachedSavedFrame flag access from FrameIter to LiveSavedFrameCache::FramePtr. r=fitzgen

Instead of using FrameIter::hasCachedSavedFrame, the code should instead try to
construct a FramePtr via LiveSavedFrameCache::getFramePtr, which returns a
Maybe<FramePtr>. If that is Some, then we know the frame at hand actually has a
flag, and we can test and set it.

Since the existing FrameIter::hasCachedSavedFrame screens out all wasm frames,
we change LiveSavedFrameCache::getFramePtr to do the same. However, the existing
behavior was incorrect, since wasm::DebugFrame does carry the flag; we'll fix
that in a later patch.

FrameIter::hasCachedSavedFrame asserted that jsJitFrame().isIonScripted(); this
is tested by isPhysicalIonFrame(), so that case is already covered.

MozReview-Commit-ID: 92wCc71s4nU

--HG--
extra : rebase_source : 54f60fbf298037448d9c051df3419df2a80f17d2

js/src/vm/SavedStacks.cpp

@@ -57,6 +57,12 @@ const uint32_t ASYNC_STACK_MAX_FRAME_COUNT = 60;
 /* static */ Maybe<LiveSavedFrameCache::FramePtr>
 LiveSavedFrameCache::getFramePtr(const FrameIter& iter)
 {
+    if (iter.done())
+        return Nothing();
+
+    if (iter.isWasm())
+        return Nothing();
+
     if (iter.hasUsableAbstractFramePtr())
         return Some(FramePtr(iter.abstractFramePtr()));
 
@@ -83,6 +89,7 @@ bool
 LiveSavedFrameCache::insert(JSContext* cx, FramePtr& framePtr, const jsbytecode* pc,
                             HandleSavedFrame savedFrame)
 {
+    MOZ_ASSERT(savedFrame);
     MOZ_ASSERT(initialized());
 
     if (!frames->emplaceBack(framePtr, pc, savedFrame)) {
@@ -90,12 +97,7 @@ LiveSavedFrameCache::insert(JSContext* cx, FramePtr& framePtr, const jsbytecode*
         return false;
     }
 
-    // Safe to dereference the cache key because the stack frames are still
-    // live. After this point, they should never be dereferenced again.
-    if (framePtr.is<AbstractFramePtr>())
-        framePtr.as<AbstractFramePtr>().setHasCachedSavedFrame();
-    else
-        framePtr.as<jit::CommonFrameLayout*>()->setHasCachedSavedFrame();
+    framePtr.setHasCachedSavedFrame();
 
     return true;
 }
@@ -105,10 +107,10 @@ LiveSavedFrameCache::find(JSContext* cx, FrameIter& frameIter, MutableHandleSave
 {
     MOZ_ASSERT(initialized());
     MOZ_ASSERT(!frameIter.done());
-    MOZ_ASSERT(frameIter.hasCachedSavedFrame());
 
     Maybe<FramePtr> maybeFramePtr = getFramePtr(frameIter);
     MOZ_ASSERT(maybeFramePtr.isSome());
+    MOZ_ASSERT(maybeFramePtr->hasCachedSavedFrame());
 
     Key key(*maybeFramePtr);
     const jsbytecode* pc = frameIter.pc();
@@ -1321,6 +1323,7 @@ SavedStacks::insertFrames(JSContext* cx, FrameIter& iter, MutableHandleSavedFram
     RootedString asyncCause(cx, nullptr);
     bool parentIsInCache = false;
     RootedSavedFrame cachedFrame(cx, nullptr);
+    Maybe<LiveSavedFrameCache::FramePtr> framePtr = LiveSavedFrameCache::getFramePtr(iter);
 
     // Accumulate the vector of Lookup objects in |stackChain|.
     SavedFrame::AutoLookupVector stackChain(cx);
@@ -1377,12 +1380,11 @@ SavedStacks::insertFrames(JSContext* cx, FrameIter& iter, MutableHandleSavedFram
         // The bit set means that the next older parent (frame, pc) pair *must*
         // be in the cache.
         if (capture.is<JS::AllFrames>())
-            parentIsInCache = iter.hasCachedSavedFrame();
+            parentIsInCache = framePtr && framePtr->hasCachedSavedFrame();
 
         auto principals = iter.compartment()->principals();
         auto displayAtom = (iter.isWasm() || iter.isFunctionFrame()) ? iter.functionDisplayAtom() : nullptr;
 
-        Maybe<LiveSavedFrameCache::FramePtr> framePtr = LiveSavedFrameCache::getFramePtr(iter);
         MOZ_ASSERT_IF(framePtr && !iter.isWasm(), iter.pc());
 
         if (!stackChain->emplaceBack(location.source(),
@@ -1408,10 +1410,11 @@ SavedStacks::insertFrames(JSContext* cx, FrameIter& iter, MutableHandleSavedFram
         }
 
         ++iter;
+        framePtr = LiveSavedFrameCache::getFramePtr(iter);
 
         if (parentIsInCache &&
-            !iter.done() &&
-            iter.hasCachedSavedFrame())
+            framePtr &&
+            framePtr->hasCachedSavedFrame())
         {
             auto* cache = activation.getLiveSavedFrameCache(cx);
             if (!cache)

js/src/vm/Stack-inl.h

@@ -998,34 +998,24 @@ InterpreterActivation::resumeGeneratorFrame(HandleFunction callee, HandleValue n
     return true;
 }
 
+struct LiveSavedFrameCache::FramePtr::HasCachedMatcher {
+    bool match(AbstractFramePtr f) const { return f.hasCachedSavedFrame(); }
+    bool match(jit::CommonFrameLayout* f) const { return f->hasCachedSavedFrame(); }
+};
+
 inline bool
-FrameIter::hasCachedSavedFrame() const
-{
-    if (isWasm())
-        return false;
-
-    if (hasUsableAbstractFramePtr())
-        return abstractFramePtr().hasCachedSavedFrame();
-
-    MOZ_ASSERT(jsJitFrame().isIonScripted());
-    // SavedFrame caching is done at the physical frame granularity (rather than
-    // for each inlined frame) for ion. Therefore, it is impossible to have a
-    // cached SavedFrame if this frame is not a physical frame.
-    return isPhysicalIonFrame() && jsJitFrame().current()->hasCachedSavedFrame();
+LiveSavedFrameCache::FramePtr::hasCachedSavedFrame() const {
+    return ptr.match(HasCachedMatcher());
 }
 
+struct LiveSavedFrameCache::FramePtr::SetHasCachedMatcher {
+    void match(AbstractFramePtr f) const { f.setHasCachedSavedFrame(); }
+    void match(jit::CommonFrameLayout* f) const { f->setHasCachedSavedFrame(); }
+};
+
 inline void
-FrameIter::setHasCachedSavedFrame()
-{
-    MOZ_ASSERT(!isWasm());
-
-    if (hasUsableAbstractFramePtr()) {
-        abstractFramePtr().setHasCachedSavedFrame();
-        return;
-    }
-
-    MOZ_ASSERT(isPhysicalIonFrame());
-    jsJitFrame().current()->setHasCachedSavedFrame();
+LiveSavedFrameCache::FramePtr::setHasCachedSavedFrame() {
+    ptr.match(SetHasCachedMatcher());
 }
 
 } /* namespace js */

js/src/vm/Stack.h

@@ -1170,7 +1170,27 @@ struct DefaultHasher<AbstractFramePtr> {
 class LiveSavedFrameCache
 {
   public:
-    using FramePtr = mozilla::Variant<AbstractFramePtr, jit::CommonFrameLayout*>;
+    // 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 {
+        using Ptr = mozilla::Variant<AbstractFramePtr, jit::CommonFrameLayout*>;
+
+        Ptr ptr;
+
+        struct HasCachedMatcher;
+        struct SetHasCachedMatcher;
+
+      public:
+        explicit FramePtr(AbstractFramePtr ptr) : ptr(ptr) { }
+        explicit FramePtr(jit::CommonFrameLayout* ptr) : ptr(ptr) { }
+
+        inline bool hasCachedSavedFrame() const;
+        inline void setHasCachedSavedFrame();
+
+        bool operator==(const FramePtr& rhs) const { return rhs.ptr == this->ptr; }
+        bool operator!=(const FramePtr& rhs) const { return !(rhs == *this); }
+    };
 
   private:
     // A key in the cache: the address of a frame, live or dead, for which we
@@ -1958,10 +1978,6 @@ class FrameIter
     bool isFunctionFrame() const;
     bool hasArgs() const { return isFunctionFrame(); }
 
-    // These two methods may not be called with asm frames.
-    inline bool hasCachedSavedFrame() const;
-    inline void setHasCachedSavedFrame();
-
     ScriptSource* scriptSource() const;
     const char* filename() const;
     const char16_t* displayURL() const;