Bug 145132 - [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "arguments"
Summary: [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 144955 144956
Blocks: 140855 152575
  Show dependency treegraph
 
Reported: 2015-05-18 10:35 PDT by GSkachkov
Modified: 2016-01-25 21:43 PST (History)
12 users (show)

See Also:


Attachments
Patch (68.47 KB, patch)
2015-08-18 05:28 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (539.68 KB, application/zip)
2015-08-18 06:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (578.07 KB, application/zip)
2015-08-18 06:18 PDT, Build Bot
no flags Details
Patch (68.58 KB, patch)
2015-08-18 07:56 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (545.48 KB, application/zip)
2015-08-18 08:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (617.11 KB, application/zip)
2015-08-18 08:47 PDT, Build Bot
no flags Details
Patch (68.30 KB, patch)
2015-08-18 09:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (536.63 KB, application/zip)
2015-08-18 09:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (574.92 KB, application/zip)
2015-08-18 09:52 PDT, Build Bot
no flags Details
Patch (68.14 KB, patch)
2015-08-18 10:36 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (67.15 KB, patch)
2015-08-18 12:51 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (67.12 KB, patch)
2015-08-19 00:24 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (31.95 KB, patch)
2015-12-30 14:43 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (745.47 KB, application/zip)
2015-12-30 15:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (751.71 KB, application/zip)
2015-12-30 15:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (895.88 KB, application/zip)
2015-12-30 15:50 PST, Build Bot
no flags Details
Patch (32.28 KB, patch)
2016-01-02 02:44 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.00 MB, application/zip)
2016-01-02 03:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-yosemite (986.24 KB, application/zip)
2016-01-02 03:54 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (986.21 KB, application/zip)
2016-01-02 03:57 PST, Build Bot
no flags Details
Patch (32.87 KB, patch)
2016-01-03 11:20 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (955.78 KB, application/zip)
2016-01-03 12:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (717.07 KB, application/zip)
2016-01-03 12:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (964.59 KB, application/zip)
2016-01-03 12:15 PST, Build Bot
no flags Details
Patch (33.99 KB, patch)
2016-01-04 08:30 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (858.19 KB, application/zip)
2016-01-04 09:40 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (879.13 KB, application/zip)
2016-01-04 09:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (863.25 KB, application/zip)
2016-01-04 09:46 PST, Build Bot
no flags Details
Patch (39.44 KB, patch)
2016-01-05 14:33 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (34.18 KB, patch)
2016-01-19 14:10 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (32.17 KB, patch)
2016-01-19 14:32 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (32.71 KB, patch)
2016-01-24 06:41 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (32.74 KB, patch)
2016-01-25 09:14 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (32.73 KB, patch)
2016-01-25 13:24 PST, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-05-18 10:35:39 PDT
It is necessary to add lexical bind of the "arguments", "super", "new.target"
Comment 1 GSkachkov 2015-08-18 05:28:59 PDT
Created attachment 259264 [details]
Patch

init version
Comment 2 Build Bot 2015-08-18 06:13:08 PDT
Comment on attachment 259264 [details]
Patch

Attachment 259264 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/71653

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 3 Build Bot 2015-08-18 06:13:12 PDT
Created attachment 259265 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-08-18 06:18:05 PDT
Comment on attachment 259264 [details]
Patch

Attachment 259264 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/71661

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 5 Build Bot 2015-08-18 06:18:08 PDT
Created attachment 259266 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 GSkachkov 2015-08-18 07:56:27 PDT
Created attachment 259267 [details]
Patch

Fix tests
Comment 7 Build Bot 2015-08-18 08:41:54 PDT
Comment on attachment 259267 [details]
Patch

Attachment 259267 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/72002

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 8 Build Bot 2015-08-18 08:41:57 PDT
Created attachment 259268 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 9 Build Bot 2015-08-18 08:47:10 PDT
Comment on attachment 259267 [details]
Patch

Attachment 259267 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/72012

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 10 Build Bot 2015-08-18 08:47:12 PDT
Created attachment 259269 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 11 GSkachkov 2015-08-18 09:00:22 PDT
Created attachment 259270 [details]
Patch

Fix layout tests
Comment 12 Build Bot 2015-08-18 09:48:43 PDT
Comment on attachment 259270 [details]
Patch

Attachment 259270 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/72159

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 13 Build Bot 2015-08-18 09:48:46 PDT
Created attachment 259274 [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 14 Build Bot 2015-08-18 09:52:31 PDT
Comment on attachment 259270 [details]
Patch

Attachment 259270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/72163

New failing tests:
js/arrowfunction-lexical-bind-arguments.html
Comment 15 Build Bot 2015-08-18 09:52:34 PDT
Created attachment 259275 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 16 GSkachkov 2015-08-18 10:36:20 PDT
Created attachment 259279 [details]
Patch

Fix layout tests
Comment 17 GSkachkov 2015-08-18 12:51:48 PDT
Created attachment 259292 [details]
Patch

Small changes
Comment 18 GSkachkov 2015-08-19 00:24:47 PDT
Created attachment 259366 [details]
Patch

Small improvementes
Comment 19 GSkachkov 2015-08-19 08:19:42 PDT
Comment on attachment 259366 [details]
Patch

Looks like something wrong with build
Comment 20 Brian Burg 2015-08-19 13:23:26 PDT
(In reply to comment #19)
> Comment on attachment 259366 [details]
> Patch
> 
> Looks like something wrong with build

../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*, WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)':
../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal compiler error: Segmentation fault

For GTK & EFL, this is almost certainly the result of the EWS machine running out of disk space, memory, or some other system resource. It's a known sporadic issue. You can ignore it.
Comment 21 GSkachkov 2015-08-19 14:28:31 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 259366 [details]
> > Patch
> > 
> > Looks like something wrong with build
> 
> ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function
> 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*,
> WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)':
> ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal
> compiler error: Segmentation fault
> 
> For GTK & EFL, this is almost certainly the result of the EWS machine
> running out of disk space, memory, or some other system resource. It's a
> known sporadic issue. You can ignore it.

OK. Thanks for info
Comment 22 Saam Barati 2015-08-21 00:26:07 PDT
Comment on attachment 259366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259366&action=review

The overall approach here is good. There needs to be some significant reworking in the bytecode generator
(and maybe the parser) here.
Also, we need many more tests.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:70
> +    if (m_needToInitializeArguments) {

It doesn't seem like you're doing any prevention of allocating the different arguments object while inside
an arrow function. This is bad. We need to prevent those allocations.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:73
> +            initializeVariable(variable(propertyNames().arguments), m_arrowFunctionArguments.get());

You can just use m_argumentsRegister here.
Also, we don't prefix local variables with "m_".

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2305
> +    RegisterID* m_tmpArgumentsRegister = emitGetFromScope(newTemporary(), scope, var, DoNotThrowIfNotFound);

This is pretty shady. We should just be using m_argumentsRegister here.
We shouldn't resolve anything. What this requires is that we now must note
that an arguments object must be created for functions that have arrow functions
nested inside them. We can get smart here, and we should if it's not too difficult, and only allocate these
arguments objects as being needed, i.e it the arrow function uses "arguments" or eval. Maybe a good first take before
optimizing is just report that any function with an arrow function nested inside of it needs to
allocate an arguments object (unless the parent function is an arrow function itself, in which
case it should get its arguments object through op_get_arrowfunction_arguments).

Once you get this working, you can optimize cases like this:
"function foo() { var x = () => 20; }". Obviously foo doesn't need an arguments object
"function foo() { var x = (p) => eval(p); }". foo does need arguments object
"function foo() { var x = () => { var x = () => arguments; } }". foo does need an arguments object if there is a chain from a non-arrow-function through only arrow functions that use arguments or eval.
"function foo() { var x = () => { var x = () { function bar() { var x = () => arguments; } } } }". bar function does need arguments. foo doesn't.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-arguments.js:1
> +var testCase = function (actual, expected, message) {

I don't think we're covering enough tests here.
JSC has three different arguments objects it allocates.
We should be writing tests to target each of these.
We also need to ensure different properties of the
arguments objects hold. For example, we shouldn't
be able to overwrite local variables for strict mode arguments
object. 

Does lexical binding of arguments object impose any restrictions on the type
of arguments object? Or does it not matter. If it doesn't matter, we need
tests for sloppy mode argument assignment inside an arrow function
which change "local" arguments parameters.
Comment 23 GSkachkov 2015-12-30 14:43:42 PST
Created attachment 268011 [details]
Patch

Upload new version of the patch
Comment 24 Build Bot 2015-12-30 15:44:55 PST
Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/627910

Number of test failures exceeded the failure limit.
Comment 25 Build Bot 2015-12-30 15:44:58 PST
Created attachment 268017 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 26 Build Bot 2015-12-30 15:46:58 PST
Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/627923

Number of test failures exceeded the failure limit.
Comment 27 Build Bot 2015-12-30 15:47:01 PST
Created attachment 268018 [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 28 Build Bot 2015-12-30 15:50:53 PST
Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/627927

Number of test failures exceeded the failure limit.
Comment 29 Build Bot 2015-12-30 15:50:56 PST
Created attachment 268019 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 30 GSkachkov 2016-01-02 02:44:09 PST
Created attachment 268098 [details]
Patch

Fix tests
Comment 31 Build Bot 2016-01-02 03:53:30 PST
Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/638982

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
inspector/console/messageRepeatCountUpdated.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/console/command-line-api.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/network/client-blocked-load.html
inspector/model/stack-trace.html
Comment 32 Build Bot 2016-01-02 03:53:35 PST
Created attachment 268100 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 33 Build Bot 2016-01-02 03:54:06 PST
Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/638988

New failing tests:
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/console/command-line-api.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
inspector/dom/setOuterHTML.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/model/stack-trace.html
inspector/css/generate-css-rule-string.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/network/client-blocked-load.html
Comment 34 Build Bot 2016-01-02 03:54:11 PST
Created attachment 268101 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 35 Build Bot 2016-01-02 03:57:26 PST
Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/638994

New failing tests:
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/console/command-line-api.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
inspector/dom/setOuterHTML.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/model/stack-trace.html
inspector/css/generate-css-rule-string.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/network/client-blocked-load.html
Comment 36 Build Bot 2016-01-02 03:57:31 PST
Created attachment 268102 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 37 GSkachkov 2016-01-03 11:20:07 PST
Created attachment 268144 [details]
Patch

Fix tests
Comment 38 Build Bot 2016-01-03 12:08:11 PST
Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/644887

Number of test failures exceeded the failure limit.
Comment 39 Build Bot 2016-01-03 12:08:15 PST
Created attachment 268147 [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 40 Build Bot 2016-01-03 12:12:08 PST
Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/644890

Number of test failures exceeded the failure limit.
Comment 41 Build Bot 2016-01-03 12:12:12 PST
Created attachment 268148 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 42 Build Bot 2016-01-03 12:15:32 PST
Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/644899

Number of test failures exceeded the failure limit.
Comment 43 Build Bot 2016-01-03 12:15:37 PST
Created attachment 268149 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 44 GSkachkov 2016-01-04 08:30:17 PST
Created attachment 268192 [details]
Patch

Fix WebKit tests
Comment 45 Build Bot 2016-01-04 09:40:18 PST
Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/648878

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/console/command-line-api.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/network/client-blocked-load.html
Comment 46 Build Bot 2016-01-04 09:40:21 PST
Created attachment 268196 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 47 Build Bot 2016-01-04 09:42:16 PST
Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/648877

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/network/client-blocked-load.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/console/command-line-api.html
Comment 48 Build Bot 2016-01-04 09:42:19 PST
Created attachment 268198 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 49 Build Bot 2016-01-04 09:46:17 PST
Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/648887

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/console/command-line-api.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/network/client-blocked-load.html
Comment 50 Build Bot 2016-01-04 09:46:20 PST
Created attachment 268199 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 51 GSkachkov 2016-01-05 14:33:11 PST
Created attachment 268326 [details]
Patch

Fix WebKit tests#2
Comment 52 Saam Barati 2016-01-10 16:48:20 PST
Comment on attachment 268326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268326&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:83
> +            if (m_codeBlock->isStrictMode())
> +                emitTDZCheck(argumentsRegister);

why would we ever need a TDZ check here?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123
> +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope()

I see what you're going for here in this function, but this seems a bit hacky to me.
I think there might be an alternate solution that might be cleaner:

We currently don't allow functions to declare the variable named "arguments"
as being captured (you can see this in the "collectFreeVariables" function inside Parser.h).
But, in the case of an arrow function, we really do want "arguments" to act like a captured variable.
If we teach the parser we can really capture "arguments", then we could probably do away
with a lot of this specialized code. We wouldn't need this function (because "arguments" would already
be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized
argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it
that we don't have a local "arguments" that we created, but that's about it. I think most of this change
could be a parser change.

You would basically have to teach the parser that any arrow function implicitly captures the name "arguments".
Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the
arrow function.
Comment 53 GSkachkov 2016-01-19 14:10:40 PST
Created attachment 269291 [details]
Patch

Fix comments
Comment 54 GSkachkov 2016-01-19 14:32:05 PST
Created attachment 269298 [details]
Patch

Remove redundant code
Comment 55 GSkachkov 2016-01-19 15:02:01 PST
Comment on attachment 268326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268326&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123
>> +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope()
> 
> I see what you're going for here in this function, but this seems a bit hacky to me.
> I think there might be an alternate solution that might be cleaner:
> 
> We currently don't allow functions to declare the variable named "arguments"
> as being captured (you can see this in the "collectFreeVariables" function inside Parser.h).
> But, in the case of an arrow function, we really do want "arguments" to act like a captured variable.
> If we teach the parser we can really capture "arguments", then we could probably do away
> with a lot of this specialized code. We wouldn't need this function (because "arguments" would already
> be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized
> argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it
> that we don't have a local "arguments" that we created, but that's about it. I think most of this change
> could be a parser change.
> 
> You would basically have to teach the parser that any arrow function implicitly captures the name "arguments".
> Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the
> arrow function.

Fixed. Oh I did not know about this collectFreeVariables. This approach is much better because allow to decrease amount of code
Comment 56 Saam Barati 2016-01-22 16:00:20 PST
Comment on attachment 269298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269298&action=review

r=me with one suggestion and a few comments:
Can you add a test to make sure it works with generators:
function* foo(a, b, c) {
    yield () => arguments;
}
foo(10, 20).value[0] === 10
foo(10, 20).value[1] === 20

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32
> +    // https://bugs.webkit.org/show_bug.cgi?id=152570

why does it lead to a reference error?

> Source/WebInspectorUI/UserInterface/Base/Object.js:63
> -        let wrappedCallback = () => {
> -            this.removeEventListener(eventType, wrappedCallback, null);
> +        let that = this;
> +        let wrappedCallback = function () {
> +            that.removeEventListener(eventType, wrappedCallback, null);

why is this necessary?
Comment 57 GSkachkov 2016-01-24 06:41:15 PST
Created attachment 269693 [details]
Patch

Fix comments
Comment 58 GSkachkov 2016-01-24 09:14:36 PST
Comment on attachment 269298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269298&action=review

Also I've added test of Generator

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32
>> +    // https://bugs.webkit.org/show_bug.cgi?id=152570
> 
> why does it lead to a reference error?

Now I understand that it is wrong place for this test. I've removed it. The idea is that Global scope does not contain variable 'arguments', and access to the 'arguments' from global scope should lead to ReferenceError. Current test is executed in JSC app where arguments is empty array.

>> Source/WebInspectorUI/UserInterface/Base/Object.js:63
>> +            that.removeEventListener(eventType, wrappedCallback, null);
> 
> why is this necessary?

See line below. Without this change, but with current patch - arguments will start contain values of singleFireEventListener instead of values that is passed during wrappedCallback invocation.
Comment 59 Saam Barati 2016-01-24 21:28:43 PST
Comment on attachment 269693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269693&action=review

> Source/WebInspectorUI/UserInterface/Base/Object.js:61
> +        let that = this;

It's inspector style to use .bind(function, this).
Comment 60 GSkachkov 2016-01-25 09:14:12 PST
Created attachment 269753 [details]
Patch

Fix comments
Comment 61 Joseph Pecoraro 2016-01-25 11:45:23 PST
Comment on attachment 269753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269753&action=review

> Source/WebInspectorUI/UserInterface/Base/Object.js:61
> -        let wrappedCallback = () => {
> +        let wrappedCallback = function () {

Style Nit: "function ()" => "function()"

Good find!
Comment 62 GSkachkov 2016-01-25 13:24:49 PST
Created attachment 269784 [details]
Patch

Fix style comments[D
Comment 63 GSkachkov 2016-01-25 14:06:24 PST
Comment on attachment 269753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269753&action=review

>> Source/WebInspectorUI/UserInterface/Base/Object.js:61
>> +        let wrappedCallback = function () {
> 
> Style Nit: "function ()" => "function()"
> 
> Good find!

Fixed
Comment 64 WebKit Commit Bot 2016-01-25 21:43:20 PST
Comment on attachment 269784 [details]
Patch

Clearing flags on attachment: 269784

Committed r195581: <http://trac.webkit.org/changeset/195581>
Comment 65 WebKit Commit Bot 2016-01-25 21:43:29 PST
All reviewed patches have been landed.  Closing bug.