2010-12-21 Oliver Hunt <oliver@apple.com>
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Dec 2010 22:54:14 +0000 (22:54 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Dec 2010 22:54:14 +0000 (22:54 +0000)
        Reviewed by Gavin Barraclough.

        ASSERTION FAILED: base->index() == m_codeBlock->argumentsRegister() while loading taobao.com
        https://bugs.webkit.org/show_bug.cgi?id=49006

        This problem was caused by having a parameter named 'arguments'.
        The fix is to treat parameters named 'arguments' as shadowing
        the actual arguments property, and so logically turn the function
        into one that doesn't "use" arguments.

        This required a bit of fiddling in the parser to ensure we correctly
        propagate the 'feature' of shadowing is set correctly.

        * bytecompiler/BytecodeGenerator.cpp:
        (JSC::BytecodeGenerator::createArgumentsIfNecessary):
          Change assertion to an early return as we may now reference
          a property named 'arguments' without being in a function that
          has the ArgumentsFeature
        * parser/JSParser.cpp:
        (JSC::JSParser::Scope::Scope):
        (JSC::JSParser::Scope::declareParameter):
        (JSC::JSParser::Scope::shadowsArguments):
        (JSC::JSParser::parseProgram):
        (JSC::JSParser::parseFormalParameters):
        (JSC::JSParser::parseFunctionInfo):
        * parser/Nodes.h:
        (JSC::ScopeNode::usesArguments):
2010-12-21  Oliver Hunt  <oliver@apple.com>

        Reviewed by Gavin Barraclough.

        ASSERTION FAILED: base->index() == m_codeBlock->argumentsRegister() while loading taobao.com
        https://bugs.webkit.org/show_bug.cgi?id=49006

        Add new tests to cover the cases of a parameter named arguments being used.
        Also correct a couple of existing (incorrect) tests.

        * fast/js/arguments-expected.txt:
        * fast/js/script-tests/arguments.js:
        (argumentsVarUndefined):
        (argumentsConstUndefined):
        (shadowedArgumentsLength):
        (shadowedArgumentsCallee):
        (shadowedArgumentsIndex):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@74428 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/ChangeLog
JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
JavaScriptCore/parser/JSParser.cpp
JavaScriptCore/parser/Nodes.h
LayoutTests/ChangeLog
LayoutTests/fast/js/arguments-expected.txt
LayoutTests/fast/js/script-tests/arguments.js

index a7ccfe5..85a35e6 100644 (file)
@@ -1,3 +1,33 @@
+2010-12-21  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Gavin Barraclough.
+
+        ASSERTION FAILED: base->index() == m_codeBlock->argumentsRegister() while loading taobao.com
+        https://bugs.webkit.org/show_bug.cgi?id=49006
+
+        This problem was caused by having a parameter named 'arguments'.
+        The fix is to treat parameters named 'arguments' as shadowing
+        the actual arguments property, and so logically turn the function
+        into one that doesn't "use" arguments.
+
+        This required a bit of fiddling in the parser to ensure we correctly
+        propagate the 'feature' of shadowing is set correctly.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::createArgumentsIfNecessary):
+          Change assertion to an early return as we may now reference
+          a property named 'arguments' without being in a function that
+          has the ArgumentsFeature
+        * parser/JSParser.cpp:
+        (JSC::JSParser::Scope::Scope):
+        (JSC::JSParser::Scope::declareParameter):
+        (JSC::JSParser::Scope::shadowsArguments):
+        (JSC::JSParser::parseProgram):
+        (JSC::JSParser::parseFormalParameters):
+        (JSC::JSParser::parseFunctionInfo):
+        * parser/Nodes.h:
+        (JSC::ScopeNode::usesArguments):
+
 2010-12-21  Daniel Bates  <dbates@rim.com>
 
         Reviewed by Eric Seidel and Darin Adler.
