1200108 - Array.prototype.map called on a negative length arraylike incorrectly hangs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ljharb]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/600.8.9 (KHTML, like Gecko) Version/8.0.8 Safari/600.8.9

Steps to reproduce:

Run `var a = Array.prototype.map.call({length:-1}, function () { throw new Error(); }); console.log(true, a);` in the FF Nightly console.


Actual results:

Nothing (it returns undefined and prints nothing)


Expected results:

With (previous) ES5 semantics, it should throw an error. With ES6 semantics, it should return an empty array, and then console.log that.

This means that including the es6-shim in the page, which does a runtime check for Array#map semantics, will *hang the JS engine* - this includes any major site that uses it.

This is a regression from a recent version of FF Nightly where it threw some sort of `int32` exception.

Comment 1

[André Bargull [:anba]]

Caused by bug 1195298, setting NI for jandem and till. :-)


js> Array.prototype.map.call({length:-1}, function () { throw new Error(); })
Assertion failure: lengthDouble < (2147483647), at /home/andre/hg/mozilla-central/js/src/vm/SelfHosting.cpp:301

Stack trace:
---
0x00000000008146b3 in js::intrinsic_NewDenseArray (cx=0x7ffff6908c00, argc=1, vp=0x7ffff3df6170) at /home/andre/hg/mozilla-central/js/src/vm/SelfHosting.cpp:301
301	    MOZ_ASSERT(lengthDouble < INT32_MAX);
(gdb) bt
#0  0x00000000008146b3 in js::intrinsic_NewDenseArray (cx=0x7ffff6908c00, argc=1, vp=0x7ffff3df6170) at /home/andre/hg/mozilla-central/js/src/vm/SelfHosting.cpp:301
#1  0x000000000076f26e in js::CallJSNative (cx=0x7ffff6908c00, native=0x8145c4 <js::intrinsic_NewDenseArray(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/hg/mozilla-central/js/src/jscntxtinlines.h:235
#2  0x000000000073b1a7 in js::Invoke (cx=0x7ffff6908c00, args=..., construct=js::NO_CONSTRUCT) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:763
#3  0x000000000074aba3 in Interpret (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:3067
#4  0x000000000073ad96 in js::RunScript (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:704
#5  0x000000000073b26d in js::Invoke (cx=0x7ffff6908c00, args=..., construct=js::NO_CONSTRUCT) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:781
#6  0x0000000000d9e955 in js::fun_call (cx=0x7ffff6908c00, argc=2, vp=0x7ffff3df60a8) at /home/andre/hg/mozilla-central/js/src/jsfun.cpp:1170
#7  0x000000000076f26e in js::CallJSNative (cx=0x7ffff6908c00, native=0xd9e788 <js::fun_call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/hg/mozilla-central/js/src/jscntxtinlines.h:235
#8  0x000000000073b1a7 in js::Invoke (cx=0x7ffff6908c00, args=..., construct=js::NO_CONSTRUCT) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:763
#9  0x000000000074aba3 in Interpret (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:3067
#10 0x000000000073ad96 in js::RunScript (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:704
#11 0x000000000073c43c in js::ExecuteKernel (cx=0x7ffff6908c00, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., 
    result=0x7fffffffd440) at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:978
#12 0x000000000073c741 in js::Execute (cx=0x7ffff6908c00, script=..., scopeChainArg=..., rval=0x7fffffffd440)
    at /home/andre/hg/mozilla-central/js/src/vm/Interpreter.cpp:1012
#13 0x0000000000d4860f in ExecuteScript (cx=0x7ffff6908c00, scope=..., script=..., rval=0x7fffffffd440) at /home/andre/hg/mozilla-central/js/src/jsapi.cpp:4356
#14 0x0000000000d489a2 in JS_ExecuteScript (cx=0x7ffff6908c00, scriptArg=..., rval=...) at /home/andre/hg/mozilla-central/js/src/jsapi.cpp:4381
#15 0x000000000043118c in EvalAndPrint (cx=0x7ffff6908c00, 
    bytes=0x7ffff6902d00 "Array.prototype.map.call({length:-1}, function () { throw new Error(); })\n", '\245' <repeats 54 times>, "\300\363\345\367\377\177\374\377", 
    length=74, lineno=1, compileOnly=false, out=0x7ffff6f6a740 <_IO_2_1_stdout_>) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:487
#16 0x0000000000431652 in ReadEvalPrintLoop (cx=0x7ffff6908c00, in=0x7ffff6f69980 <_IO_2_1_stdin_>, out=0x7ffff6f6a740 <_IO_2_1_stdout_>, compileOnly=false)
    at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:551
#17 0x0000000000431815 in Process (cx=0x7ffff6908c00, filename=0x0, forceTTY=true) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:582
#18 0x00000000004463e4 in ProcessArgs (cx=0x7ffff6908c00, op=0x7fffffffd940) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:5801
#19 0x0000000000447706 in Shell (cx=0x7ffff6908c00, op=0x7fffffffd940, envp=0x7fffffffdb48) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:6121
#20 0x0000000000448a81 in main (argc=1, argv=0x7fffffffdb38, envp=0x7fffffffdb48) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:6470
---

Comment 2

[Jan de Mooij [:jandem]]

Hm, before bug 1195298, we'd throw in intrinsic_NewDenseArray because the argument is not an int32 but a double (UINT32_MAX).

Now when we ignore the assert in intrinsic_NewDenseArray, we try to allocate an array of length UINT32_MAX and this fails, but doesn't throw an exception...

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Hm, before bug 1195298, we'd throw in intrinsic_NewDenseArray because the
> argument is not an int32 but a double (UINT32_MAX).

OK, I think this was also unexpected. Now NewDenseArray tries to create a fully allocated array with a giant length and this fails. Chrome and Safari just perform the map and immediately reach this case:

https://github.com/paulmillr/es6-shim/blob/616eee62ceed6f1e988abab4e49d6d682702be1b/es6-shim.js#L1088

I think that's the behavior we want here. Replacing the NewDenseArray call with std_Array seems to fix that.

I'll see if we can get rid of NewDenseArray completely.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch removes the NewDenseArray intrinsic and uses std_Array instead.

I think NewDenseArray was necessary for UnsafePutElements. However that has been replaced with _DefineDataProperty and we no longer need to allocate all elements upfront.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8654803 [details] [diff] [review]
Patch

Review of attachment 8654803 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, good to see this going. Do we even need Std_Array, though? Why can't we always use [] instead? If you think it'd be good to be closer to the spec here, then that's a perfectly fine reason. I'm fine with using [], too.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #5)
> Do we even need Std_Array, though? Why can't
> we always use [] instead? If you think it'd be good to be closer to the spec
> here, then that's a perfectly fine reason. I'm fine with using [], too.

If we know the array length, std_Array is more efficient because it will eagerly allocate the elements if the length < 1000 or so. If we use [] we may have to realloc the elements a number of times.

Thanks for the fast review!

Comment 7

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> If we know the array length, std_Array is more efficient because it will
> eagerly allocate the elements if the length < 1000 or so. If we use [] we
> may have to realloc the elements a number of times.

Ah, I misremembered, then. I was under the impression we never allocate eagerly. But it makes much more sense that we'd only do it up to a certain length.
> 
> Thanks for the fast review!

Sure.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff3c8b20659

Comment 10

[ljharb]

Thanks for the lightning fast responses on this! As long as this regression doesn't make it into a stable Firefox, then I won't have to patch around it for literally forever in the es6-shim, so the sooner a fix is merged hte better :-)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dff3c8b20659