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

Wrong error message «TypeError: something is not a function» #3934

Closed
ChALkeR opened this issue Nov 20, 2015 · 7 comments
Closed

Wrong error message «TypeError: something is not a function» #3934

ChALkeR opened this issue Nov 20, 2015 · 7 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Nov 20, 2015

for (var i = 0; i < 18436; i++);
require('http').notAFunction();

results in

require('http').notAFunction();
^
TypeError: require is not a function

But

for (var i = 0; i < 18435; i++);
require('http').notAFunction();

results in

require('http').notAFunction();
                ^
TypeError: require(...).notAFunction is not a function

For anything ≤ 18435 the error message is correct, for anything ≥ 18436 the error message is wrong.
Reproduced on v4.x and v5.1.0. If you can't reproduce that, try using some high number there.

@targos
Copy link
Member

targos commented Nov 20, 2015

I can reproduce it with d8 4.6:

function func() { return {}; }
for (var i = 0; i < 18436; i++);
func().notAFunction();
% out/native/d8 test.js
test.js:3: TypeError: func is not a function
func().notAFunction();
^
TypeError: func is not a function
    at test.js:3:8

Disabling crankshaft fixes it:

% node --no-crankshaft test.js
/home/mzasso/git/chromium/v8/test.js:3
func().notAFunction();
       ^

TypeError: func(...).notAFunction is not a function

The problem is fixed in the 4.7 branch of V8.

@rvagg
Copy link
Member

rvagg commented Nov 20, 2015

wait, what?

/cc @nodejs/v8

If we can identify the fix in V8 and a it's not too complex this it seems like a good candidate for backporting all the way to v4.x.

@targos
Copy link
Member

targos commented Nov 20, 2015

Found it !
PR incoming ;)

targos added a commit to targos/node that referenced this issue Nov 20, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: nodejs#3934
@targos
Copy link
Member

targos commented Nov 20, 2015

Fix for v5.x: #3937

@ChALkeR ChALkeR added the v8 engine Issues and PRs related to the V8 dependency. label Nov 20, 2015
targos added a commit to targos/node that referenced this issue Nov 20, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: nodejs#3934
@targos
Copy link
Member

targos commented Nov 20, 2015

Fix for v4.x: #3938

@a0viedo
Copy link
Member

a0viedo commented Nov 20, 2015

This surely got a nice score in the weirdometer. Does anyone know why the breakpoint at 18435? Looking at the fix it doesn't seem like anything trivial.

@targos
Copy link
Member

targos commented Nov 20, 2015

The problem is that without the patch, optimized code computes the wrong message location.
I don't know why the threshold is at exactly 18436, but V8 decides to optimize the function starting from this number of iterations.

@targos targos closed this as completed in ab25589 Nov 25, 2015
targos added a commit that referenced this issue Nov 25, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3938
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos added a commit that referenced this issue Dec 4, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3938
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos added a commit that referenced this issue Dec 5, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3937
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos added a commit that referenced this issue Dec 17, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3938
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos added a commit that referenced this issue Dec 23, 2015
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: #3934
PR-URL: #3938
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
Original commit message:

    Use baseline code to compute message locations.

    This switches Isolate::ComputeLocation to use baseline code when
    computing message locations. This unifies locations between optimized
    and non-optimized code by always going through the FrameSummary for
    location computation.

    [email protected]
    TEST=message/regress/regress-4266
    BUG=v8:4266
    LOG=n

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

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

Fixes: nodejs#3934
PR-URL: nodejs#3937
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants