Closed Bug 1099054 Opened 5 years ago Closed 3 years ago

Misleading TypeError

Categories

(Core :: JavaScript Engine, defect)

33 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: michalwadas, Assigned: jandem)

Details

Attachments

(1 file, 1 obsolete file)

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
Still seeing this problem in Firefox 34.0.
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.
Attached patch Patch (obsolete) — Splinter Review
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
Assignee: nobody → jdemooij
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jorendorff)
(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 &&.
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.
Flags: needinfo?(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.
(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...
Attached patch PatchSplinter Review
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.
Attachment #8567845 - Attachment is obsolete: true
Attachment #8809356 - Flags: review?(nicolas.b.pierron)
(In reply to Jan de Mooij [:jandem] from comment #8)
> the |*valuepc = nullptr| check

Er, s/check/assignment/
Attachment #8809356 - Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/f0103ae8bdd0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.