Bug 163208

Created attachment 291304 [details] Patch Draft version of the patch. Later will be added more tests

Created attachment 291309 [details] Patch Rebase to latest version

Created attachment 294188 [details] Patch Patch for review, later I will provide more tests

Created attachment 294284 [details] Patch Try to fix 32bit builds

Comment on attachment 294284 [details] Patch Attachment 294284 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2486106 New failing tests: imported/w3c/web-platform-tests/IndexedDB/transaction-create_in_versionchange.htm imported/w3c/web-platform-tests/IndexedDB/transaction-abort-index-metadata-revert.html

Created attachment 294311 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 294413 [details] Patch One more, try to fix 32bit builds

Created attachment 294533 [details] Patch One more, try to fix 32bit builds

Comment on attachment 294533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294533&action=review I think the main approach is OK, but there are a lot of little things I think might be wrong or could be better/simpler. Comments below. > Source/JavaScriptCore/ChangeLog:13 > + Current patch implement Annex B.3.3 that is related to hoisting of function > + declaration in eval. https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation > + Function declaration in eval should create variable with function name in function scope > + where eval is invoked or bind to variable if it declared outside of the eval. If variable is created > + it can be removed by delete a; command. If eval is invoke in block scope that > + contains let/const variable with the same name as function declaration we do not binding It's worth also talking about your implementation in the changelog. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851 > + // Fixme: we do not have llint optimization for those op_codes Please file a bug for this or implement the optimization. Also, should be FIXME not Fixme. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754 > + auto varKind = [&] (UniquedStringImpl* uid) -> VarKind { > + return evalNode->captures(uid) ? VarKind::Scope : VarKind::Stack; > + }; > + > + for (FunctionMetadataNode* function : evalNode->functionStack()) { > + VarKind kind = varKind(function->ident().impl()); > + if (kind == VarKind::Scope) > + m_codeBlock->addFunctionDecl(makeFunction(function)); > + else > + m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable)); > + } Why is this change needed? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193 > +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope) This function name is incorrect. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196 > + m_codeBlock->addPropertyAccessInstruction(instructions().size()); This looks wrong. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201 > + dst = tempDestination(dst); > + emitOpcode(op_is_scope_var_type); > + instructions().append(kill(dst)); Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209 > + m_codeBlock->addPropertyAccessInstruction(instructions().size()); This looks wrong. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215 > + remove newline. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217 > + instructions().append(localScopeDepth()); Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100 > + JSScope* scope = jsCast<JSScope*>(child.value()); Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103 > + if (scope->symbolTable() != nullptr) > + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope; How we normally do this is: if (SymbolTable* symbolTable = scope->symbolTable()) .... > Source/JavaScriptCore/dfg/DFGNode.h:983 > + int32_t getOpInfo2() Please give this a name for your usage of it. > Source/JavaScriptCore/dfg/DFGOperations.h:193 > +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t); should be uint32_t > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777 > + callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2()); We usually give names to the opInfo methods so we know what it's for. > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:821 > + linkSlowCase(iter); > + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_is_scope_var_type); > + slowPathCall.call(); This should not have a slow path. The fast path should do the function call. > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:828 > + linkSlowCase(iter); > + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_resolve_closest_var_scope); > + slowPathCall.call(); ditto > Source/JavaScriptCore/parser/Parser.h:1182 > + // We allways try to hoist function to top scope inside of eval I don't think this comment adds anything. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826 > + CHECK_EXCEPTION(); An exception could not be thrown by the previous statement. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832 > + else if (scope->symbolTable() != nullptr) > + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope; We usually do: else if (SymbolTable* symbolTable = scope->symbolTable()) > Source/JavaScriptCore/runtime/JSScope.cpp:218 > +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope) should be unsigned instead of int > Source/JavaScriptCore/runtime/JSScope.cpp:224 > + int skipedScopes = 0; typo, should be: "skippedScopes" also, should be unsigned. > Source/JavaScriptCore/runtime/JSScope.cpp:245 > + if (skipedScopes < skipScope) { > + ++skipedScopes; > + continue; > + } Why not just have the bytecode emit starting from the base-most scope of the function and you can remove the skipScope parameter entirely?

<rdar://problem/29145081>

Created attachment 296083 [details] Patch Fix comments

Created attachment 296084 [details] Patch Fix build & comments

Comment on attachment 294533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294533&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851 >> + // Fixme: we do not have llint optimization for those op_codes > > Please file a bug for this or implement the optimization. > Also, should be FIXME not Fixme. Just a question, do we need optimization there? I'm not familiar with this part. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754 >> + } > > Why is this change needed? It is allow to fix following code: eval("'use strict'; class C { constructor() { } }; function foo() { return new C; }; foo()"); >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193 >> +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope) > > This function name is incorrect. Renamed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196 >> + m_codeBlock->addPropertyAccessInstruction(instructions().size()); > > This looks wrong. Removed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201 >> + instructions().append(kill(dst)); > > Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong. Hmm, without dst = tempDestination(dst), I received segment 11. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209 >> + m_codeBlock->addPropertyAccessInstruction(instructions().size()); > > This looks wrong. Removed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215 >> + > > remove newline. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217 >> + instructions().append(localScopeDepth()); > > Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right? Ohh, not sure if I got how I can do this. >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100 >> + JSScope* scope = jsCast<JSScope*>(child.value()); > > Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject? Hmm, it is rewritten function: bool JSScope::isVarScope() { if (type() != LexicalEnvironmentType) return false; return jsCast<JSLexicalEnvironment*>(this)->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope; } So I expect that it should work >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103 >> + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope; > > How we normally do this is: > if (SymbolTable* symbolTable = scope->symbolTable()) > .... Fixed >> Source/JavaScriptCore/dfg/DFGNode.h:983 >> + int32_t getOpInfo2() > > Please give this a name for your usage of it. Renamed to skipScope() >> Source/JavaScriptCore/dfg/DFGOperations.h:193 >> +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t); > > should be uint32_t Done >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777 >> + callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2()); > > We usually give names to the opInfo methods so we know what it's for. Fixed >> Source/JavaScriptCore/parser/Parser.h:1182 >> + // We allways try to hoist function to top scope inside of eval > > I don't think this comment adds anything. Removed >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826 >> + CHECK_EXCEPTION(); > > An exception could not be thrown by the previous statement. Removed >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832 >> + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope; > > We usually do: > else if (SymbolTable* symbolTable = scope->symbolTable()) Fixed >> Source/JavaScriptCore/runtime/JSScope.cpp:218 >> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope) > > should be unsigned instead of int Fixed >> Source/JavaScriptCore/runtime/JSScope.cpp:224 >> + int skipedScopes = 0; > > typo, should be: "skippedScopes" > also, should be unsigned. Removed

