-
Notifications
You must be signed in to change notification settings - Fork 7.3k
"FATAL ERROR: invalid array length Allocation failed - process out of memory" introduced in 0.11.15 #9181
Comments
See nodejs/node-v0.x-archive#9181, introduced in 0.11.15 Since bug affects nodejs stable 0.12, travis now uses 0.11.14 and io.js
@SheetJSDev Can you post the URL to the exact test that's failing? |
I pushed a fix that was created by @bnoordhuis in 04b63e0. Can you let me know if that fixes the issue? |
@trevnorris that did not resolve the issue, unfortunately :( I pulled from the v0.12 branch which appears to have that commit. The repo is https://github.com/SheetJS/js-xls and the travis commands are https://github.com/SheetJS/js-xls/blob/master/.travis.yml (warning: it downloads a ton of test files) It's difficult to point to a specific test because each individual test case works on its own (the repo includes a CLI script xls.njs and that passes on each file, even in 0.11.15 and 0.12). It just blows up after a certain number of tests. |
@SheetJSDev Thanks. I've been able to reproduce the issue. I'll do some testing and let you know what I find. |
Update: this is a race condition, but for what I can tell. If I run the tests using latest v0.12 debug build it doesn't have that issue. On the flip side, it generated this error:
I've bisected the bad range from 9116b24 to 7a0cfe9. That series of commits is where we upgrade V8 to 3.28.73. So it appears the issue is coming from V8 itself. |
@trevnorris thanks for looking into this! |
@SheetJSDev I've opened up a PR to help take care of this issue (see #9185). Thanks for reporting it. |
@trevnorris it seems that v8 has attempted to trim fixed array in Large Object space. (probably you already figured it out) |
And it doesn't seem like recent v8 has that assertion anymore... Searching for particular commit... |
@trevnorris v8/v8@5e57059?diff=unified#diff-27e3acf610fe20cbc4721c969ad00f62L3324 see |
And big kudos for using stale v8 version again, gosh... |
@indutny thanks for narrowing that down. I ported the fix, and while it does fix the abort in the debug build, it doesn't prevent a FATAL ERROR from a release build. |
And, again for the record, the reason V8 hasn't been updated was because there are too many customers on el5/6, etc., that don't have easy access to gcc that supports compiling native add-ons using c++11. If it weren't for that, I would have made sure we'd upgrade V8 before the v0.12.0 release. |
@trevnorris @indutny again thank you very much for looking into this! We should have picked up on this sooner but most of our users are using the 0.10 stable series. There are many people who are discovering node through our modules, and the last time we had an issue with a node stable version (0.10.31 #8208) we advised people to roll back to 0.10.30. What should we recommend for new people who install node 0.12.0: use stable 0.10, switch to iojs or wait for 0.12.1? |
@SheetJSDev Unfortunately NodeSummit is happening at the moment, so most of the team is tied up. I'd say wait until v0.12.1. |
@SheetJSDev #9185 landed as #18206, and I believe it fixes this issue. Before the above mentioned changes, I was able to reproduce the
After the changes, I get a different error:
Could you please confirm that #18206 fixes your issue? |
Sorry for very late reply, but this was resolved in 0.12.18 and most likely in a much older version too. |
The js-xls test script passes in node <= 0.11.14 as well as in io.js but not in node 0.11.15 0.11.16 or 0.12
https://travis-ci.org/SheetJS/js-xls/builds/50244885 shows where the fails occur. It works in io.js as well as in node 0.10 and versions up to 0.11.14 so I suspect that 0.11.15 changed Buffer behavior.
@trevnorris was there a fix in iojs that hasn't been applied to node?
The text was updated successfully, but these errors were encountered: