Repository navigation
DCHECK(IsNumber) Crash when running debug node 4.8.3 with meteor #13351
Description
- Version: v4.8.3 (Debug build)
- Platform: Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
- Subsystem: http_parser (maybe)
Whilst trying to gather more information for a different issue, I downloaded and built the node 4.8.3 sources with debug, and then ran a complex app (including meteor, angular, plus a non-trivial app) with lldb attached.
After significant usage of the app (and significant startup time), say 30minutes, I get the following error in a debug build (release build is fine):
#
# Fatal error in ../deps/v8/src/objects-inl.h, line 1117
# Check failed: IsNumber().
#
==== C stack trace ===============================
1: V8_Fatal
2: v8::internal::Object::Number()
3: v8::Value::IntegerValue(v8::Local<v8::Context>) const
4: v8::Value::IntegerValue() const
5: node::Parser::on_headers_complete_()
6: node::Parser::on_headers_complete(http_parser*)
7: http_parser_execute
8: node::Parser::Execute(char*, unsigned long)
9: node::Parser::Execute(v8::FunctionCallbackInfo<v8::Value> const&)
10: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
11: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&)
12: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
13: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*)
14: 0x3ca8d10068fb
15: 0x3ca8d69b27c8
16: 0x3ca8d69b247b
Attached are two files containing more lldb output:
crash1.txt
crash2.txt
The error occurs at the DCHECK(IsNumber()) when this according to lldb is 0x1baddead0baddeaf aka kHandleZapValue (but that may be spurious/out-of-date frame information).
1116 double Object::Number() {
-> 1117 DCHECK(IsNumber());
1118 return IsSmi()
1119 ? static_cast<double>(reinterpret_cast<Smi*>(this)->value())
1120 : reinterpret_cast<HeapNumber*>(this)->value();
In both crashes the error occurs when doing some form of http header processing, with the data being different in the two cases.
Sadly, I don't think I can give reliable repro instructions, and I realize that there's a large stack of components sitting on top of node in order to exhibit the problem. But with my setup, it seems to be just a matter of time to repeat.
As some background, meteor (in development mode) runs two node processes one acting as a http proxy to the other. It also uses websockets, and when the app is in use there will be regular traffic both ways on the websocket. When the source of the app changes (detected via file system monitoring), the app is rebuilt and either the client and/or the server parts are reloaded as needed.
In the cases that triggered the problem, I think it was only the client restarting and hence probably re-establishing a proxied websocket, but would also have been reloading a number of non-weksocket http content too.
Activity
mscdex commentedon May 31, 2017
Does it work with node v6.x?
sdarnell commentedon Jun 1, 2017
Sorry, I'm not sure I can easily test that as meteor (and associated packages) only support 4.x at the moment, though moving to 6.x is in progress - I'll see if I can get the alpha release working to test.
bnoordhuis commentedon Jun 1, 2017
crash1.txt looks like a V8 bug at first glance:
head_response->IntegerValue()which calls the IntegerValue primitive unlessobj->IsNumber()is already true.num->Number(),IsNumber()is false.The
thisvalue in the Object::Number() stack frame doesn't leave much to the imagination:Calling
head_response->ShortPrint()in lldb might work and should tell you what kind of object it contains.If it doesn't work, then you should be able to inspect with llnode (with the
v8 inspectcommand) what kind of objecthead_responsecontains. You can install it with npm.sdarnell commentedon Jun 1, 2017
@mscdex I managed to get the pre-release of meteor working with node 6.10.3, then built a debug build and ran with that for a while. It was substantially slower and rebuilding and relaunching the app was unbearable - but in my usage to trying to reproduce the issue I couldn't. It's obviously not definitive but 6.10.3 appears to not have the issue.
sdarnell commentedon Jun 1, 2017
@bnoordhuis I switched back to debug node 4.8.3 and reproduced the issue, but for some reason I couldn't compile llnode when installed via npm, so I tried brew install llnode. This loaded into lldb but segfaulted everytime I tried
v8 inspect.ShortPrint didn't seem to work:
But after quite a bit of trial and error with the IsXxx() functions I got:
I was trying to find out what the callback which returned the value was, but I'm not sure how to drive lldb well enough, and it wouldn't let me call GetName() on cb. And I couldn't get the casting right to convert it to a v8::Function. I did check that cb->IsFunction() returned true (no big surprise).
sdarnell commentedon Jun 1, 2017
After a bit more trying, I've got a 'v8 bt' dump:
crash3.txt
Again, head_response->IsUndefined() returns true.
Tried 'v8 inspect' on various things but now, just seems to output '<Smi: 1>'.
bnoordhuis commentedon Jun 2, 2017
It's the parserOnHeadersComplete function in lib/_http_common.js. Since we haven't received bug reports for regular node applications and since I can't see how the return value could be
undefinedwith the stock implementation of parserOnHeadersComplete in v4.x, I'm inclined to say the issue is caused by meteor somehow. Barring evidence to the contrary, I suggest we close this.sdarnell commentedon Jun 2, 2017
Meteor does use faye-websocket and they appear to replace the callback, so I've raised an issue there, to suggest that the implementation match in always returning a number.
However, it strikes me that given a call out to regular js code (I can't tell whether that code was ever intended to be replaced), that the head_response should be checked first (e.g. with IsNumber()).
bnoordhuis commentedon Jun 2, 2017
It's not. I'll close this out - if you touch internals and it breaks, that's not a bug in core.