Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Show JS exception details for FATAL ERROR message #2

Closed
wants to merge 386 commits into from
Closed

Show JS exception details for FATAL ERROR message #2

wants to merge 386 commits into from

Conversation

obastemur
Copy link
Contributor

If ChakraShim fails to initialize, there is no clear information on what is going on. This happened with the latest Windows update.

Suggested change shows the details of JS exception (if any)

othiym23 and others added 30 commits February 6, 2015 11:55
Fixes #9126.

Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
* npm: Upgrade to 2.5.1

* mdb_v8: update for v0.12 (Dave Pacheco)
'.' and '..' are directory specs and resolving urls with or without the
hostname with '.' and '..' should add a trailing slash to the end of the
url.

Fixes: nodejs/node-v0.x-archive#8992
PR-URL: nodejs/node-v0.x-archive#9010
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
AsyncWrap should always properly propagate asynchronous calls to any
child that is created. Regardless whether kCallInitHook is currently
active. The previous logic would always return early if kCallInitHook
wasn't set.

PR-URL: nodejs/node-v0.x-archive#9146
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
A number -> uint32 type coercion bug made buffer sizes
larger than kMaxLength (0x3fffffff) wrap around.

Instead of rejecting the requested size with an exception,
the constructor created a buffer with the wrong size.

PR-URL: nodejs/node#657
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: nodejs/node-v0.x-archive#9164
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
Timeout#unref() call returns undefined, not this. The test already
worked before, because the interval was still unref'd, and the test also
succeeds without clearing the interval.

PR-URL: nodejs/node-v0.x-archive#9171
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
Reviewed-by: Timothy J Fontaine <tjfontaine@gmail.com>
Change the fs.F_OK/R_OK/W_OK/X_OK out of fs.js will lead unexpect
exception. Make these properties readonly.

PR-URL: nodejs/node-v0.x-archive#9060
[trev.norris@gmail.com test properties are read only]
Signed-off-by: Trevor Norris <trev.norris@gmail.com>
Add a ';' to the end of a function call in documentation.

PR-URL: nodejs/node-v0.x-archive#9198
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
The NativeModule system passes NativeModule.require transparently and so
is unnecessary to call explicitly.

The only one which should have the prefix is the in line 295, where
actually implements a big fs-based module system and actually requires a
native module. That is left unchanged.

PR-URL: nodejs/node-v0.x-archive#9201
Ref: nodejs/node-v0.x-archive#2009
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. This commit
uses Object.create(null) to construct the _times object.

Fixes: nodejs/node-v0.x-archive#9069
PR-URL: nodejs/node-v0.x-archive#9116
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
If the Buffer allocation isn't a slice then there's no need to adjust
the pool offset after realloc'ing the space available.

Fixes: 6462519 "buffer, doc: misc. fix and cleanup"
Conflicts:
	deps/v8/src/log-utils.cc