Comment on attachment 296084 [details] Patch Attachment 296084 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2621362 New failing tests: media/modern-media-controls/media-controller/media-controller-resize.html

Created attachment 296087 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 294533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294533&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2199 > + dst = tempDestination(dst); Yeah, you're right. dst could be nullptr. However, you don't want to use tempDestination here, you want finalDestination. >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217 >>> + instructions().append(localScopeDepth()); >> >> Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right? > > Ohh, not sure if I got how I can do this. The first thing that the bytecode generator does is emit a get_scope. You'll just need to make sure to record what RegisterID* is that scope inside BytecodeGenerator(Eval) constructor, and then your operation can just work off that scope, instead of having to record the localDepth(). This localDepth() just adds more complexity to what you're trying to do for no reason IMO.

Comment on attachment 296084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296084&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2876 > + // Fixme: we do not have llint optimization for those op_codes Please file a bug for this if you think we should do it. That said, this is way more than a LLInt optimization, knowing which scope you'll resolve to helps in all tiers. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:769 > + for (FunctionMetadataNode* function : evalNode->functionStack()) { > + VarKind kind = varKind(function->ident().impl()); > + if (kind == VarKind::Scope) > + m_codeBlock->addFunctionDecl(makeFunction(function)); > + else > + m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable)); > + } Do you mind elaborating on your previous example for why you said this is needed, I don't quite get it. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193 > + // resolve_scope dst, id, ResolveType, skipLocalScope This comment is wrong. It has the wrong bytecode name. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2208 > + // resolve_scope dst, scope id Ditto. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1109 > + ASSERT_NOT_REACHED(); > + break; This feels wrong, AI should be able to handle a constant you don't expect. That said, I think this only allows for scope arguments, however, I think this would break if a `with` scope or some other scope was the argument to this. It seems totally possible that other scopes can be your constant argument here. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2848 > + case ResolveClosestVarScope: You also need a rule for IsVarScopeType here. Did this not cause any issues for you? If seems like it should lead to a crash, so please add a test that stresses this. > Source/JavaScriptCore/dfg/DFGNode.h:994 > + int32_t skipScope() > + { > + return m_opInfo2.as<int32_t>(); > + } should be uint32_t > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2118 > + loadisFromInstruction(4, t0) I don't think this is needed. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124 > + loadisFromInstruction(4, t0) Ditto > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2069 > + loadisFromInstruction(4, t0) ditto > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2075 > + loadisFromInstruction(4, t0) ditto

Created attachment 297708 [details] Patch Fix comments

Created attachment 297718 [details] Patch Small fixes

Comment on attachment 296084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296084&action=review >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2876 >> + // Fixme: we do not have llint optimization for those op_codes > > Please file a bug for this if you think we should do it. That said, this is way more than a LLInt optimization, knowing which scope you'll resolve to helps in all tiers. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:769 >> + } > > Do you mind elaborating on your previous example for why you said this is needed, I don't quite get it. I've removed this change. It seems that not ready yet. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193 >> + // resolve_scope dst, id, ResolveType, skipLocalScope > > This comment is wrong. It has the wrong bytecode name. Removed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2208 >> + // resolve_scope dst, scope id > > Ditto. Removed >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1109 >> + break; > > This feels wrong, AI should be able to handle a constant you don't expect. That said, I think this only allows for scope arguments, however, I think this would break if a `with` scope or some other scope was the argument to this. It seems totally possible that other scopes can be your constant argument here. Yeah. I've just removed this part. I did not managed to find case when child.value().isObject() is valid. So Only forNode(node).setType(m_graph, SpecBoolean) is left. >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2848 >> + case ResolveClosestVarScope: > > You also need a rule for IsVarScopeType here. > Did this not cause any issues for you? If seems like it should lead to a crash, so please add a test that stresses this. Done >> Source/JavaScriptCore/dfg/DFGNode.h:994 >> + } > > should be uint32_t Removed >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2118 >> + loadisFromInstruction(4, t0) > > I don't think this is needed. Removed >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124 >> + loadisFromInstruction(4, t0) > > Ditto Removed >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2069 >> + loadisFromInstruction(4, t0) > > ditto Removed >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2075 >> + loadisFromInstruction(4, t0) > > ditto Removed

Created attachment 297742 [details] Patch Additional fixes

