Title: [204305] trunk
Revision
204305
Author
sbar...@apple.com
Date
2016-08-09 15:03:28 -0700 (Tue, 09 Aug 2016)

Log Message

Parser<LexerType>::parseFunctionInfo() has the wrong info about captured vars when a function is not cached.
https://bugs.webkit.org/show_bug.cgi?id=160671
<rdar://problem/27756112>

Reviewed by Mark Lam.

Source/_javascript_Core:

There was a bug in our captured variable analysis when a function has a default
parameter _expression_ that is a function that captures something from the parent scope.
The bug was that we were relying on the SourceProviderCache to succeed for the
analysis to work. This is obviously wrong. I've fixed this to work regardless
of getting a cache hit. To prevent future bugs that rely on the success of the
SourceProviderCache, I've made the validate testing mode disable the SourceProviderCache

* parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionInfo):
* parser/Parser.h:
(JSC::Scope::setInnerArrowFunctionUsesEvalAndUseArgumentsIfNeeded):
(JSC::Scope::addClosedVariableCandidateUnconditionally):
(JSC::Scope::collectFreeVariables):
* runtime/Options.h:

Tools:

* Scripts/run-jsc-stress-tests:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (204304 => 204305)


--- trunk/Source/_javascript_Core/ChangeLog	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-08-09 22:03:28 UTC (rev 204305)
@@ -1,3 +1,26 @@
+2016-08-09  Saam Barati  <sbar...@apple.com>
+
+        Parser<LexerType>::parseFunctionInfo() has the wrong info about captured vars when a function is not cached.
+        https://bugs.webkit.org/show_bug.cgi?id=160671
+        <rdar://problem/27756112>
+
+        Reviewed by Mark Lam.
+
+        There was a bug in our captured variable analysis when a function has a default
+        parameter _expression_ that is a function that captures something from the parent scope.
+        The bug was that we were relying on the SourceProviderCache to succeed for the
+        analysis to work. This is obviously wrong. I've fixed this to work regardless
+        of getting a cache hit. To prevent future bugs that rely on the success of the
+        SourceProviderCache, I've made the validate testing mode disable the SourceProviderCache
+
+        * parser/Parser.cpp:
+        (JSC::Parser<LexerType>::parseFunctionInfo):
+        * parser/Parser.h:
+        (JSC::Scope::setInnerArrowFunctionUsesEvalAndUseArgumentsIfNeeded):
+        (JSC::Scope::addClosedVariableCandidateUnconditionally):
+        (JSC::Scope::collectFreeVariables):
+        * runtime/Options.h:
+
 2016-08-08  Mark Lam  <mark....@apple.com>
 
         ASSERTION FAILED: hasInlineStorage() in JSFinalObject::visitChildren().

Modified: trunk/Source/_javascript_Core/parser/Parser.cpp (204304 => 204305)


--- trunk/Source/_javascript_Core/parser/Parser.cpp	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Source/_javascript_Core/parser/Parser.cpp	2016-08-09 22:03:28 UTC (rev 204305)
@@ -1939,6 +1939,8 @@
 {
     RELEASE_ASSERT(isFunctionParseMode(mode));
 
+    ScopeRef parentScope = currentScope();
+
     bool upperScopeIsGenerator = currentScope()->isGenerator();
     AutoPopScopeRef functionScope(this, pushScope());
     functionScope->setSourceParseMode(mode);
@@ -1954,6 +1956,9 @@
     FunctionBodyType functionBodyType;
 
     auto loadCachedFunction = [&] () -> bool {
+        if (UNLIKELY(!Options::useSourceProviderCache()))
+            return false;
+
         ASSERT(parametersStart != -1);
         ASSERT(startColumn != -1);
 
@@ -2133,8 +2138,11 @@
     // as their own scope).
     UniquedStringImplPtrSet nonLocalCapturesFromParameterExpressions;
     functionScope->forEachUsedVariable([&] (UniquedStringImpl* impl) {
-        if (!functionScope->hasDeclaredParameter(impl))
+        if (!functionScope->hasDeclaredParameter(impl)) {
             nonLocalCapturesFromParameterExpressions.add(impl);
+            if (TreeBuilder::NeedsFreeVariableInfo)
+                parentScope->addClosedVariableCandidateUnconditionally(impl);
+        }
     });
 
     auto performParsingFunctionBody = [&] {

Modified: trunk/Source/_javascript_Core/parser/Parser.h (204304 => 204305)


--- trunk/Source/_javascript_Core/parser/Parser.h	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Source/_javascript_Core/parser/Parser.h	2016-08-09 22:03:28 UTC (rev 204305)
@@ -539,6 +539,11 @@
         if (usedVariablesContains(m_vm->propertyNames->arguments.impl()))
             setInnerArrowFunctionUsesArguments();
     }
+
+    void addClosedVariableCandidateUnconditionally(UniquedStringImpl* impl)
+    {
+        m_closedVariableCandidates.add(impl);
+    }
     
     void collectFreeVariables(Scope* nestedScope, bool shouldTrackClosedVariables)
     {

Modified: trunk/Source/_javascript_Core/runtime/Options.h (204304 => 204305)


--- trunk/Source/_javascript_Core/runtime/Options.h	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Source/_javascript_Core/runtime/Options.h	2016-08-09 22:03:28 UTC (rev 204305)
@@ -379,6 +379,8 @@
     \
     v(bool, reportLLIntStats, false, Configurable, "Reports LLInt statistics") \
     v(optionString, llintStatsFile, nullptr, Configurable, "File to collect LLInt statistics in") \
+    \
+    v(bool, useSourceProviderCache, true, Normal, "If false, the parser will not use the source provider cache. It's good to verify everything works when this is false. Because the cache is so successful, it can mask bugs.") \
 
 enum OptionEquivalence {
     SameOption,

Modified: trunk/Tools/ChangeLog (204304 => 204305)


--- trunk/Tools/ChangeLog	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Tools/ChangeLog	2016-08-09 22:03:28 UTC (rev 204305)
@@ -1,3 +1,13 @@
+2016-08-09  Saam Barati  <sbar...@apple.com>
+
+        Parser<LexerType>::parseFunctionInfo() has the wrong info about captured vars when a function is not cached.
+        https://bugs.webkit.org/show_bug.cgi?id=160671
+        <rdar://problem/27756112>
+
+        Reviewed by Mark Lam.
+
+        * Scripts/run-jsc-stress-tests:
+
 2016-08-09  Alexey Proskuryakov  <a...@apple.com>
 
         Make directory reading code in iOSSimulatorDevices() more strict

Modified: trunk/Tools/Scripts/run-jsc-stress-tests (204304 => 204305)


--- trunk/Tools/Scripts/run-jsc-stress-tests	2016-08-09 21:54:58 UTC (rev 204304)
+++ trunk/Tools/Scripts/run-jsc-stress-tests	2016-08-09 22:03:28 UTC (rev 204305)
@@ -800,7 +800,7 @@
 end
 
 def runNoCJITValidatePhases
-    run("no-cjit-validate-phases", "--validateBytecode=true", "--validateGraphAtEachPhase=true", *NO_CJIT_OPTIONS)
+    run("no-cjit-validate-phases", "--validateBytecode=true", "--validateGraphAtEachPhase=true", "--useSourceProviderCache=false", *NO_CJIT_OPTIONS)
 end
 
 def runDefault
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to