Closed Bug 1200108 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ljharb, Assigned: jandem)

References

Details

Attachments

(1 file)

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.
Severity: normal → blocker
Summary: Array.prototype.map called on a negative length arraylike incorrectly noops → Array.prototype.map called on a negative length arraylike incorrectly hangs
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
---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(till)
Flags: needinfo?(jdemooij)
See Also: → 1199935
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...
(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.
Flags: needinfo?(till)
Attached patch PatchSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8654803 - Flags: review?(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.
Attachment #8654803 - Flags: review?(till) → review+
(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!
(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.
Duplicate of this bug: 1199935
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
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 :-)
https://hg.mozilla.org/mozilla-central/rev/dff3c8b20659
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
See Also: → 924058
You need to log in before you can comment on or make changes to this bug.