DCHECK(IsNumber) Crash when running debug node 4.8.3 with meteor

[sdarnell]

• 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.

[mscdex]

Does it work with node v6.x?

[sdarnell]

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]

crash1.txt looks like a V8 bug at first glance:

1. The abort is triggered by head_response->IntegerValue() which calls the IntegerValue primitive unless obj->IsNumber() is already true.
2. Later on, in the call to num->Number(), IsNumber() is false.
3. Therefore logically the primitive must have returned something that isn't a number.

The this value in the Object::Number() stack frame doesn't leave much to the imagination:

(v8::internal::Object *) this = 0x1baddead0baddeaf

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 inspect command) what kind of object head_response contains. You can install it with npm.

[sdarnell]

@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]

@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:

(lldb) p head_response->ShortPrint()
error: no member named 'ShortPrint' in 'v8::Value'

But after quite a bit of trial and error with the IsXxx() functions I got:

(lldb) p head_response->IsUndefined()
(bool) $10 = true

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]

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]

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 undefined with 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]

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]

I can't tell whether that code was ever intended to be replaced

It's not. I'll close this out - if you touch internals and it breaks, that's not a bug in core.