Closed
Bug 1200108
Opened 9 years ago
Closed 9 years ago
Array.prototype.map called on a negative length arraylike incorrectly hangs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ljharb, Assigned: jandem)
References
Details
Attachments
(1 file)
7.51 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
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.
Summary: Array.prototype.map called on a negative length arraylike incorrectly noops → Array.prototype.map called on a negative length arraylike incorrectly hangs
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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...
Assignee | ||
Comment 3•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(till)
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Reporter | ||
Comment 10•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dff3c8b20659
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•