1099054 - Misleading TypeError

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[michalwadas]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141013200110

Steps to reproduce:

I typed to console:

 (1||eval)('3')



Actual results:

 TypeError: eval is not a function


Expected results:

 TypeError: number is not a function

Comment 1

[scmcmurray]

Still seeing this problem in Firefox 34.0.

Comment 2

[Jeff Walden [:Waldo]]

The expression decompiler, that tries to produce more helpful error messages, is irredeemably broken in some cases.  This appears to be but one of them.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch fixes the decompiler to handle branches correctly. For (1||eval)('3') we now report:

    TypeError: (intermediate value)(...) is not a function

instead of:

    TypeError: eval is not a function

Whether "intermediate value" is more helpful than just "number" or "1" in this case is debatable, but that's another issue.

However, it also fails two tests in expression-autopsy.js:

  check_one(`(intermediate value)[Symbol.iterator](...).next(...).value`,
            function () { var [{ x }] = [null, {}]; }, " is null");
  check_one(`(intermediate value)[Symbol.iterator](...).next(...).value`,
            function () { ieval("let (x) { var [a, b, [c0, c1]] = [x, x, x]; }") }, " is undefined");

  Error: Assertion failed: got "(intermediate value).x", expected "(intermediate value)[Symbol.iterator](...).next(...).value"

This is because the destructuring also uses branches:

  00039:  getprop "done"
  00044:  ifeq 56 (+12)
  00049:  pop
  00050:  undefined
  00051:  goto 61 (+10)
  00056:  getprop "value"
  00061:  dup
  00062:  getprop "x"
  00067:  setlocal 0

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> This patch fixes the decompiler to handle branches correctly. For
> (1||eval)('3') we now report:
> 
>     TypeError: (intermediate value)(...) is not a function

Hm, the bogus "(...)" is from this in FindStartPC:

    jsbytecode *pc = parser.pcForStackOperand(current, spindex);
    *valuepc = pc ? pc : current;

I think we should set *valuepc to nullptr if !pc. With that, we get:

    TypeError: 1 is not a function

That's probably the best we can do here without teaching the decompiler more about || and &&.

Comment 5

[Jason Orendorff [:jorendorff]]

But the decompiler *does* know about || and &&!

IsJumpOpcode(JSOP_AND) is true, so we should get a call to addJump() which should result in a later call to mergeOffsetStack(), which should blank out (with UINT32_MAX) the stack slot containing "eval".

Looking into it.

Comment 6

[Jason Orendorff [:jorendorff]]

addJump() and mergeOffsetStack() seem to be working as desired, but BytecodeParser::parse's own copy of the stack, offsetStack, is never merged with other stacks when we reach a join point in the code.

The issue Jan points out in comment 4 looks like a separate issue that should also be fixed.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> addJump() and mergeOffsetStack() seem to be working as desired, but
> BytecodeParser::parse's own copy of the stack, offsetStack, is never merged
> with other stacks when we reach a join point in the code.

Yes, the patch I attached fixes that but breaks the jit-tests I mentioned...

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

A few years later, but here's a patch for the issue in comment 4.

Without the patch, the test fails with "(intermediate value)(...) is not a function", which makes no sense. If we can't figure out the pc that pushed the value, it seems better to disable the decompiler instead of assuming it was the current pc (this often gives bogus results, like here).

I also moved the |*valuepc = nullptr| check to the top of the function - if a frame is running in Ion or something we also don't want to assume the value was pushed by the current pc.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> the |*valuepc = nullptr| check

Er, s/check/assignment/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0103ae8bdd0
Fix bogus error messages caused by the decompiler falling back to the current pc. r=nbp

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0103ae8bdd0