Comment on attachment 297742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297742&action=review This code mostly LGTM, some comments below. Also, it's worth opening a bugzilla for a bug that has existed since I implemented the first version of sloppy hoisting: function foo() { { let bar; { function bar() { } } } return bar; } in jsc: foo() === function bar() { } expected behavior: foo(); throws a ReferenceError I believe your eval code exhibits the same bad behavior for local lexical variables in the eval. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2822 > + // Fixme: https://bugs.webkit.org/show_bug.cgi?id=166418 Style: "Fixme" => "FIXME" > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2186 > +RegisterID* BytecodeGenerator::emitResolveOuterScope(RegisterID* dst, const Identifier& property) Nit: Lets call this: "emitResolveOuterVarScope" > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2198 > +RegisterID* BytecodeGenerator::emitCheckIfScopeVarType(RegisterID* dst, RegisterID* scope) Nit: Lets call this: "emitCheckIfScopeIsVarType" > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:829 > +void JIT::emitSlow_op_is_scope_var_type(Instruction* currentInstruction, Vector<SlowCaseEntry>::iterator& iter) > +{ > + linkSlowCase(iter); > + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_is_scope_var_type); > + slowPathCall.call(); > +} > + > +void JIT::emitSlow_op_resolve_closest_var_scope(Instruction* currentInstruction, Vector<SlowCaseEntry>::iterator& iter) > +{ > + linkSlowCase(iter); > + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_resolve_closest_var_scope); > + slowPathCall.call(); > +} These shouldn't be slow paths. The opcodes should just do the call on the fast patch, and should not have slow paths. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2124 > +_llint_op_resolve_closest_var_scope: > + traceExecution() > + callOpcodeSlowPath(_slow_path_resolve_closest_var_scope) > + dispatch(4) > + > +_llint_op_is_scope_var_type: > + traceExecution() > + callOpcodeSlowPath(_slow_path_is_scope_var_type) > + dispatch(3) The 32 and 64 implementations are the same, so they can just go inside LowLevelInterpreter.asm > Source/JavaScriptCore/parser/Parser.h:639 > + addResult.iterator->value.setIsFunction(); Why is this change needed? > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:834 > + bool result = false; > + if (scope->isGlobalObject()) > + result = true; > + else if (SymbolTable* scopeSymbolTable = scope->symbolTable()) > + result = scopeSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope; This doesn't match the code inside DFGOperations. Which code is correct? > Source/JavaScriptCore/runtime/JSScope.cpp:234 > + if (object->hasProperty(exec, ident)) > + return object; I think you need to check an exception here. > Source/JavaScriptCore/runtime/JSScope.cpp:242 > + if (object->hasProperty(exec, ident)) { ditto > JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:14 > +function foo() { > + { > + var f = 20; > + eval('var f = 15; eval(" { function f() { }; } ")'); > + assert(typeof f, "function"); > + } > + assert(typeof f, "function", "#1"); > +} Another good test to have: function foo() { eval("if (false) { function bar() { } }"); assert(bar === undefined); } > JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:56 > +/* //TODO: Set error > +// Fixme: Current test does not work because it should raise exception that f could > +// not be redeclared > +function goo() { > + { > + var error = false; > + try { > + let f = 20; > + eval('var f = 15; eval(" { function f() { }; } ")'); > + } catch (e) { > + error = e instanceof SyntaxError; > + } > + assert(error, true, "syntax error should be raisen"); > + } > + assert(typeof f, "undefined", "#6"); > +} > + > +for (var i = 0; i < 10000; i++) { > + goo(); > + assert(typeof f, "undefined", "#7"); > +} > +*/ What's happening with this? > JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:65 > +function hoo() { > + { > + let h = 20; > + eval('var h = 15; eval(" if (false){ function h() { }; } ");'); > + assert(h, 15); > + } > + assert(typeof h, "undefined", "#8"); > +} Another good test: function foo() { let h = 20; eval("var h; if (false) { function h() { } }"); return h; } assert(foo() === 20);

Created attachment 298664 [details] Patch Fix comments

Comment on attachment 298664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298664&action=review This LGTM, I think it's just about ready to land, just a few remaining comments. > Source/JavaScriptCore/bytecode/BytecodeList.json:157 > + { "name" : "op_resolve_closest_var_scope", "length" : 4 } I have a naming suggestion for this bytecode (and the various functions & DFG node for it) Instead of calling it "resolve_closest_var_scope", we should maybe call it: "resolve_closest_or_var_scope" since the intention is to do a normal scope walk, but stop if we see a var scope. The objective is not to *always* stop at the closest var scope. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:-2115 > - revert > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-2068 > - revert > Source/JavaScriptCore/parser/Parser.h:639 > + addResult.iterator->value.setIsFunction(); I don't think this is strictly true here. Is this needed? > Source/JavaScriptCore/runtime/JSScope.cpp:219 > +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident) This function is almost identical to the below resolve. Maybe you can combine them by defining a common function that takes some kind of shouldStopEarly lambda? And just make that function ALWAYS_INLINE. > Source/JavaScriptCore/runtime/JSScope.cpp:231 > + JSScope* globalScopeExtension = scope->globalObject(vm)->globalScopeExtension(); Do you know what this is? We should have a test for this if it's easily doable.

*** Bug 167328 has been marked as a duplicate of this bug. ***

Created attachment 299743 [details] Patch Fix review comments

Created attachment 299744 [details] Patch Fix review comments #1

Comment on attachment 299744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299744&action=review > Source/JavaScriptCore/ChangeLog:19 > + op_is_scope_var_type - check if scope with variable is function scope Let's call this op_is_var_scope. I don't understand your explanation "check if scope with variable is function scope", but since op_resolve_closest_or_var_scope names something a "var scope", I'm inferring that this check applies to the same thing. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:697 > + RegisterID* emitResolveOuterVarScope(RegisterID* dst, const Identifier&); It is bad that this function name does not match the name of the bytecode it emits. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:698 > + RegisterID* emitCheckIfScopeIsVarType(RegisterID* dst, RegisterID* scope); Same comment here. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:699 > + RegisterID* emitPutToOuterScope(RegisterID* scope, const Identifier&, RegisterID* value, ResolveMode, InitializationMode); How is emitPutToOuterScope different from emitPutToScope? > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:814 > +SLOW_PATH_DECL(slow_path_resolve_closest_var_scope) You forgot the word "or" in this name, which changes its meaning significantly. > Source/JavaScriptCore/runtime/JSScope.cpp:220 > +ALWAYS_INLINE JSObject* JSScope::baseResolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor finishResolveEarlier) There's no need to use a different function name here. You can call this resolve. > Source/JavaScriptCore/runtime/JSScope.cpp:256 > + if (finishResolveEarlier(scope)) I would call this 'accept' or 'predicate'.

Created attachment 299955 [details] Patch Fix latest comments

Comment on attachment 299744 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299744&action=review Thanks for review!!! >> Source/JavaScriptCore/ChangeLog:19 >> + op_is_scope_var_type - check if scope with variable is function scope > > Let's call this op_is_var_scope. > > I don't understand your explanation "check if scope with variable is function scope", but since op_resolve_closest_or_var_scope names something a "var scope", I'm inferring that this check applies to the same thing. I've tried to make it more clear in new patch >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:697 >> + RegisterID* emitResolveOuterVarScope(RegisterID* dst, const Identifier&); > > It is bad that this function name does not match the name of the bytecode it emits. Fixed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:698 >> + RegisterID* emitCheckIfScopeIsVarType(RegisterID* dst, RegisterID* scope); > > Same comment here. Fixed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:699 >> + RegisterID* emitPutToOuterScope(RegisterID* scope, const Identifier&, RegisterID* value, ResolveMode, InitializationMode); > > How is emitPutToOuterScope different from emitPutToScope? Yeah, now it is the same. Removed >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:814 >> +SLOW_PATH_DECL(slow_path_resolve_closest_var_scope) > > You forgot the word "or" in this name, which changes its meaning significantly. Fixed >> Source/JavaScriptCore/runtime/JSScope.cpp:220 >> +ALWAYS_INLINE JSObject* JSScope::baseResolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor finishResolveEarlier) > > There's no need to use a different function name here. You can call this resolve. Fixed >> Source/JavaScriptCore/runtime/JSScope.cpp:256 >> + if (finishResolveEarlier(scope)) > > I would call this 'accept' or 'predicate'. Renamed

Created attachment 300628 [details] Patch Fix build

Created attachment 300630 [details] Patch Fix build

Created attachment 300632 [details] Patch Add fix me comment

Comment on attachment 300632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300632&action=review r=me I have a few minor comments, and I have one comment for a bug that looks real. Otherwise, this patch looks good to go to me. > Source/JavaScriptCore/ChangeLog:15 > + If eval is invoke ind block scope that contains let/const typo: "invoke ind" => "invoked in" > Source/JavaScriptCore/ChangeLog:20 > + 'hoist function in slommy mode' feature, with small changes. typo: "slommy" => "sloppy" > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2825 > + // we need add optimization for op_resolve_closest_or_var_scope and op_is_var_scope if it possible Nit: Lets write this as: "We need to add optimizations for op_resolve_closest_or_var_scope and op_is_var_scope to do link time scope resolution." > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1843 > + case IsVarScope: This should be KnownCellUse on its child1(). > Source/JavaScriptCore/dfg/DFGOperations.h:197 > +size_t JIT_OPERATION operationIsVarScope(ExecState*, JSScope*); This looks wrong to me. operationResolveClosestOrVarScope may return to you any arbitrary JSObject because of `with` scopes. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9159 > + JSValueOperand scope(this, node->child1()); And then this will just be SpeculateCellOperand > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9168 > +#if USE(JSVALUE64) > + GPRReg scopeGPR = scope.gpr(); > + flushRegisters(); > + callOperation(operationIsVarScope, resultGPR, scopeGPR); > +#else > + JSValueRegs scopeRegs = scope.jsValueRegs(); > + flushRegisters(); > + callOperation(operationIsVarScope, resultGPR, scopeRegs.payloadGPR()); > +#endif And you won't need different code for 32 bit and 64 bit, since you'll just be getting a pointer-width value. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1660 > + JITCompiler::Call callOperation(S_JITOperation_EJsc operation, GPRReg result, GPRReg arg1) > + { > + m_jit.setupArgumentsWithExecState(arg1); > + return appendCallSetResult(operation, result); > + } Nit: You don't need both a 32-bit and 64-bit version of this function since they're identical. You can just move it outside the compile time conditional. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9868 > + setBoolean(vmCall(pointerType(), m_out.operation(operationIsVarScope), m_callFrame, lowCell(m_node->child1()))); You use lowCell, which is why you need to make sure that this is KnownCellUse. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:829 > + JSScope* scope = exec->uncheckedR(pc[2].u.operand).Register::scope(); Same comment as in DFG code: This looks wrong to me. op_resolve_closest_or_var_scope may return to you any arbitrary JSObject because of `with` scopes. Can you please make sure to have a test for this? It should probably be an insta-crash.

Created attachment 301063 [details] Patch Fix comments

Comment on attachment 301063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301063&action=review > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:833 > + if (JSScope* scope = jsDynamicCast<JSScope*>(exec->vm(), object)) { You already have a VM available here, no need to exec->vm(). > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:837 > + result = ((JSCell*)object)->isObject(); This should always be true. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:839 > + result = ((JSCell*)object)->isObject(); This should always be true. If it resolved to a with scope, with scopes can only be over objects, I think. It's worth verifying I'm correct. I suspect `with(foo) { ... }` will always toObject the `foo`.

Comment on attachment 301063 [details] Patch Attachment 301063 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3051925 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html

Created attachment 301103 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 301136 [details] Patch Fix last comments

Created attachment 301145 [details] Patch Improve code readability

Created attachment 301171 [details] Patch Remove unnecessary variable declaration

Created attachment 301177 [details] Patch Rebase

Comment on attachment 301177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301177&action=review I think your code may be wrong for `with` scope. Consider this example: ``` function foo() { let o = {f: 20}; with (o) { eval("{ function f(){} }"); } assert(o.f === 20); } foo() ``` I think your code will fail the assertion. Please let me know if I'm wrong and misreading your code. I'm pretty sure the spec says that we should not bind to `f` unless `f` is a var, and in this case, it's not a var. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832 > + bool result = true; I think this should start as false.

Created attachment 301300 [details] Patch Fix with case

Comment on attachment 301300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301300&action=review I'm leaving as r? given my question about with scopes. > Source/JavaScriptCore/ChangeLog:24 > + In short it works in following way: during hoisting of function > + with name S in eval, we are looking for first scope that > + contains space for variable with name S and if this scope > + has var type we bind function there This code I believe is slightly wrong w.r.t the specification. The behavior we want is not to always create the binding and then to conditionally bind, but rather, to conditionally create the binding, and to bind to it unconditionally if we created it. For example, I checked out your code, and this function won't throw an exception. It'll return undefined. I believe the desired behavior is to throw a ReferenceError: ``` function foo() { { let f; eval("{ function f() { return 20; } }"); } return f; } foo(); ``` That said, I think this patch still does the majority of the work to implementing what I said above. To get the behavior I said above, I think all you'll need to do is change code inside Interpreter.cpp when running eval code. I think it's best at this point to do this fix as a follow up. > Source/JavaScriptCore/runtime/JSScope.cpp:248 > + if (UNLIKELY(doNotCountWithScope) && scope->isWithScope()) > + continue; What does the spec say here? Does it say to skip over `with`? Or does it say to iterate the scope chain during `eval` instantiation. This is important because the hasProperty below is observable for `with` scopes. Also, thinking about this patch more, I think the resolve_closest_or_var and is_var_scope_type could be fully answerable during bytecode linking if the spec behavior is to skip over `with` scopes. So we need to find out if we indeed skip `with` scopes, or we somehow interact with them when creating the eval.

If you run this program in Chrome Canary, you can see V8's behavior here w.r.t the specification. It looks like the eval does indeed perform `has` on the thing inside the with scope. ``` function foo() { let p2 = new Proxy({}, { has(t, p) { console.log(p) return Reflect.has(t, p); } }); with (p2) { eval("Reflect.has(p2, 'bar'); { function baz() { }; }"); } } foo(); ``` I'm not sure if V8 is correct here, but we should verify the correct behavior and try to get that landed. V8 will have the following output: ``` eval Reflect p2 bar baz ```

Firefox nightly outputs: ``` eval eval Reflect p2 bar ``` Which looks pretty obviously wrong w.r.t looking up `eval` twice. But they don't look up `baz` at all. So perhaps the spec says to skip `with` scopes here.

Comment on attachment 301300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301300&action=review >> Source/JavaScriptCore/ChangeLog:24 >> + has var type we bind function there > > This code I believe is slightly wrong w.r.t the specification. > The behavior we want is not to always create the binding and then to conditionally bind, but rather, to conditionally create the binding, and to bind to it unconditionally if we created it. > > For example, I checked out your code, and this function won't throw an exception. It'll return undefined. I believe the desired behavior is to throw a ReferenceError: > > ``` > function foo() { > { > let f; > eval("{ function f() { return 20; } }"); > } > return f; > } > foo(); > ``` > > That said, I think this patch still does the majority of the work to implementing what I said above. To get the behavior I said above, I think all you'll need to do is change code inside Interpreter.cpp when running eval code. > I think it's best at this point to do this fix as a follow up. Yeah, my bad, I see my patch is only focus to conditionally bind. >> Source/JavaScriptCore/runtime/JSScope.cpp:248 >> + continue; > > What does the spec say here? Does it say to skip over `with`? Or does it say to iterate the scope chain during `eval` instantiation. > This is important because the hasProperty below is observable for `with` scopes. > > Also, thinking about this patch more, I think the resolve_closest_or_var and is_var_scope_type could be fully answerable during bytecode linking if the spec behavior is to skip over `with` scopes. > So we need to find out if we indeed skip `with` scopes, or we somehow interact with them when creating the eval. According to spec B.3.3.3.1.d.ii.4 we do loop over EnvironmentRecord, and only B.3.3.3.1.d.ii.4.b we do check 'If thisEnvRec is not an _object_ Environment Record' and then in B.3.3.3.1.d.ii.4.b.i we do check hasBinding for found object. So how I can understand we need to skip 'with' scope

Created attachment 301902 [details] Patch New approach

Created attachment 301950 [details] Patch Fix build

Created attachment 301960 [details] Patch Fix 32bit

Created attachment 302075 [details] Patch Add conditional creation of binding

Created attachment 302079 [details] Patch Add conditional creation of binding and add new test

Comment on attachment 302079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302079&action=review Some comments below. Mostly looks good, but I think there might be a few bugs. > Source/JavaScriptCore/ChangeLog:18 > + we do not bind. In simple patch should lead to following > + behavior: "In simple patch should lead to following behavior" => "This patch leads to the following behavior" > Source/JavaScriptCore/ChangeLog:29 > + boo(); foo > Source/JavaScriptCore/ChangeLog:51 > + you need a call here to match the other ones. Or you can just remove the calls from your example, I think it's clear what you mean without them. > Source/JavaScriptCore/ChangeLog:59 > + function bas() { > + { > + let boo = 10; > + eval(' { function boo() {} } '); > + print(boo); // 10 > + } > + print(boo); //Reference Error > + } Can you open a bug to fix this issue: (This exists both for eval and normal function hoisting): eval(" { let foo; {function foo(){} } }"); > Source/JavaScriptCore/ChangeLog:72 > + or return undefined if variable can be binded there. can => can't > Source/JavaScriptCore/ChangeLog:75 > + that is not covered by this path, and will be fixed in Typo: path => patch > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:767 > + for (FunctionMetadataNode* function : evalNode->functionStack()) > + m_codeBlock->addFunctionDecl(makeFunction(function)); Doesn't this still cause us to unconditionally create the binding? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:770 > + // unsigned numVariables = varDeclarations.size(); Please remove. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:779 > + if (!entry.value.isFunction()) > + variables.append(Identifier::fromUid(m_vm, entry.key.get())); > + else > + hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get())); This looks wrong to me and looks like it'd cause weird behavior for strict mode. I think you want to check if it's a hoisting candidate instead. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1076 > + if ((numVariables || numFunctions || numHoistedFunctions) && eval->isStrictMode()) { I don't think you want this number to be > 0 in strict mode. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:827 > +SLOW_PATH_DECL(slow_path_is_var_scope) Please remove. > Source/JavaScriptCore/runtime/JSScope.cpp:220 > +ALWAYS_INLINE JSObject* JSScope::resolve(ExecState* exec, JSScope* scope, const Identifier& ident, FinishResolveEarlierFunctor predicate, bool doNotCountWithScope) Nit: I'd just make doNoutCountWithScope a templatized variable.

Created attachment 302546 [details] Patch Fixed comments

Comment on attachment 302546 [details] Patch Attachment 302546 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3180089 New failing tests: fast/dom/timer-throttling-hidden-page-non-nested.html

Created attachment 302561 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 302546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302546&action=review Mostly LGTM. Just a couple of questions and a recommendation. > Source/JavaScriptCore/bytecode/BytecodeList.json:156 > + { "name" : "op_resolve_scope_for_binding_func_decl_in_eval", "length" : 4 } Can we call this: "op_resolve_scope_for_hoisting_func_decl_in_eval" I think having the word "hoisting" in here will help everyone better understand what this opcode is doing. (Also, can you change the various C++ functions that have "binding" in the name to "hoisting" or "hoist") > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1063 > + case ResolveScopeForBindingFuncDeclInEval: > + compileResolveScopeForBindingFuncDeclInEval(); > + break; You need to say that the FTL supports this opcode inside FTLCapabilities.cpp. Please verify that this gets compiled in some of your tests, or add new tests that make it to the FTL. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1148 > + for (int i = 0; i < numFunctions; ++i) { Why are there both "numFunctions" and "numHoistedFunctions". Isn't every function declaration in an eval a hoisting candidate?

Created attachment 304941 [details] Patch Fix comments

Comment on attachment 302546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302546&action=review >> Source/JavaScriptCore/bytecode/BytecodeList.json:156 >> + { "name" : "op_resolve_scope_for_binding_func_decl_in_eval", "length" : 4 } > > Can we call this: > "op_resolve_scope_for_hoisting_func_decl_in_eval" > I think having the word "hoisting" in here will help everyone better understand what this opcode is doing. > (Also, can you change the various C++ functions that have "binding" in the name to "hoisting" or "hoist") Fixed >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1063 >> + break; > > You need to say that the FTL supports this opcode inside FTLCapabilities.cpp. > Please verify that this gets compiled in some of your tests, or add new tests that make it to the FTL. Fixed I added RELEASE_ASSERT_.. and receive following tests failed: stress/eval-func-decl-block-with-remove.js.ftl-eager stress/eval-func-decl-block-with-remove.js.ftl-eager-no-cjit stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit stress/eval-func-decl-in-eval-within-with-scope.js.default stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-inline-validate stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-put-stack-validate stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-validate-sampling-profiler stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit stress/eval-func-decl-within-eval-without-reassign-to-let.js.ftl-eager stress/eval-func-decl-within-eval-without-reassign-to-let.js.ftl-eager-no-cjit So I think it covered by Tests >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1148 >> + for (int i = 0; i < numFunctions; ++i) { > > Why are there both "numFunctions" and "numHoistedFunctions". Isn't every function declaration in an eval a hoisting candidate? We have topLevel function that is list of the declared functions, and list of the function names from codeBlockes, that later will be bounded to function. I've renamed to correspond this difference. Could you please look if it more clear?

Comment on attachment 304941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304941&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:1095 > + int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls(); > + unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls(); These names are not better IMO. I still don't understand what the difference here is. Can you explain this to me with an example?

(In reply to Saam Barati from comment #61) > Comment on attachment 304941 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304941&action=review > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1095 > > + int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls(); > > + unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls(); > > These names are not better IMO. > > I still don't understand what the difference here is. Can you explain this > to me with an example? As I remember spec, top level function in eval might lead to error of redeclaration. ``` function bar() { { let f = 20; let value = 10; eval("function f() { value = 20; }; f();"); print(value); } } bar(); // SyntaxError: Can't create duplicate variable in eval: 'f' ``` but function declared in code block not lead to error ``` function baz() { { let f = 20; let value = 10; eval("{ function f() { value = 20; }; f();}"); print(value); } } baz(); // 20 ``` So we need to differentiate them.

(In reply to Saam Barati from comment #46) > If you run this program in Chrome Canary, you can see V8's behavior here > w.r.t the specification. It looks like the eval does indeed perform `has` on > the thing inside the with scope. > > ``` > function foo() { > let p2 = new Proxy({}, { > has(t, p) { > console.log(p) > return Reflect.has(t, p); > } > }); > with (p2) { > eval("Reflect.has(p2, 'bar'); { function baz() { }; }"); > } > } > foo(); > ``` > > I'm not sure if V8 is correct here, but we should verify the correct > behavior and try to get that landed. V8 will have the following output: > > ``` > eval > Reflect > p2 > bar > baz > ``` I believe this is correct, per https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n and https://tc39.github.io/ecma262/#sec-object-environment-records-getbindingvalue-n-s

Comment on attachment 304941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304941&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094 > + int numTopLevelFunctionDecls = eval->numTopLevelFunctionDecls(); I know this is how the code was before, but let's make this function return an unsigned integer. It's dumb that this is a signed int. >>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1095 >>> + unsigned numCodeBlockFunctionDecls = eval->numCodeBlockFunctionDecls(); >> >> These names are not better IMO. >> >> I still don't understand what the difference here is. Can you explain this to me with an example? > > As I remember spec, top level function in eval might lead to error of redeclaration. > ``` > function bar() { > { > let f = 20; > let value = 10; > eval("function f() { value = 20; }; f();"); > print(value); > } > } > bar(); // SyntaxError: Can't create duplicate variable in eval: 'f' > ``` > > but function declared in code block not lead to error > ``` > function baz() { > { > let f = 20; > let value = 10; > eval("{ function f() { value = 20; }; f();}"); > print(value); > } > } > baz(); // 20 > ``` > So we need to differentiate them. Ok. I understand what's going on now. I like the name top level function. For the "CodeBlockFunctions", I'll propose a different name: "functionHoistingCandidates" or something similar. This is more in line with our terminology elsewhere.

(In reply to Caitlin Potter (:caitp) from comment #63) > (In reply to Saam Barati from comment #46) > > If you run this program in Chrome Canary, you can see V8's behavior here > > w.r.t the specification. It looks like the eval does indeed perform `has` on > > the thing inside the with scope. > > > > ``` > > function foo() { > > let p2 = new Proxy({}, { > > has(t, p) { > > console.log(p) > > return Reflect.has(t, p); > > } > > }); > > with (p2) { > > eval("Reflect.has(p2, 'bar'); { function baz() { }; }"); > > } > > } > > foo(); > > ``` > > > > I'm not sure if V8 is correct here, but we should verify the correct > > behavior and try to get that landed. V8 will have the following output: > > > > ``` > > eval > > Reflect > > p2 > > bar > > baz > > ``` > > I believe this is correct, per > https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n > and > https://tc39.github.io/ecma262/#sec-object-environment-records- > getbindingvalue-n-s Hmm, I stil think that it should not be printed 'baz', because according to the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record, then') we don't invoke hasBinding, and just go to outer env recode https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation

(In reply to GSkachkov from comment #65) > (In reply to Caitlin Potter (:caitp) from comment #63) > > (In reply to Saam Barati from comment #46) > > > If you run this program in Chrome Canary, you can see V8's behavior here > > > w.r.t the specification. It looks like the eval does indeed perform `has` on > > > the thing inside the with scope. > > > > > > ``` > > > function foo() { > > > let p2 = new Proxy({}, { > > > has(t, p) { > > > console.log(p) > > > return Reflect.has(t, p); > > > } > > > }); > > > with (p2) { > > > eval("Reflect.has(p2, 'bar'); { function baz() { }; }"); > > > } > > > } > > > foo(); > > > ``` > > > > > > I'm not sure if V8 is correct here, but we should verify the correct > > > behavior and try to get that landed. V8 will have the following output: > > > > > > ``` > > > eval > > > Reflect > > > p2 > > > bar > > > baz > > > ``` > > > > I believe this is correct, per > > https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n > > and > > https://tc39.github.io/ecma262/#sec-object-environment-records- > > getbindingvalue-n-s > > Hmm, I stil think that it should not be printed 'baz', because according to > the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record, > then') we don't invoke hasBinding, and just go to outer env recode > https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation As I said on IRC, I wasn't commenting on the hoisting of baz, because the behaviour of block scoping of functions and annex B changes to that are somewhat hard to follow. It looks like you're right about that HasProperty() not occurring from step 5, but it may occur further down in the algorithm, from `Let fobj be ! benvRec.GetBindingValue(F, false).`, as I believe in this instance, benv still refers to the with block. I could be wrong about this, Adam Klein is much more knowledgable about the block-scoping of functions than I am.

Created attachment 306399 [details] Patch Patch with fixes

(In reply to Caitlin Potter (:caitp) from comment #66) > (In reply to GSkachkov from comment #65) > > (In reply to Caitlin Potter (:caitp) from comment #63) > > > (In reply to Saam Barati from comment #46) > > > > If you run this program in Chrome Canary, you can see V8's behavior here > > > > w.r.t the specification. It looks like the eval does indeed perform `has` on > > > > the thing inside the with scope. > > > > > > > > ``` > > > > function foo() { > > > > let p2 = new Proxy({}, { > > > > has(t, p) { > > > > console.log(p) > > > > return Reflect.has(t, p); > > > > } > > > > }); > > > > with (p2) { > > > > eval("Reflect.has(p2, 'bar'); { function baz() { }; }"); > > > > } > > > > } > > > > foo(); > > > > ``` > > > > > > > > I'm not sure if V8 is correct here, but we should verify the correct > > > > behavior and try to get that landed. V8 will have the following output: > > > > > > > > ``` > > > > eval > > > > Reflect > > > > p2 > > > > bar > > > > baz > > > > ``` > > > > > > I believe this is correct, per > > > https://tc39.github.io/ecma262/#sec-object-environment-records-hasbinding-n > > > and > > > https://tc39.github.io/ecma262/#sec-object-environment-records- > > > getbindingvalue-n-s > > > > Hmm, I stil think that it should not be printed 'baz', because according to > > the p.B.3.3.3.1.d.ii.4.b('If thisEnvRec is not an object Environment Record, > > then') we don't invoke hasBinding, and just go to outer env recode > > https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation > > As I said on IRC, I wasn't commenting on the hoisting of baz, because the > behaviour of block scoping of functions and annex B changes to that are > somewhat hard to follow. > > It looks like you're right about that HasProperty() not occurring from step > 5, but it may occur further down in the algorithm, from `Let fobj be ! > benvRec.GetBindingValue(F, false).`, as I believe in this instance, benv > still refers to the with block. > > I could be wrong about this, Adam Klein is much more knowledgable about the > block-scoping of functions than I am. Oh sorry, I misunderstood your message in IRC. I'll try to chat with Adam Klein.

Comment on attachment 306399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306399&action=review Almost everything LGTM, just 1 fundamental question below: > Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h:60 > + void adoptFunctionHoistingCandidates(Vector<Identifier, 0, UnsafeVectorOverflow>& functionHoistingCandidates) > + { > + ASSERT(m_functionHoistingCandidates.isEmpty()); > + m_functionHoistingCandidates.swap(functionHoistingCandidates); > + } Style nit: You can use C++ move semantics here and do something like: adopt(Vector<T>&& v) { ASSERT(m_v.isEmpty()); m_v = WTFMove(v); } > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:771 > + if (!entry.value.isSloppyModeHoistingCandidate()) > + variables.append(Identifier::fromUid(m_vm, entry.key.get())); > + else > + hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get())); Style nit: I think this is easier to read if you flip the if and else and remove the "!" on the if's condition. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2225 > + ASSERT_UNUSED(checkResult.get(), checkResult.get() != nullptr); no need for ASSERT_UNUSED here, can just be a regular ASSERT. ASSERT_UNUSED is only for variables, not temp expressions > Source/JavaScriptCore/parser/Parser.h:1201 > + bool isSloppyModeHoistingCandidate = !strictMode() && closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext(); This can only be true if m_statementDepth == 1, however, isn't this just a top level function if m_statementDepth == 1? Why is this needed here then?

Created attachment 307127 [details] Patch Fix latest comments

Comment on attachment 306399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306399&action=review >> Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h:60 >> + } > > Style nit: You can use C++ move semantics here and do something like: > adopt(Vector<T>&& v) { ASSERT(m_v.isEmpty()); m_v = WTFMove(v); } Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:771 >> + hoistedFunctions.append(Identifier::fromUid(m_vm, entry.key.get())); > > Style nit: > I think this is easier to read if you flip the if and else and remove the "!" on the if's condition. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2225 >> + ASSERT_UNUSED(checkResult.get(), checkResult.get() != nullptr); > > no need for ASSERT_UNUSED here, can just be a regular ASSERT. > ASSERT_UNUSED is only for variables, not temp expressions I've just remove ASSERT, not sure if it necessary at this place >> Source/JavaScriptCore/parser/Parser.h:1201 >> + bool isSloppyModeHoistingCandidate = !strictMode() && closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext(); > > This can only be true if m_statementDepth == 1, however, isn't this just a top level function if m_statementDepth == 1? Why is this needed here then? Ohhh, you are right.I think it should be just false

Comment on attachment 307127 [details] Patch Attachment 307127 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3535219 New failing tests: webrtc/multi-video.html

Created attachment 307138 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 307127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307127&action=review Nice! r=me > Source/JavaScriptCore/parser/Parser.cpp:1784 > + if (!currentScope()->isFunction() && !closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext()) { Please update the comment below, since you're now adding this for eval.

Committed r215476: <http://trac.webkit.org/changeset/215476>

Patch landed

Comment on attachment 307127 [details] Patch Patch landed, clear flags

This appears to have caused 1 new Test262 failure: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20Test262%20%28Tests%29/builds/211 > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: Exception: SyntaxError: Can't create duplicate variable in eval: 'err' > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: at var-env-lower-lex-catch-non-strict.js:28 > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: eval@[native code] > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: global code@var-env-lower-lex-catch-non-strict.js:28:7 > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: ERROR: Unexpected exit code: 3 > FAIL: test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default Note we are going to make the bot correctly turn red on failures with: https://bugs.webkit.org/show_bug.cgi?id=171044

(In reply to Joseph Pecoraro from comment #78) > This appears to have caused 1 new Test262 failure: > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Debug%20Test262%20%28Tests%29/builds/211 > > > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: Exception: SyntaxError: Can't create duplicate variable in eval: 'err' > > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: at var-env-lower-lex-catch-non-strict.js:28 > > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: eval@[native code] > > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: global code@var-env-lower-lex-catch-non-strict.js:28:7 > > test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default: ERROR: Unexpected exit code: 3 > > FAIL: test262.yaml/test262/test/annexB/language/eval-code/direct/var-env-lower-lex-catch-non-strict.js.default > > Note we are going to make the bot correctly turn red on failures with: > https://bugs.webkit.org/show_bug.cgi?id=171044 Ohh, I'll take look to it in 171055. Before I created separated issue 168184 for case that is covered by this test.

Was there a plan for this to fix this test: ``` eval("'use strict'; const x = 10; function f() {console.log(x);} f()") ``` This still throws a ReferenceError.

(In reply to Saam Barati from comment #80) > Was there a plan for this to fix this test: > > ``` > eval("'use strict'; const x = 10; function f() {console.log(x);} f()") > ``` > This still throws a ReferenceError. Yeah, we started from this issue, but later I removed code that fix exact case, because it doesn't fix more complex cases. I think I'll switch to this issue 161099

Re-opened since this is blocked by bug 171304

This patch broke JSBench. You can run JSBench locally by going to: http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html With this change applied, the benchmark never gets past stage 1. It seems like other browsers implement this behavior, but also work in JSBench. So perhaps it's a bug in our implementation? I haven't looked into it besides bisecting the change that broke JSBench. I guess it's possible that it's also some kind of JIT bug.

(In reply to Saam Barati from comment #83) > This patch broke JSBench. > You can run JSBench locally by going to: > http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html > > With this change applied, the benchmark never gets past stage 1. > > It seems like other browsers implement this behavior, but also work in > JSBench. So perhaps it's a bug in our implementation? I haven't looked into > it besides bisecting the change that broke JSBench. I guess it's possible > that it's also some kind of JIT bug. I'm looking to it. I see error in console "ReferenceError: Can't find variable: adx_sh_291332" I'll check why it is happens.

(In reply to Saam Barati from comment #83) > This patch broke JSBench. > You can run JSBench locally by going to: > http://jsbench.cs.purdue.edu/suite/jsbench-2013.1/harness.html > > With this change applied, the benchmark never gets past stage 1. > > It seems like other browsers implement this behavior, but also work in > JSBench. So perhaps it's a bug in our implementation? I haven't looked into > it besides bisecting the change that broke JSBench. I guess it's possible > that it's also some kind of JIT bug. I managed to find root of the issue, and it was already existed issue I created separate bug 171456. Current patch just make this bug visible in JSBench. Will prepare rebased patch after fixing 171456

Created attachment 308677 [details] Patch Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric. This test will be fixed in separate issue

(In reply to GSkachkov from comment #86) > Created attachment 308677 [details] > Patch > > Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric. > This test will be fixed in separate issue No need to put this back up for review since you didn't change the patch and you landed the fix to the preexisting bug separately. You can just reland this change.

(In reply to Saam Barati from comment #87) > (In reply to GSkachkov from comment #86) > > Created attachment 308677 [details] > > Patch > > > > Rebased patch with suppresed error in var-env-lower-lex-catch-non-stric. > > This test will be fixed in separate issue > > No need to put this back up for review since you didn't change the patch and > you landed the fix to the preexisting bug separately. You can just reland > this change. 👌 There is result of the JSBench on my Mac: JSBench Harness Final results:   13.72ms ± 9.39% (lower is better)   Standard deviation = 20.09% of mean   Standard error = 4.49% of mean   23 runs

Landed https://trac.webkit.org/r215984.

Comment on attachment 308677 [details] Patch Clear flags

Comment on attachment 304941 [details] Patch Unflagging this old onbsoleted patch on a resolved bug.