Bug 201910 – Array methods should throw TypeError upon attempting to modify a string

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yang Tian]

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

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 379090 [details]
Patch

[EWS Watchlist]

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

[Ross Kirsling]

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).

[Ross Kirsling]

Created attachment 379103 [details]
Patch

[Saam Barati]

(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

[Ross Kirsling]

(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!

[Yang Tian]

(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.

[Keith Miller]

Comment on attachment 379103 [details]
Patch

r=me.

[Mark Lam]

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.

[Ross Kirsling]

Created attachment 379404 [details]
Patch for landing

[Ross Kirsling]

(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.

[WebKit Commit Bot]

Comment on attachment 379404 [details]
Patch for landing

Clearing flags on attachment: 379404

Committed r250275: <https://trac.webkit.org/changeset/250275>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55644328>

[Alexey Shvayka]

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

[Alexey Shvayka]

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