Bug 1342483 - Preserve envChain in Ion if script uses lexical environments r=jandem
authorTed Campbell <tcampbell@mozilla.com>
Fri, 03 Mar 2017 10:10:13 -0500
changeset 345809 e5a3bbe621c9
parent 345808 4b805bbd9a83
child 345810 2bf7fb2523c3
push id38302
push usertcampbell@mozilla.com
push dateFri, 03 Mar 2017 17:16:55 +0000
treeherderautoland@e5a3bbe621c9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1342483
milestone54.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 1342483 - Preserve envChain in Ion if script uses lexical environments r=jandem Under rare cases, Ion was optimizing out |envChain| while lexical environments were in use, leading to a crash during bailout. This extends the criteria for preserving the |envChain| slot to include lexical blocks. MozReview-Commit-ID: 4sd42F4TIq8
js/src/jit-test/tests/ion/bug1342483-1.js
js/src/jit-test/tests/ion/bug1342483-2.js
js/src/jit/BaselineBailouts.cpp
js/src/jit/CompileInfo.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug1342483-1.js
@@ -0,0 +1,6 @@
+// |jit-test| error: ReferenceError
+for (var i = 0; i < 10; ++i) {}
+for (var i = 0; i < 3; i++) {
+    throw eval(raisesException);
+    function ff() {}
+}
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug1342483-2.js
@@ -0,0 +1,17 @@
+// |jit-test| error: () => g
+function f() {
+    // Block Scope
+    {
+        // Lexical capture creates environment
+        function g() {}
+        var h = [() => g];
+
+        // OSR Re-Entry Point
+        for (;;) { break; }
+
+        // Type Invalidation + Throw
+        throw h[0];
+    }
+}
+
+f();
--- a/js/src/jit/BaselineBailouts.cpp
+++ b/js/src/jit/BaselineBailouts.cpp
@@ -732,16 +732,31 @@ InitFromBailout(JSContext* cx, HandleScr
             if (fun && fun->needsFunctionEnvironmentObjects()) {
                 MOZ_ASSERT(fun->nonLazyScript()->initialEnvironmentShape());
                 MOZ_ASSERT(!fun->needsExtraBodyVarEnvironment());
                 flags |= BaselineFrame::HAS_INITIAL_ENV;
             }
         } else {
             MOZ_ASSERT(v.isUndefined() || v.isMagic(JS_OPTIMIZED_OUT));
 
+#ifdef DEBUG
+            // The |envChain| slot must not be optimized out if the currently
+            // active scope requires any EnvironmentObjects beyond what is
+            // available at body scope. This checks that scope chain does not
+            // require any such EnvironmentObjects.
+            // See also: |CompileInfo::isObservableFrameSlot|
+            jsbytecode* pc = script->offsetToPC(iter.pcOffset());
+            Scope* scopeIter = script->innermostScope(pc);
+            while (scopeIter != script->bodyScope()) {
+                MOZ_ASSERT(scopeIter);
+                MOZ_ASSERT(!scopeIter->hasEnvironment());
+                scopeIter = scopeIter->enclosing();
+            }
+#endif
+
             // Get env chain from function or script.
             if (fun) {
                 // If pcOffset == 0, we may have to push a new call object, so
                 // we leave envChain nullptr and enter baseline code before
                 // the prologue.
                 if (!IsPrologueBailout(iter, excInfo))
                     envChain = fun->environment();
             } else if (script->module()) {
--- a/js/src/jit/CompileInfo.h
+++ b/js/src/jit/CompileInfo.h
@@ -235,22 +235,27 @@ class CompileInfo
                     continue;
                 BindingLocation loc = bi.location();
                 if (loc.kind() == BindingLocation::Kind::Frame) {
                     thisSlotForDerivedClassConstructor_ = mozilla::Some(localSlot(loc.slot()));
                     break;
                 }
             }
         }
+
+        // If the script uses an environment in body, the environment chain
+        // will need to be observable.
+        needsBodyEnvironmentObject_ = script->needsBodyEnvironment();
     }
 
     explicit CompileInfo(unsigned nlocals)
       : script_(nullptr), fun_(nullptr), osrPc_(nullptr),
         analysisMode_(Analysis_None), scriptNeedsArgsObj_(false),
-        mayReadFrameArgsDirectly_(false), inlineScriptTree_(nullptr)
+        mayReadFrameArgsDirectly_(false), inlineScriptTree_(nullptr),
+        needsBodyEnvironmentObject_(false)
     {
         nimplicit_ = 0;
         nargs_ = 0;
         nlocals_ = nlocals;
         nstack_ = 1;  /* For FunctionCompiler::pushPhiInput/popPhiOutput */
         nslots_ = nlocals_ + nstack_;
     }
 
@@ -429,31 +434,40 @@ class CompileInfo
     AnalysisMode analysisMode() const {
         return analysisMode_;
     }
 
     bool isAnalysis() const {
         return analysisMode_ != Analysis_None;
     }
 
+    bool needsBodyEnvironmentObject() const {
+        return needsBodyEnvironmentObject_;
+    }
+
     // Returns true if a slot can be observed out-side the current frame while
     // the frame is active on the stack.  This implies that these definitions
     // would have to be executed and that they cannot be removed even if they
     // are unused.
     bool isObservableSlot(uint32_t slot) const {
         if (isObservableFrameSlot(slot))
             return true;
 
         if (isObservableArgumentSlot(slot))
             return true;
 
         return false;
     }
 
     bool isObservableFrameSlot(uint32_t slot) const {
+        // The |envChain| value must be preserved if environments are added
+        // after the prologue.
+        if (needsBodyEnvironmentObject() && slot == environmentChainSlot())
+            return true;
+
         if (!funMaybeLazy())
             return false;
 
         // The |this| value must always be observable.
         if (slot == thisSlot())
             return true;
 
         // The |this| frame slot in derived class constructors should never be
@@ -490,19 +504,21 @@ class CompileInfo
 
         return false;
     }
 
     // Returns true if a slot can be recovered before or during a bailout.  A
     // definition which can be observed and recovered, implies that this
     // definition can be optimized away as long as we can compute its values.
     bool isRecoverableOperand(uint32_t slot) const {
-        // If this script is not a function, then none of the slots are
-        // observable.  If it this |slot| is not observable, thus we can always
-        // recover it.
+        // The |envChain| value cannot be recovered if environments can be
+        // added in body (after the prologue).
+        if (needsBodyEnvironmentObject() && slot == environmentChainSlot())
+            return false;
+
         if (!funMaybeLazy())
             return true;
 
         // The |this| and the |envChain| values can be recovered.
         if (slot == thisSlot() || slot == environmentChainSlot())
             return true;
 
         if (isObservableFrameSlot(slot))
@@ -542,14 +558,18 @@ class CompileInfo
 
     // Record the state of previous bailouts in order to prevent compiling the
     // same function identically the next time.
     bool hadOverflowBailout_;
 
     bool mayReadFrameArgsDirectly_;
 
     InlineScriptTree* inlineScriptTree_;
+
+    // Whether a script needs environments within its body. This informs us
+    // that the environment chain is not easy to reconstruct.
+    bool needsBodyEnvironmentObject_;
 };
 
 } // namespace jit
 } // namespace js
 
 #endif /* jit_CompileInfo_h */