Nan 2.10.0 causing errors

[xzyfer]

It appears the native addon Node Sass compiled for Node 9 with Nan 2.10.0 are resulting in errors.

i.e.

[1]    9331 illegal hardware instruction  npm test

There is more detailed information in sass/node-sass#2293

This may be related the the MakeCallback deprecation. We're happy to address the deprecation within Node Sass, but the I suspect these failure were unexpected.

[kkoopa]

Do not see how any of the changes could be related. The stack traces do not indicate any error with makecallback and the deprecation should not have affected anything, since that only makes the compiler emit warnings.

…

On March 17, 2018 9:02:37 AM GMT+02:00, Michael Mifsud ***@***.***> wrote: It appears the native addon Node Sass compiled for Node 9 with Nan 2.10.0 are resulting in errors. i.e. ``` [1] 9331 illegal hardware instruction npm test ``` There is more detailed information in sass/node-sass#2293 This may be related the the MakeCallback deprecation. We're happy to address the deprecation within Node Sass, but the I suspect these failure were unexpected.

[xzyfer]

I'll aim to do a bisect this week to find the offending commit, however we are fairly confident the nan bump to be the root cause of the error. It's possible we have always had a latent bug that this change has surfaced.

…

On 17 Mar. 2018 9:18 pm, "Benjamin Byholm" ***@***.***> wrote: Do not see how any of the changes could be related. The stack traces do not indicate any error with makecallback and the deprecation should not have affected anything, since that only makes the compiler emit warnings. On March 17, 2018 9:02:37 AM GMT+02:00, Michael Mifsud < ***@***.***> wrote: >It appears the native addon Node Sass compiled for Node 9 with Nan >2.10.0 are resulting in errors. > >i.e. >``` >[1] 9331 illegal hardware instruction npm test >``` > >There is more detailed information in >sass/node-sass#2293 > >This may be related the the MakeCallback deprecation. We're happy to >address the deprecation within Node Sass, but the I suspect these >failure were unexpected. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#755 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWGMUT40-IiM_ISqYOZuhCf1_gUGWks5tfOL-gaJpZM4SusrQ> .

[kkoopa]

I cannot exclude a bug in NAN, but I do not see how the changes since 2.9.2 could have this effect. diff

[kkoopa]

Then again, I now have a theory. Nan::MakeCallback now returns an empty handle upon failure. Perhaps it used to return undefined or such previously. Nevermind.

[kkoopa]

Alright, found the cause of the error, you are doing an asynchronous call where you meant to do a synchronous call. This issue became prominent due to the new async hooks changes made to accomodate Node 10.

--- a/src/callback_bridge.h
+++ b/src/callback_bridge.h
@@ -107,7 +107,7 @@ T CallbackBridge<T, L>::operator()(std::vector<void*> argv) {
     argv_v8.push_back(Nan::New(wrapper));

     return this->post_process_return_value(
-      this->callback->Call(argv_v8.size(), &argv_v8[0])
+      Nan::Call(*callback, argv_v8.size(), &argv_v8[0]).ToLocalChecked()
     );
   } else {
     /*

[xzyfer]

Great, thanks. I can't try this adjustment on our end and see how it goes. cc @saper

…

On 17 Mar. 2018 11:29 pm, "Benjamin Byholm" ***@***.***> wrote: Alright, found the cause of the error, you are doing an asynchornous call where you meant to do a synchronous call. This issue became prominent due to the new asynch hooks changes made to accomodate Node 10. --- a/src/callback_bridge.h+++ b/src/callback_bridge.h@@ -107,7 +107,7 @@ T CallbackBridge<T, L>::operator()(std::vector<void*> argv) { argv_v8.push_back(Nan::New(wrapper)); return this->post_process_return_value(- this->callback->Call(argv_v8.size(), &argv_v8[0])+ Nan::Call(*callback, argv_v8.size(), &argv_v8[0]).ToLocalChecked() ); } else { /* — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#755 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWPqueV-bzGe3wc4wuSbN7UhH7RZHks5tfQG7gaJpZM4SusrQ> .

[kkoopa]

While the problem has been fixed, I still do not know precisely why it occurred, or if it can be easily prevented from our end, so this issue is still not resolved. Perhaps @ofrobots can take a look next week?

[saper]

Strange, always trying to do Nan::Get() for a third time on that object causes a crash, even asking for the same property three times.