Bug 1062869 part 7 - Prevent miss-use of instruction result computation. r=h4writer

js/src/jit/BaselineBailouts.cpp

@@ -379,6 +379,33 @@ struct BaselineStackBuilder
     }
 };
 
+// Ensure that all value locations are readable from the SnapshotIterator.
+// Remove RInstructionResults from the JitActivation if the frame got recovered
+// ahead of the bailout.
+class SnapshotIteratorForBailout : public SnapshotIterator
+{
+    RInstructionResults results_;
+  public:
+
+    SnapshotIteratorForBailout(const IonBailoutIterator &iter)
+      : SnapshotIterator(iter),
+        results_()
+    {
+    }
+
+    // Take previously computed result out of the activation, or compute the
+    // results of all recover instructions contained in the snapshot.
+    bool init(JSContext *cx, JitActivation *activation) {
+        activation->maybeTakeIonFrameRecovery(fp_, &results_);
+        if (!results_.isInitialized() && !computeInstructionResults(cx, &results_))
+            return false;
+
+        MOZ_ASSERT(results_.isInitialized());
+        instructionResults_ = &results_;
+        return true;
+    }
+};
+
 static inline bool
 IsInlinableFallback(ICFallbackStub *icEntry)
 {
@@ -1343,11 +1370,8 @@ jit::BailoutIonToBaseline(JSContext *cx, JitActivation *activation, IonBailoutIt
         return BAILOUT_RETURN_FATAL_ERROR;
     JitSpew(JitSpew_BaselineBailouts, "  Incoming frame ptr = %p", builder.startFrame());
 
-    RInstructionResults instructionResults;
-    activation->maybeTakeIonFrameRecovery(iter.jsFrame(), &instructionResults);
-    SnapshotIterator snapIter(iter);
-
-    if (!snapIter.initInstructionResults(cx, &instructionResults))
+    SnapshotIteratorForBailout snapIter(iter);
+    if (!snapIter.init(cx, activation))
         return BAILOUT_RETURN_FATAL_ERROR;
 
 #ifdef TRACK_SNAPSHOTS

js/src/jit/IonFrames.cpp

@@ -1485,7 +1485,7 @@ bool
 RInstructionResults::isInitialized() const
 {
     MOZ_ASSERT_IF(results_, fp_);
-    return results_;
+    return fp_;
 }
 
 IonJSFrameLayout *
@@ -1780,7 +1780,6 @@ SnapshotIterator::initInstructionResults(MaybeReadFallback &fallback)
 
     // If there is only one resume point in the list of instructions, then there
     // is no instruction to recover, and thus no need to register any results.
-    MOZ_ASSERT(recover_.numInstructionsRead() == 1);
     if (recover_.numInstructions() == 1)
         return true;
 
@@ -1795,20 +1794,28 @@ SnapshotIterator::initInstructionResults(MaybeReadFallback &fallback)
         if (!ionScript_->invalidate(cx, /* resetUses = */ false, "Observe recovered instruction."))
             return false;
 
-        // Start a new snapshot at the beginning of the JitFrameIterator.  This
-        // SnapshotIterator is used for evaluating the content of all recover
-        // instructions.  The result is then saved on the JitActivation.
-        SnapshotIterator s(*fallback.frame);
+        // Register the list of result on the activation.  We need to do that
+        // before we initialize the list such as if any recover instruction
+        // cause a GC, we can ensure that the results are properly traced by the
+        // activation.
         RInstructionResults tmp;
-        if (!s.initInstructionResults(cx, &tmp))
-            return false;
-
-        // Register the list of result on the activation.
         if (!fallback.activation->registerIonFrameRecovery(fallback.frame->jsFrame(),
                                                            mozilla::Move(tmp)))
             return false;
 
         results = fallback.activation->maybeIonFrameRecovery(fp);
+
+        // Start a new snapshot at the beginning of the JitFrameIterator.  This
+        // SnapshotIterator is used for evaluating the content of all recover
+        // instructions.  The result is then saved on the JitActivation.
+        SnapshotIterator s(*fallback.frame);
+        if (!s.computeInstructionResults(cx, results)) {
+
+            // If the evaluation failed because of OOMs, then we discard the
+            // current set of result that we collected so far.
+            fallback.activation->maybeTakeIonFrameRecovery(fp, &tmp);
+            return false;
+        }
     }
 
     MOZ_ASSERT(results->isInitialized());
@@ -1817,24 +1824,26 @@ SnapshotIterator::initInstructionResults(MaybeReadFallback &fallback)
 }
 
 bool