431eb17 had integrated the addition of
V8's version in V8's profiler log files, without backporting the test
that was included in the original change
(https://codereview.chromium.org/806143002). This commit backports this
test.

The newly added test was tested with
nodejs/node-v0.x-archive#9208.
The commit in v0.10 (431eb17) that
backported the original change
(https://codereview.chromium.org/806143002) did add an extra newline
because the logging facilities in v0.10's V8 do not add one.

When merging this commit in v0.12, V8's logging facilities now
automatically add the newline character, and the debug builds assert if
one is already present.
Currently, fs.truncate() silently fails when a file descriptor
is passed as the first argument. This commit changes this
behavior to properly call fs.ftruncate().

PR-URL: nodejs/node-v0.x-archive#9161
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
This is a backport of 6c3647c from
v0.12 to v0.10.

Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. In v0.12, this was fixed
by using Object.create(null) to construct the _times object

However, the version of V8 in the v0.10 branch makes this fix not work
as expected. In v0.10, this commit changes the _times object into a
array of objects of the form:

{ label: someLabel, time: staringWallClockTime }

someLabel can thus be any string, including any string that represents
any Object.prototype field.

Fixes #9116.

PR: #9215
PR-URL: nodejs/node-v0.x-archive#9215
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`TLSSocket` wraps the original `net.Socket`, but writes/reads to/from
`TLSSocket` do not touch the timers of original `net.Socket`.

Introduce `socket._parent` property, and iterate through all parents
to unref timers and prevent timeout event on original `net.Socket`.

Fix: nodejs/node-v0.x-archive#9242
PR-URL: nodejs/node#891
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
node::Environment isn't accessible to user APIs, so extend smalloc to
also accept v8::Isolate.

Fixes: 75adde0 "src: remove `node_isolate` from source"
PR-URL: nodejs/node#905
Reviewed-by: Fedor Indutny <fedor@indutny.com>
Original commit message:

  Fix --max_old_space_size=4096 integer overflow.

  BUG=v8:3857
  LOG=N

  Review URL: https://codereview.chromium.org/897543002

  Cr-Commit-Position: refs/heads/master@{#26510}

PR-URL: nodejs/node-v0.x-archive#9200
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
closes nodejs/nodejs.org-archive#77

PR: #9172
PR-URL: nodejs/node-v0.x-archive#9172
Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
In the documentation for querystring.parse, the documentation mentions
that the default value for options.decodeURIComponent is the
decodeURIComponent function, but it's actually the querystring.unescape
function.

PR-URL: nodejs/node-v0.x-archive#9259
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
 - add an article: `decode a non-utf8 string`
 - explain default and fallback behaviour of `querystring.unescape`

PR-URL: nodejs/node-v0.x-archive#9259
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

PR-URL: nodejs/node-v0.x-archive#9270
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
Currently, lib/_tls_legacy.js and lib/crypto.js cannot be loaded when
--use-strict is passed to node. In addition to that, console.trace
throws because it uses arguments.callee.

This change fixes these issues and adds a test that makes sure
every external built-in module can be loaded with require when
--use-strict is enabled.

Please note that this change does not fix all issues with built-in
modules' code running with --use-strict. It is very likely that some
code in the built-in modules still fails when passing this flag.

However, fixing all code would require us to enable strict mode by
default in all builtins modules, which would certainly break existing
applications.

Fixes #9187.

PR: #9237
PR-URL: nodejs/node-v0.x-archive#9237
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Conflicts:
	lib/console.js
	test/simple/test-console.js
This test setups two event listeners: one on a child process' exit event
, another for the same child process' stdandard output's 'data' event.
The data even listener writes to a stream, and the exit event listener
ends it.

Because the exit event can be emitted before the data event, there is a
chance that something will be written to the stream after it's ended,
and that an error is thrown.

This change makes the test end the stream in the listener for the child
process' standard output's end event, which is guaranteed to be emitted
after the last data event, thus avoiding the race.

PR: #9301
PR-URL: nodejs/node-v0.x-archive#9301
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@users.noreply.github.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
aruneshchandra and others added 9 commits September 8, 2015 11:27
When use SetInternalField with Interger will assert:

// assert this
object->SetInternalField(0, v8::Integer::New(env->isolate(), 6));
object->GetInternalField(0)->IsInt32();

Becasue ObjectData::FieldValue will change ref pointer with a mask , and then will not revert to a normal int value.

Soloution:
Add a bool flag to ObjectData::FieldValue to demined a FieldValue is JsValueRef or not.
I was implement v8::FunctionTemplate::Inherit and use in my game engine.
Fix JsIntToNumber calls which may overflow when casting to int.

Reviewed-by: @kunalspathak
"supportsOverrideToString" was originally added to make old
FastBuffer work. Buffer has long changed implementation. This is
no longer used.

Fix #80

Reviewed-by: @kunalspathak
Added below 2 new nan.h APIs of version 2.12
- SetCallHandler
- SetCallAsFunctionHandler

Reviewed-By: @munyirik
Fixed the install windows link to a more specific location
If ChakraShim fails to initialize, there is no clear information on what is going on. This happened with the latest Windows update.

Suggested change shows the details of JS exception (if any)
@obastemur
Copy link
Contributor Author

@kunalspathak I sent these three PRs since there is no node-ch version is actually working on any updated Windows. These are some of the things I've done to make JXcore(Chakra) working on Windows UWP again.

I wasn't expecting this much behavior change in Chakra native API since it's shipped with OS. I do understand the JS engine is evolving over time but also expect that the C API is behaving similarly. My naive hope is that the breaking changes in Chakra native API was limited to this Windows update or you release the source codes for Chakra before it breaks again on next Windows update.

Otherwise it will be hard to ship any UWP/UWA app with either JXcore or node-ch.

kunalspathak pushed a commit that referenced this pull request Dec 7, 2015
PR #2 enables the details for the exception below;
```
FATAL ERROR: Failed to initialize context
TypeError: Unable to get property 'get' of undefined or null reference
   at Anonymous function (Unknown script code:393:7)
   at patchUtils (Unknown script code:386:5)
   at Anonymous function (Unknown script code:410:3)
```
@kunalspathak
Copy link
Member

@obastemur , Thanks for submitting the PRs. There were known issues with node0.12 with chakra.dll shipped with Windows update. We will be soon publishing our node6.0 version in "chnext" that works with latest chakra.dll shipped and addresses the issue in #3 .
I have merged #2 and #3 in "ch0.12" branch which is based on node0.12. I am reviewing #2 and #3 if that should go in "chnext" as well.

@kunalspathak
Copy link
Member

Merged 5bee568 in "ch0.12"

@jianchun
Copy link

jianchun commented Dec 7, 2015

@obastemur Thanks for your fixes! We got delayed by schedule changes and we are pushing updated node-ch today.

Just to clarify, the behavior change you addressed with PR #3 was actually from JS spec, not the Chakra API itself. We'll certainly take note and be very careful to keep Chakra APIs stable. (The other 2 PRs are not behavior change related.)

@obastemur
Copy link
Contributor Author

@kunalspathak thanks for the details.

@obastemur Thanks for your fixes! We got delayed by schedule changes and we are pushing updated node-ch today.

@jianchun good to hear that.

Just to clarify, the behavior change you addressed with PR #3 was actually from JS spec, not the Chakra API itself. We'll certainly take note and be very careful to keep Chakra APIs stable. (The other 2 PRs are not behavior change related.)

Actually #1 is a Chakra API change. Same call on previous Chakra works as expected.

Besides, i.e.

  JsValueRef args[] = { nullptr, target };
  JsErrorCode error = JsConstructObject(hostRef, args, _countof(args), result);

Consider hostRef as a constructor on top of global, this won't work with the new Chakra.dll. (instead of nullptr, we should reference 'this' object) However it works with the pre-update Chakra.

IMHO, these are all expected changes from a JS engine. However it's hard to provide a node like solution when you are not able to lock the JS engine version.

@jianchun
Copy link

jianchun commented Dec 7, 2015

@obastemur We are not aware of the issue in #1. I'll try to repro without your fix; please let us know in thread #1 if you have more details.

Yes, the JsConstructObject/JsCallFunction arguments requirement hardening is a breaking change that we took after evaluating all options. The previous loose requirement was leading to inconsistency and crashes. Apologize for any inconvenience.

@kunalspathak
Copy link
Member

Included the change in c8385b0. Closing this PR.

jackhorton pushed a commit that referenced this pull request Nov 2, 2017
Currently when running the test without an internet connection there are
two JavaScript test failures and one cctest. The cctest only fails on
Mac as far as I know. (I've only tested using Mac and Linux thus far).

This commit moves the two JavaScript tests to test/internet.

The details for test_inspector_socket_server.cc:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
make[1]: *** [cctest] Segmentation fault: 11
make: *** [test] Error 2

lldb output:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
Process 63058 stopped
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
    frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
libsystem_info.dylib`_gai_simple:
->  0x7fff96d04384 <+87>: movw   (%rdx), %ax
    0x7fff96d04387 <+90>: movw   %ax, -0x2a(%rbp)
    0x7fff96d0438b <+94>: movq   %r13, -0x38(%rbp)
    0x7fff96d0438f <+98>: movq   0x18(%rbp), %rcx

(lldb) bt
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
  * frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
    frame #1: 0x00007fff96cfe98b libsystem_info.dylib`search_addrinfo +
179
    frame #2: 0x00007fff96cfafef libsystem_info.dylib`si_addrinfo + 2255
    frame #3: 0x00007fff96cfa67b libsystem_info.dylib`getaddrinfo + 179
    frame #4: 0x00000001017d8888
cctest`uv__getaddrinfo_work(w=0x00007fff5fbfe210) + 72 at
getaddrinfo.c:102
    frame #5: 0x00000001017d880e
cctest`uv_getaddrinfo(loop=0x000000010287cb80, req=0x00007fff5fbfe1c8,
cb=0x0000000000000000, hostname="nodejs.org", service="0",
hints=0x00007fff5fbfe268) + 734 at getaddrinfo.c:192
    frame #6: 0x000000010171f781
cctest`node::inspector::InspectorSocketServer::Start(this=0x00007fff5fbfe658)
+ 801 at inspector_socket_server.cc:398
    frame #7: 0x00000001016ed590
cctest`InspectorSocketServerTest_FailsToBindToNodejsHost_Test::TestBody(this=0x0000000105001fd0)
+ 288 at test_inspector_socket_server.cc:593

I'm not sure about the exact cause for this but when using a standalone
c program to simulate this it seems like when the ai_flags
`AI_NUMERICSERV` is set, which is done in inspector_socket_server.cc
line 394, the servname (the port in the FailsToBindToNodejsHost test) is
expected to be a numeric port string to avoid looking it up in
/etc/services. When the port is 0 as is it was before this commit the
segment fault occurs but not if it is non-zero.

PR-URL: nodejs/node#16255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
kfarnung pushed a commit that referenced this pull request Jan 11, 2018
Currently when running the test without an internet connection there are
two JavaScript test failures and one cctest. The cctest only fails on
Mac as far as I know. (I've only tested using Mac and Linux thus far).

This commit moves the two JavaScript tests to test/internet.

The details for test_inspector_socket_server.cc:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
make[1]: *** [cctest] Segmentation fault: 11
make: *** [test] Error 2

lldb output:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
Process 63058 stopped
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
    frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
libsystem_info.dylib`_gai_simple:
->  0x7fff96d04384 <+87>: movw   (%rdx), %ax
    0x7fff96d04387 <+90>: movw   %ax, -0x2a(%rbp)
    0x7fff96d0438b <+94>: movq   %r13, -0x38(%rbp)
    0x7fff96d0438f <+98>: movq   0x18(%rbp), %rcx

(lldb) bt
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
  * frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
    frame #1: 0x00007fff96cfe98b libsystem_info.dylib`search_addrinfo +
179
    frame #2: 0x00007fff96cfafef libsystem_info.dylib`si_addrinfo + 2255
    frame #3: 0x00007fff96cfa67b libsystem_info.dylib`getaddrinfo + 179
    frame #4: 0x00000001017d8888
cctest`uv__getaddrinfo_work(w=0x00007fff5fbfe210) + 72 at
getaddrinfo.c:102
    frame #5: 0x00000001017d880e
cctest`uv_getaddrinfo(loop=0x000000010287cb80, req=0x00007fff5fbfe1c8,
cb=0x0000000000000000, hostname="nodejs.org", service="0",
hints=0x00007fff5fbfe268) + 734 at getaddrinfo.c:192
    frame #6: 0x000000010171f781
cctest`node::inspector::InspectorSocketServer::Start(this=0x00007fff5fbfe658)
+ 801 at inspector_socket_server.cc:398
    frame #7: 0x00000001016ed590
cctest`InspectorSocketServerTest_FailsToBindToNodejsHost_Test::TestBody(this=0x0000000105001fd0)
+ 288 at test_inspector_socket_server.cc:593

I'm not sure about the exact cause for this but when using a standalone
c program to simulate this it seems like when the ai_flags
`AI_NUMERICSERV` is set, which is done in inspector_socket_server.cc
line 394, the servname (the port in the FailsToBindToNodejsHost test) is
expected to be a numeric port string to avoid looking it up in
/etc/services. When the port is 0 as is it was before this commit the
segment fault occurs but not if it is non-zero.

PR-URL: nodejs/node#16255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
boingoing pushed a commit that referenced this pull request Jan 18, 2018
Remove a pointless adapter frame  by fixing up the function's formal
parameter count.  Before:

    frame #0: 0x000033257ea446d5 onParserExecute(...)
    frame #1: 0x000033257ea3b93f <adaptor>
    frame #2: 0x000033257ea41959 <internal>
    frame #3: 0x000033257e9840ff <entry>

After:

    frame #0: 0x00000956287446d5 onParserExecute(...)
    frame #1: 0x0000095628741959 <internal>
    frame #2: 0x00000956286840ff <entry>

PR-URL: nodejs/node#17693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
kfarnung pushed a commit that referenced this pull request Mar 7, 2018
Remove a pointless adapter frame  by fixing up the function's formal
parameter count.  Before:

    frame #0: 0x000033257ea446d5 onParserExecute(...)
    frame #1: 0x000033257ea3b93f <adaptor>
    frame #2: 0x000033257ea41959 <internal>
    frame #3: 0x000033257e9840ff <entry>

After:

    frame #0: 0x00000956287446d5 onParserExecute(...)
    frame #1: 0x0000095628741959 <internal>
    frame #2: 0x00000956286840ff <entry>

PR-URL: nodejs/node#17693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.