Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inheritance of C/C++ Addons doesn't work #9288

Closed
stixx200 opened this issue Oct 26, 2016 · 5 comments
Closed

inheritance of C/C++ Addons doesn't work #9288

stixx200 opened this issue Oct 26, 2016 · 5 comments
Labels
addons Issues and PRs related to native addons. v8 engine Issues and PRs related to the V8 dependency.

Comments

@stixx200
Copy link

stixx200 commented Oct 26, 2016

  • Version: v6.9.1
  • Platform: Win7 SP1, 64-Bit
  • Subsystem:

There is an issue with the inheritance of native node addons in Node v6.9.1.
The problem doesn't exist with Node v4.6.1 or Node v7.0.0.

To reconstruct, i used the example code of Node docs (https://nodejs.org/dist/latest-v6.x/docs/api/addons.html#addons_wrapping_c_objects) and ran the code with this example:

"use strict";
const addon = require('./build/Release/addon');
class Test extends addon.MyObject {
    constructor() {
        super();
        this.doOutput();
    }
    doOutput() {
        console.log("hello world");
    }
}
var obj = new Test();

Result is:

TypeError: this.doOutput is not a function

The problem is, that the this is empty after super() call.

@mscdex mscdex added the addons Issues and PRs related to native addons. label Oct 26, 2016
@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Oct 26, 2016
@bnoordhuis
Copy link
Member

This was fixed in V8 5.2 in commit v8/v8@73ee7943 but it also depends on commit v8/v8@306c412c which is arguably an ABI change and therefore not eligible for back-porting. I'll open a pull request for further discussion.

@bnoordhuis
Copy link
Member

#9293

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Nov 21, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: nodejs#9689
Refs: nodejs#9288
Refs: nodejs#9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this issue Nov 22, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Dec 20, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@stixx200
Copy link
Author

@bnoordhuis Do you know, which node v6 version will fix this issue?

@addaleax
Copy link
Member

@stixx200 Whatever version the changes in #9293 land in (if they do at all).

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 2, 2017
The patch has been modified to maintain ABI compatibility.  The original
change removes the v8::FunctionCallbackInfo<T>::is_construct_call_ field
from deps/v8/include/v8.h.  The field is set directly by JIT-ted code so
the removal of those code paths has been backed out as well.

Original commit message:

    [api] Expose FunctionCallbackInfo::NewTarget

    This is needed by Blink to implement the Custom Elements spec.

    BUG=v8:4261
    LOG=y

    Review-Url: https://codereview.chromium.org/1910253005
    Cr-Commit-Position: refs/heads/master@{nodejs#35833}

Fixes: nodejs#9288
PR-URL: nodejs#9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 2, 2017
Original commit message:

    When instantiating a subclassed API function, the instance cache
    is avoided. There is currently no direct API yet to instantiate
    a Template while passing in a new.target. It probably makes sense
    to extend ObjectTemplate::NewInstance to accept a new.target, in
    line with Reflect.construct.

    BUG=v8:3330, v8:5001

    Review-Url: https://codereview.chromium.org/1972613002
    Cr-Commit-Position: refs/heads/master@{nodejs#36179}

Fixes: nodejs#9288
PR-URL: nodejs#9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 2, 2017
Remove the new method that was introduced in the back-port of
v8/v8@306c412c ("[api] Expose FunctionCallbackInfo::NewTarget")
so that the meat of the patch can land in a patch release.

This commit can be reverted again in the next minor release.

Fixes: nodejs#9288
PR-URL: nodejs#9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 2, 2017
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: nodejs#9689
Refs: nodejs#9288
Refs: nodejs#9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@bnoordhuis
Copy link
Member

Fixed in 1d631ce...2ebceed, will be in the next v6.x release.

MylesBorins pushed a commit that referenced this issue Mar 9, 2017
The patch has been modified to maintain ABI compatibility.  The original
change removes the v8::FunctionCallbackInfo<T>::is_construct_call_ field
from deps/v8/include/v8.h.  The field is set directly by JIT-ted code so
the removal of those code paths has been backed out as well.

Original commit message:

    [api] Expose FunctionCallbackInfo::NewTarget

    This is needed by Blink to implement the Custom Elements spec.

    BUG=v8:4261
    LOG=y

    Review-Url: https://codereview.chromium.org/1910253005
    Cr-Commit-Position: refs/heads/master@{#35833}

Fixes: #9288
PR-URL: #9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Original commit message:

    When instantiating a subclassed API function, the instance cache
    is avoided. There is currently no direct API yet to instantiate
    a Template while passing in a new.target. It probably makes sense
    to extend ObjectTemplate::NewInstance to accept a new.target, in
    line with Reflect.construct.

    BUG=v8:3330, v8:5001

    Review-Url: https://codereview.chromium.org/1972613002
    Cr-Commit-Position: refs/heads/master@{#36179}

Fixes: #9288
PR-URL: #9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Remove the new method that was introduced in the back-port of
v8/v8@306c412c ("[api] Expose FunctionCallbackInfo::NewTarget")
so that the meat of the patch can land in a patch release.

This commit can be reverted again in the next minor release.

Fixes: #9288
PR-URL: #9293
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Add a test that checks that new.target inheritance works when inheriting
from a constructor defined in C++.

PR-URL: #9689
Refs: #9288
Refs: #9293
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants