Created attachment 262990 [details] Patch

Comment on attachment 262990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262990&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014 > + generator.pushLexicalScope(this, true); section 14.5.14, step 4, 6-a, 11. http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3084 > + generator.emitPutToScope(scope.get(), classNameVar, constructor.get(), ThrowIfNotFound, Initialization); section 14.5.14, step 23. http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation

Comment on attachment 262990 [details] Patch r=me

Comment on attachment 262990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262990&action=review LGTM >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014 >> + generator.pushLexicalScope(this, true); > > section 14.5.14, step 4, 6-a, 11. > > http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation You could store the constructor into the scope earlier, once the constructor is generated. That way, all the method/static method properties don't get compile with the class name being under TDZ. You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably not an obvious way to do this that doesn't clutter the code.

(In reply to comment #4) > Comment on attachment 262990 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262990&action=review > > LGTM > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014 > >> + generator.pushLexicalScope(this, true); > > > > section 14.5.14, step 4, 6-a, 11. > > > > http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation > > You could store the constructor into the scope earlier, once the constructor > is generated. > That way, all the method/static method properties don't get compile with the > class name being under TDZ. > > You could also get more fancy and eliminate the TDZ check in the > constructor, but there is probably > not an obvious way to do this that doesn't clutter the code. Actually, there is an easy way to have nothing be under TDZ. You could call BytecodeGenerator::pushLexicalScopeInternal instead of pushLexicalScope. It has more customizability. If you do this, though, you should probably rename pushLexicalScopeInternal because it will no longer be internal to BytecodeGenerator.

Comment on attachment 262990 [details] Patch Attachment 262990 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/280047 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-with-mutations.html inspector/css/stylesheet-events-inspector-stylesheet.html inspector/unit-tests/event-listener-set.html inspector/css/stylesheet-events-basic.html inspector/console/messagesCleared.html inspector/console/clearMessages.html inspector/console/command-line-api.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/console/messageRepeatCountUpdated.html inspector/css/stylesheet-events-multiple-documents.html inspector/unit-tests/event-listener.html

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

Comment on attachment 262990 [details] Patch Attachment 262990 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/280049 New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-with-mutations.html inspector/css/stylesheet-events-inspector-stylesheet.html inspector/unit-tests/event-listener-set.html inspector/css/stylesheet-events-basic.html inspector/console/messagesCleared.html inspector/console/clearMessages.html inspector/console/command-line-api.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/console/messageRepeatCountUpdated.html inspector/css/stylesheet-events-multiple-documents.html inspector/unit-tests/event-listener.html

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

Comment on attachment 262990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262990&action=review >>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014 >>>> + generator.pushLexicalScope(this, true); >>> >>> section 14.5.14, step 4, 6-a, 11. >>> >>> http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation >> >> You could store the constructor into the scope earlier, once the constructor is generated. >> That way, all the method/static method properties don't get compile with the class name being under TDZ. >> >> You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably >> not an obvious way to do this that doesn't clutter the code. > > Actually, there is an easy way to have nothing be under TDZ. > You could call BytecodeGenerator::pushLexicalScopeInternal instead of > pushLexicalScope. It has more customizability. If you do this, though, > you should probably rename pushLexicalScopeInternal because it will > no longer be internal to BytecodeGenerator. Nice catch. I think moving initialization part just before the following can solve the problem :) if (m_staticMethods) generator.emitNode(constructor.get(), m_staticMethods); And we should keep TDZ in superClass phase. I've just added the test to cover it. class A extends A { }; => "ReferenceError: Cannot access uninitialized variable." According to the step 6-b, ClassHeritage is executed under this class scope. But at that time, the variable is not initialized yet (TDZ).

The errors raised in EWS; WebInspector JS code has the following, WebInspector.Object = class Object { static method() { Object.keys(...) } } Object inside method is shadowed by the user-defined Object (not global Object constructor). So there is no Object.keys. Before this patch, we don't define the variable Object anywhere because this is the class expression. But after this patch, it correctly defines class scope with `Object` variable and it makes the current code an error.

(In reply to comment #10) > Comment on attachment 262990 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262990&action=review > > >>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014 > >>>> + generator.pushLexicalScope(this, true); > >>> > >>> section 14.5.14, step 4, 6-a, 11. > >>> > >>> http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation > >> > >> You could store the constructor into the scope earlier, once the constructor is generated. > >> That way, all the method/static method properties don't get compile with the class name being under TDZ. > >> > >> You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably > >> not an obvious way to do this that doesn't clutter the code. > > > > Actually, there is an easy way to have nothing be under TDZ. > > You could call BytecodeGenerator::pushLexicalScopeInternal instead of > > pushLexicalScope. It has more customizability. If you do this, though, > > you should probably rename pushLexicalScopeInternal because it will > > no longer be internal to BytecodeGenerator. > > Nice catch. > I think moving initialization part just before the following can solve the > problem :) > > if (m_staticMethods) > generator.emitNode(constructor.get(), m_staticMethods); > > And we should keep TDZ in superClass phase. I've just added the test to > cover it. > > class A extends A { > }; > => "ReferenceError: Cannot access uninitialized variable." > > According to the step 6-b, ClassHeritage is executed under this class scope. > But at that time, the variable is not initialized yet (TDZ). Nice. I forgot about this.

https://bugs.webkit.org/show_bug.cgi?id=150093 After this is fixed, I'll land the fixed patch.

Ah, oops. There is another edge case. (This is included in tests/es6) class C { [C]() { // This should touch TDZ. } } So in the meantime, I'll land the patch as is (with the added tests). And I'll open the bug to optimize the common case.

(In reply to comment #14) > Ah, oops. There is another edge case. (This is included in tests/es6) > > > class C { > [C]() { // This should touch TDZ. > } > } > > So in the meantime, I'll land the patch as is (with the added tests). > And I'll open the bug to optimize the common case. Lol. This is funny. Your plan sounds good to me. Are computed methods the only way we could try to resolve the constructor before we putToScope?

Committed r191030: <http://trac.webkit.org/changeset/191030>

Debug build causes CRASH. (With assertion). Investigating...

OK, figured out. Current Class expression implementation declares method names as a variable. For example, class A { method() { } } In the above case, we declare a variable `method` in the current implementation. I guess this is the old draft behavior because I can't see any step in the current spec. http://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-definemethod As a result, if we evaluate the following, class A { 1() { } } We attempt to declare the variable `1`. We now wrap the class body with the class lexical scope. However, the lexical scope does not allow us to declare incorrect Identifier variable (like `1`). So assertion is fired.

We will fix this in the other bug.

(In reply to comment #19) > We will fix this in the other bug. Handled in https://bugs.webkit.org/show_bug.cgi?id=150115

Re-opened since this is blocked by bug 150116

After https://bugs.webkit.org/show_bug.cgi?id=150115 is landed, I'll attempt to reland.

Committed r191110: <http://trac.webkit.org/changeset/191110>

Since https://bugs.webkit.org/show_bug.cgi?id=150115 is landed, I've landed the roll-outed patch.