-SnapshotIterator::initInstructionResults(JSContext *cx, RInstructionResults *results)
+SnapshotIterator::computeInstructionResults(JSContext *cx, RInstructionResults *results) const
 {
+    MOZ_ASSERT(!results->isInitialized());
     MOZ_ASSERT(recover_.numInstructionsRead() == 1);
 
-    // The last instruction will always be a resume point, no need to allocate
-    // space for it.
-    if (recover_.numInstructions() == 1)
-        return true;
-
-    MOZ_ASSERT(recover_.numInstructions() > 1);
+    // The last instruction will always be a resume point.
     size_t numResults = recover_.numInstructions() - 1;
-    instructionResults_ = results;
-    if (!instructionResults_->isInitialized()) {
-        if (!instructionResults_->init(cx, numResults, fp_))
+    if (!results->isInitialized()) {
+        if (!results->init(cx, numResults, fp_))
             return false;
 
+        // No need to iterate over the only resume point.
+        if (!numResults) {
+            MOZ_ASSERT(results->isInitialized());
+            return true;
+        }
+
         // Fill with the results of recover instructions.
         SnapshotIterator s(*this);
+        s.instructionResults_ = results;
         while (s.moreInstructions()) {
             // Skip resume point and only interpret recover instructions.
             if (s.instruction()->isResumePoint()) {
@@ -1848,6 +1857,7 @@ SnapshotIterator::initInstructionResults(JSContext *cx, RInstructionResults *res
         }
     }
 
+    MOZ_ASSERT(results->isInitialized());
     return true;
 }
 

js/src/jit/JitFrameIterator.h

@@ -296,16 +296,21 @@ class RInstructionResults
 
 struct MaybeReadFallback
 {
+    enum NoGCValue {
+        NoGC_UndefinedValue,
+        NoGC_MagicOptimizedOut
+    };
+
     JSContext *maybeCx;
     JitActivation *activation;
     JitFrameIterator *frame;
-    const Value unreadablePlaceholder;
+    const NoGCValue unreadablePlaceholder_;
 
     MaybeReadFallback(const Value &placeholder = UndefinedValue())
       : maybeCx(nullptr),
         activation(nullptr),
         frame(nullptr),
-        unreadablePlaceholder(placeholder)
+        unreadablePlaceholder_(noGCPlaceholder(placeholder))
     {
     }
 
@@ -313,11 +318,23 @@ struct MaybeReadFallback
       : maybeCx(cx),
         activation(activation),
         frame(frame),
-        unreadablePlaceholder(UndefinedValue())
+        unreadablePlaceholder_(NoGC_UndefinedValue)
     {
     }
 
     bool canRecoverResults() { return maybeCx; }
+
+    Value unreadablePlaceholder() const {
+        if (unreadablePlaceholder_ == NoGC_MagicOptimizedOut)
+            return MagicValue(JS_OPTIMIZED_OUT);
+        return UndefinedValue();
+    }
+
+    NoGCValue noGCPlaceholder(Value v) const {
+        if (v.isMagic(JS_OPTIMIZED_OUT))
+            return NoGC_MagicOptimizedOut;
+        return NoGC_UndefinedValue;
+    }
 };
 
 
@@ -327,6 +344,7 @@ class RResumePoint;
 // to innermost frame).
 class SnapshotIterator
 {
+  protected:
     SnapshotReader snapshot_;
     RecoverReader recover_;
     IonJSFrameLayout *fp_;
@@ -388,6 +406,10 @@ class SnapshotIterator
 
     int32_t readOuterNumActualArgs() const;
 
+    // Used by recover instruction to store the value back into the instruction
+    // results array.
+    void storeInstructionResult(Value v);
+
   public:
     // Exhibits frame properties contained in the snapshot.
     uint32_t pcOffset() const;
@@ -420,14 +442,16 @@ class SnapshotIterator
         return recover_.moreInstructions();
     }
 
+  protected:
     // Register a vector used for storing the results of the evaluation of
     // recover instructions. This vector should be registered before the
     // beginning of the iteration. This function is in charge of allocating
     // enough space for all instructions results, and return false iff it fails.
     bool initInstructionResults(MaybeReadFallback &fallback);
-    bool initInstructionResults(JSContext *cx, RInstructionResults *results);
 
-    void storeInstructionResult(Value v);
+    // This function is used internally for computing the result of the recover
+    // instructions.
+    bool computeInstructionResults(JSContext *cx, RInstructionResults *results) const;
 
   public:
     // Handle iterating over frames of the snapshots.
@@ -461,7 +485,7 @@ class SnapshotIterator
 
         if (fallback.canRecoverResults()) {
             if (!initInstructionResults(fallback))
-                return fallback.unreadablePlaceholder;
+                return fallback.unreadablePlaceholder();
 
             if (allocationReadable(a))
                 return allocationValue(a);
@@ -469,7 +493,7 @@ class SnapshotIterator
             MOZ_ASSERT_UNREACHABLE("All allocations should be readable.");
         }
 
-        return fallback.unreadablePlaceholder;
+        return fallback.unreadablePlaceholder();
     }
 
     void readCommonFrameSlots(Value *scopeChain, Value *rval) {