index 2303dfd..3a99957 100644 (file)
@@ -1599,7 +1599,9 @@ void BytecodeGenerator::createArgumentsIfNecessary()
 {
     if (m_codeType != FunctionCode)
         return;
-    ASSERT(m_codeBlock->usesArguments());
+    
+    if (!m_codeBlock->usesArguments())
+        return;
 
     // If we're in strict mode we tear off the arguments on function
     // entry, so there's no need to check if we need to create them
index 7d6fecd..4201555 100644 (file)
@@ -171,7 +171,7 @@ private:
     template <class TreeBuilder> ALWAYS_INLINE TreeArguments parseArguments(TreeBuilder&);
     template <bool strict, class TreeBuilder> ALWAYS_INLINE TreeProperty parseProperty(TreeBuilder&);
     template <class TreeBuilder> ALWAYS_INLINE TreeFunctionBody parseFunctionBody(TreeBuilder&);
-    template <class TreeBuilder> ALWAYS_INLINE TreeFormalParameterList parseFormalParameters(TreeBuilder&, bool& usesArguments);
+    template <class TreeBuilder> ALWAYS_INLINE TreeFormalParameterList parseFormalParameters(TreeBuilder&);
     template <class TreeBuilder> ALWAYS_INLINE TreeExpression parseVarDeclarationList(TreeBuilder&, int& declarations, const Identifier*& lastIdent, TreeExpression& lastInitializer, int& identStart, int& initStart, int& initEnd);
     template <class TreeBuilder> ALWAYS_INLINE TreeConstDeclList parseConstDeclarationList(TreeBuilder& context);
     enum FunctionRequirements { FunctionNoRequirements, FunctionNeedsName };
@@ -235,6 +235,7 @@ private:
     struct Scope {
         Scope(JSGlobalData* globalData, bool isFunction, bool strictMode)
             : m_globalData(globalData)
+            , m_shadowsArguments(false)
             , m_usesEval(false)
             , m_needsFullActivation(false)
             , m_allowsNewDecls(true)
@@ -301,8 +302,11 @@ private:
 
         bool declareParameter(const Identifier* ident)
         {
-            bool isValidStrictMode = m_declaredVariables.add(ident->ustring().impl()).second && m_globalData->propertyNames->eval != *ident && m_globalData->propertyNames->arguments != *ident;
+            bool isArguments = m_globalData->propertyNames->arguments == *ident;
+            bool isValidStrictMode = m_declaredVariables.add(ident->ustring().impl()).second && m_globalData->propertyNames->eval != *ident && !isArguments;
             m_isValidStrictMode = m_isValidStrictMode && isValidStrictMode;
+            if (isArguments)
+                m_shadowsArguments = true;
             return isValidStrictMode;
         }
         
@@ -362,9 +366,11 @@ private:
         void setStrictMode() { m_strictMode = true; }
         bool strictMode() const { return m_strictMode; }
         bool isValidStrictMode() const { return m_isValidStrictMode; }
+        bool shadowsArguments() const { return m_shadowsArguments; }
 
     private:
         JSGlobalData* m_globalData;
+        bool m_shadowsArguments : 1;
         bool m_usesEval : 1;
         bool m_needsFullActivation : 1;
         bool m_allowsNewDecls : 1;
@@ -488,7 +494,13 @@ bool JSParser::parseProgram()
         return true;
     IdentifierSet capturedVariables;
     scope->getCapturedVariables(capturedVariables);
-    m_globalData->parser->didFinishParsing(sourceElements, context.varDeclarations(), context.funcDeclarations(), context.features() | (scope->strictMode() ? StrictModeFeature : 0),
+    CodeFeatures features = context.features();
+    if (scope->strictMode())
+        features |= StrictModeFeature;
+    if (scope->shadowsArguments())
+        features |= ShadowsArgumentsFeature;
+
+    m_globalData->parser->didFinishParsing(sourceElements, context.varDeclarations(), context.funcDeclarations(), features,
                                            m_lastLine, context.numConstants(), capturedVariables);
     return false;
 }
@@ -1075,10 +1087,9 @@ template <class TreeBuilder> TreeStatement JSParser::parseStatement(TreeBuilder&
     }
 }
 
-template <class TreeBuilder> TreeFormalParameterList JSParser::parseFormalParameters(TreeBuilder& context, bool& usesArguments)
+template <class TreeBuilder> TreeFormalParameterList JSParser::parseFormalParameters(TreeBuilder& context)
 {
     matchOrFail(IDENT);
-    usesArguments = m_globalData->propertyNames->arguments == *m_token.m_data.ident;
     failIfFalseIfStrict(declareParameter(m_token.m_data.ident));
     TreeFormalParameterList list = context.createFormalParameterList(*m_token.m_data.ident);
     TreeFormalParameterList tail = list;
@@ -1087,10 +1098,8 @@ template <class TreeBuilder> TreeFormalParameterList JSParser::parseFormalParame
         next();
         matchOrFail(IDENT);
         const Identifier* ident = m_token.m_data.ident;
-        usesArguments |= m_globalData->propertyNames->arguments == *m_token.m_data.ident;
         failIfFalseIfStrict(declareParameter(ident));
         next();
-        usesArguments = usesArguments || m_globalData->propertyNames->arguments == *ident;
         tail = context.createFormalParameterList(tail, *ident);
     }
     return list;
@@ -1119,9 +1128,8 @@ template <JSParser::FunctionRequirements requirements, bool nameIsInContainingSc
     } else if (requirements == FunctionNeedsName)
         return false;
     consumeOrFail(OPENPAREN);
-    bool usesArguments = false;
     if (!match(CLOSEPAREN)) {
-        parameters = parseFormalParameters(context, usesArguments);
+        parameters = parseFormalParameters(context);
         failIfFalse(parameters);
     }
     consumeOrFail(CLOSEPAREN);
@@ -1133,8 +1141,6 @@ template <JSParser::FunctionRequirements requirements, bool nameIsInContainingSc
 
     body = parseFunctionBody(context);
     failIfFalse(body);
