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.

Sign up for GitHub

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

Jump to bottom

Possible bug: instanceof evaluates to true in Node 4 but not Node 6 #7592

Closed
ghost opened this issue Jul 7, 2016 · 11 comments
Closed

Possible bug: instanceof evaluates to true in Node 4 but not Node 6 #7592

ghost opened this issue Jul 7, 2016 · 11 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@ghost
Copy link

ghost commented Jul 7, 2016 •
edited by ghost
Loading

  • Version: 6.3.0:
  • Platform: Darwin 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64:
  • Subsystem:

In Node 4, the last statement evaluates to true, but in Node 6 it evaluates to false. Is this a bug?

F = () => {};
F.prototype = {};
Object.create(F.prototype) instanceof F;

Another user verified the behavior on their Windows system: http://stackoverflow.com/questions/38253656/instanceof-evaluates-to-true-in-node-4-but-not-node-6
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Jul 7, 2016
@mscdex
Copy link
Contributor

mscdex commented Jul 7, 2016

/cc @nodejs/v8

@mscdex
Copy link
Contributor

mscdex commented Jul 7, 2016

I should note that it returns true in master.

@bnoordhuis
Copy link
Member

Looks like a bug to me. v7.0.0-pre (V8 5.1) returns true, which is what I would expect.

@Trott
Copy link
Member

Trott commented Jul 7, 2016

All Node.js version 6.x releases that I tried returned the surprising false including 6.0.0.

Version 5.12.0 returned true.

So this would seem to be either a bug in the V8 that shipped with Node.js v6.x or (less likely, I imagine, but still possible) a bug in a patch that we are only floating in v6.x. (I'm not even sure there are any that are in 6.x but not in 5.x or current master.)

@fhinkel
Copy link
Member

fhinkel commented Jul 7, 2016

In d8 version 5.0.71.54 it returns false. So not related to floating patches.

@fhinkel
Copy link
Member

fhinkel commented Jul 8, 2016

This is the commit that fixed it: https://codereview.chromium.org/1810953002/

The patch applies clean on v6.x and fixes this issue. Do we want to float the patch on v6.x or just wait because it's fixed in V8 5.1 anyways?

@Trott
Copy link
Member

Trott commented Jul 8, 2016

/cc @nodejs/lts Not strictly an LTS question/issue, but I suspect they'll have an opinion on @fhinkel's question.

@MylesBorins
Copy link
Contributor

Another patch was recently back ported to 5.1 by @bnoordhuis

There is going to be a discussion about maintaining lts release streams directly from V8 to avoid floating patches. Aiming for 6 am estwif anyone wants to join.

@bnoordhuis
Copy link
Member

I'm all for back-porting patches that apply cleanly or with minimal wedging.

@fhinkel I see you've done the work already. If you file a pull request and cc me, I'll review it.

@fhinkel fhinkel mentioned this issue Jul 9, 2016
Closed
3 tasks
fhinkel added a commit to fhinkel/node that referenced this issue Jul 21, 2016
@fhinkel
deps: cherry-pick 2aa070be from V8 upstream
ca51390 
Original commit message:

    InstanceOfStub incorrectly interprets the hole as a prototype.

    Repair this to match what the runtime correctly does, by first
    checking if the function is a constructor before we access the
    prototype.

    [email protected]

    BUG=

    Committed: https://crrev.com/2aa070be4fd2960df98905b254f12ed801ef26cd

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

This fixes the behavior of instanceof when the second parameter is not a
constructor.

Fixes: nodejs#7592
fhinkel added a commit to fhinkel/node that referenced this issue Jul 21, 2016
@fhinkel
deps: cherry-pick 3a903c4 for PPC from V8 upstream
e06e255 
Original commit message:

    PPC: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected], [email protected],
    [email protected]

    BUG=

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

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

Fixes: nodejs#7592 for PPC
fhinkel added a commit to fhinkel/node that referenced this issue Jul 21, 2016
@fhinkel
deps: cherry-pick 5b5d24b for X87 from V8 upstream
f007c0a 
Original commit message:

    port 2aa070b (r34863)

    original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    BUG=

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

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

Fixes: nodejs#7592 for X87
fhinkel added a commit to fhinkel/node that referenced this issue Jul 21, 2016
@fhinkel
test: add regression test for instanceof
ef9092b 
See issue nodejs#7592.
evanlucas pushed a commit that referenced this issue Jul 21, 2016
@fhinkel @evanlucas
deps: cherry-pick 2aa070be from V8 upstream
e239045 
Original commit message:

    InstanceOfStub incorrectly interprets the hole as a prototype.

    Repair this to match what the runtime correctly does, by first
    checking if the function is a constructor before we access the
    prototype.

    [email protected]

    BUG=

    Committed: https://crrev.com/2aa070be4fd2960df98905b254f12ed801ef26cd

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

This fixes the behavior of instanceof when the second parameter is not a
constructor.

Fixes: #7592
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this issue Jul 21, 2016
@fhinkel @evanlucas
deps: cherry-pick 3a903c4 for PPC from V8 upstream
e5cce7a 
Original commit message:

    PPC: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected], [email protected],
    [email protected]

    BUG=

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

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

Fixes: #7592 for PPC
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this issue Jul 21, 2016
@fhinkel @evanlucas
deps: cherry-pick 5b5d24b for X87 from V8 upstream
2870061 
Original commit message:

    port 2aa070b (r34863)

    original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    BUG=

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

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

Fixes: #7592 for X87
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this issue Jul 21, 2016
@fhinkel @evanlucas
test: add regression test for instanceof
44c9a72 
Ref: #7592.
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
@fhinkel
Copy link
Member

fhinkel commented Jul 22, 2016

Fixed in #7638.

Can we close this?

@addaleax
Copy link
Member

Yup, works as expected in 6.3.1. Thanks for taking care of this!

BethGriggs pushed a commit to ibmruntimes/node that referenced this issue Jul 22, 2016
@fhinkel @BethGriggs
deps: cherry-pick 2aa070be from V8 upstream
7963103 
Original commit message:

    InstanceOfStub incorrectly interprets the hole as a prototype.

    Repair this to match what the runtime correctly does, by first
    checking if the function is a constructor before we access the
    prototype.

    [email protected]

    BUG=

    Committed: https://crrev.com/2aa070be4fd2960df98905b254f12ed801ef26cd

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

This fixes the behavior of instanceof when the second parameter is not a
constructor.

Fixes: nodejs/node#7592
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this issue Jul 22, 2016
@fhinkel @BethGriggs
deps: cherry-pick 3a903c4 for PPC from V8 upstream
3d4d0db 
Original commit message:

    PPC: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected], [email protected],
    [email protected]

    BUG=

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

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

Fixes: nodejs/node#7592 for PPC
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this issue Jul 22, 2016
@fhinkel @BethGriggs
deps: cherry-pick 5b5d24b for X87 from V8 upstream
1521a0c 
Original commit message:

    port 2aa070b (r34863)

    original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    BUG=

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

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

Fixes: nodejs/node#7592 for X87
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
richardlau added a commit to ibmruntimes/node that referenced this issue Aug 15, 2016
@richardlau
deps: cherry-pick 71d525a for S390 from V8 upstream
d051e78 
Original commit message:

    S390: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected],
    [email protected], [email protected]

    BUG=

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

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

Fixes: nodejs/node#7592 for S390
@fhinkel fhinkel mentioned this issue Oct 19, 2016
Closed
3 tasks
fhinkel added a commit to fhinkel/node that referenced this issue Oct 20, 2016
@fhinkel
test: add regression test for instanceof
f376ad4 
Add regression test for issue nodejs#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: nodejs#7638 and
nodejs#7592.
fhinkel added a commit that referenced this issue Oct 20, 2016
@fhinkel
test: add regression test for instanceof
8f8f031 
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Oct 20, 2016
@fhinkel @jasnell
test: add regression test for instanceof
d62e7bd 
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 15, 2016
@fhinkel
test: add regression test for instanceof
a038fcc 
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mscdex @fhinkel @bnoordhuis @MylesBorins @Trott @addaleax