Created attachment 379026 [details] testcase #Testcase: 1 var array = [ 0, 1, 2 ]; 2 var str = "asdadc"; 3 Array.prototype.push.apply(str, array); 4 print(str.length); 5 print(str); #Command: ./webkit/WebKitBuild/Release/bin/jsc testcase.js #Output: 6 asdadc #Expected output: Throw a TypeError in line3. #Description: When executing the above testcase, a TypeError should be thrown according to ECMAScript-262 standard. However, javascriptCore doesn't throw any exception but outputs "6" and "asdadc" while other JS engines such as V8, SpiderMonkey throw a TypeError. According to the output, I guess that JavaScriptCore neither executed Array.prototype.push() function normally nor thrown a TypeError. So I suspect that it's a bug of JavaScriptCore. References: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-array.prototype.push http://www.ecma-international.org/ecma-262/5.1/index.html#sec-15.4.4.7
Looks like this is a bug in JSC as well as XS, but there's no Test262 case enforcing it -- would you be willing to submit yours as a PR there? https://github.com/tc39/test262 The specific issue is that Array#push calls the abstract operation Set(... , true) which makes failure to set into a TypeError. (Thus `Array.prototype.push.call('abc', 1);` should throw even though `let x = 'abc'; x.length++;` does not.) https://tc39.es/ecma262/#sec-array.prototype.push https://tc39.es/ecma262/#sec-set-o-p-v-throw
Created attachment 379090 [details] Patch
Comment on attachment 379090 [details] Patch Attachment 379090 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13045120 New failing tests: mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-llint stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.mini-mode stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-eager-no-cjit mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-dfg-eager-no-cjit-validate-phases stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-no-cjit-validate-sampling-profiler stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-no-cjit-b3o0 stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-eager stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.no-cjit-collect-continuously stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.no-cjit-validate-phases stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.no-llint stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-no-cjit-no-inline-validate stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-no-cjit-no-put-stack-validate stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-no-cjit-small-pool stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.eager-jettison-no-cjit mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-ftl-eager-no-cjit-validate-phases stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.ftl-eager-no-cjit-b3o1 stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.no-ftl mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-baseline mozilla-tests.yaml/js1_6/Array/regress-304828.js.mozilla-no-ftl stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.dfg-eager-no-cjit-validate stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.dfg-eager stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.default stress/phantom-insertion-live-range-should-agree-with-arguments-forwarding.js.bytecode-cache
Hmm, I think I will need Saam or Yusuke to tell me how phantom-insertion-live-range-should-agree-with-arguments-forwarding.js (from r250058) should be corrected, as `Object.__proto__ = []; Object.shift();` shouldn't be valid JavaScript (even though JSC is treating it as such before this patch).
Created attachment 379103 [details] Patch
(In reply to Ross Kirsling from comment #4) > Hmm, I think I will need Saam or Yusuke to tell me how > phantom-insertion-live-range-should-agree-with-arguments-forwarding.js (from > r250058) should be corrected, as `Object.__proto__ = []; Object.shift();` > shouldn't be valid JavaScript (even though JSC is treating it as such before > this patch). I’d just make sure the test fails before my fix with this modification
(In reply to Saam Barati from comment #6) > (In reply to Ross Kirsling from comment #4) > > Hmm, I think I will need Saam or Yusuke to tell me how > > phantom-insertion-live-range-should-agree-with-arguments-forwarding.js (from > > r250058) should be corrected, as `Object.__proto__ = []; Object.shift();` > > shouldn't be valid JavaScript (even though JSC is treating it as such before > > this patch). > > I’d just make sure the test fails before my fix with this modification Confirmed!
(In reply to Ross Kirsling from comment #1) > Looks like this is a bug in JSC as well as XS, but there's no Test262 case > enforcing it -- would you be willing to submit yours as a PR there? > https://github.com/tc39/test262 > > The specific issue is that Array#push calls the abstract operation Set(... , > true) which makes failure to set into a TypeError. > (Thus `Array.prototype.push.call('abc', 1);` should throw even though `let x > = 'abc'; x.length++;` does not.) > https://tc39.es/ecma262/#sec-array.prototype.push > https://tc39.es/ecma262/#sec-set-o-p-v-throw I have no time to submit this to test262 now, but maybe later i will. And I want to confirm that this issue is a bug of JSC or not? thx.
Comment on attachment 379103 [details] Patch r=me.
Comment on attachment 379103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379103&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:187 > + putLength(exec, vm, obj, jsNumber(value)); You should call scope.release() before the call to putLength() because we expect putLength() can throw.
Created attachment 379404 [details] Patch for landing
(In reply to Mark Lam from comment #10) > Comment on attachment 379103 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379103&action=review > > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:187 > > + putLength(exec, vm, obj, jsNumber(value)); > > You should call scope.release() before the call to putLength() because we > expect putLength() can throw. Thanks! Addressed.
Comment on attachment 379404 [details] Patch for landing Clearing flags on attachment: 379404 Committed r250275: <https://trac.webkit.org/changeset/250275>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55644328>
*** Bug 188789 has been marked as a duplicate of this bug. ***
*** Bug 189470 has been marked as a duplicate of this bug. ***