-    if (usesArguments)
-        context.setUsesArguments(body);
     if (functionScope->strictMode() && name) {
         failIfTrue(m_globalData->propertyNames->arguments == *name);
         failIfTrue(m_globalData->propertyNames->eval == *name);
index 6930f63..b8e9cdf 100644 (file)
@@ -58,7 +58,10 @@ namespace JSC {
     const CodeFeatures CatchFeature = 1 << 5;
     const CodeFeatures ThisFeature = 1 << 6;
     const CodeFeatures StrictModeFeature = 1 << 7;
-    const CodeFeatures AllFeatures = EvalFeature | ClosureFeature | AssignFeature | ArgumentsFeature | WithFeature | CatchFeature | ThisFeature;
+    const CodeFeatures ShadowsArgumentsFeature = 1 << 8;
+    
+    
+    const CodeFeatures AllFeatures = EvalFeature | ClosureFeature | AssignFeature | ArgumentsFeature | WithFeature | CatchFeature | ThisFeature | StrictModeFeature | ShadowsArgumentsFeature;
 
     enum Operator {
         OpEqual,
@@ -1409,7 +1412,7 @@ namespace JSC {
         CodeFeatures features() { return m_features; }
 
         bool usesEval() const { return m_features & EvalFeature; }
-        bool usesArguments() const { return m_features & ArgumentsFeature; }
+        bool usesArguments() const { return (m_features & ArgumentsFeature) && !(m_features & ShadowsArgumentsFeature); }
         bool isStrictMode() const { return m_features & StrictModeFeature; }
         void setUsesArguments() { m_features |= ArgumentsFeature; }
         bool usesThis() const { return m_features & ThisFeature; }
index 45c516b..c4f1e45 100644 (file)
@@ -1,3 +1,21 @@
+2010-12-21  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Gavin Barraclough.
+
+        ASSERTION FAILED: base->index() == m_codeBlock->argumentsRegister() while loading taobao.com
+        https://bugs.webkit.org/show_bug.cgi?id=49006
+
+        Add new tests to cover the cases of a parameter named arguments being used.
+        Also correct a couple of existing (incorrect) tests.
+
+        * fast/js/arguments-expected.txt:
+        * fast/js/script-tests/arguments.js:
+        (argumentsVarUndefined):
+        (argumentsConstUndefined):
+        (shadowedArgumentsLength):
+        (shadowedArgumentsCallee):
+        (shadowedArgumentsIndex):
+
 2010-12-21  Jian Li  <jianli@chromium.org>
 
         Reviewed by Darin Adler.
index 0970334..f195a73 100644 (file)
@@ -126,9 +126,14 @@ PASS access_after_delete_extra_3(1, 2, 3, 4, 5) is 3
 PASS access_after_delete_extra_5(1, 2, 3, 4, 5) is 5
 PASS argumentsParam(true) is true
 PASS argumentsFunctionConstructorParam(true) is true
-FAIL argumentsVarUndefined() should be undefined. Was [object Arguments]
-FAIL argumentsConstUndefined() should be undefined. Was [object Arguments]
+PASS argumentsVarUndefined() is '[object Arguments]'
+PASS argumentsConstUndefined() is '[object Arguments]'
 PASS argumentCalleeInException() is argumentCalleeInException
+PASS shadowedArgumentsApply([true]) is true
+PASS shadowedArgumentsLength([]) is 0
+PASS shadowedArgumentsLength() threw exception TypeError: 'undefined' is not an object (evaluating 'arguments.length').
+PASS shadowedArgumentsCallee([]) is undefined.
+PASS shadowedArgumentsIndex([true]) is true
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 5810af7..2ea1ba9 100644 (file)
@@ -514,16 +514,16 @@ shouldBeTrue("argumentsFunctionConstructorParam(true)");
 function argumentsVarUndefined()
 {
     var arguments;
-    return arguments;
+    return String(arguments);
 }
-shouldBeUndefined("argumentsVarUndefined()");
+shouldBe("argumentsVarUndefined()", "'[object Arguments]'");
 
 function argumentsConstUndefined()
 {
     const arguments;
-    return arguments;
+    return String(arguments);
 }
-shouldBeUndefined("argumentsConstUndefined()");
+shouldBe("argumentsConstUndefined()", "'[object Arguments]'");
 
 function argumentCalleeInException() {
     try {
@@ -534,4 +534,26 @@ function argumentCalleeInException() {
 }
 shouldBe("argumentCalleeInException()", "argumentCalleeInException")
 
+function shadowedArgumentsApply(arguments) {
+    return function(a){ return a; }.apply(null, arguments);
+}
+
+function shadowedArgumentsLength(arguments) {
+    return arguments.length;
+}
+
+function shadowedArgumentsCallee(arguments) {
+    return arguments.callee;
+}
+
+function shadowedArgumentsIndex(arguments) {
+    return arguments[0]
+}
+
+shouldBeTrue("shadowedArgumentsApply([true])");
+shouldBe("shadowedArgumentsLength([])", '0');
+shouldThrow("shadowedArgumentsLength()");
+shouldBeUndefined("shadowedArgumentsCallee([])");
+shouldBeTrue("shadowedArgumentsIndex([true])");
+
 var successfullyParsed = true;