706240 - JS Correctness: "reference to undefined property" vs. "is not defined" errors with/without methodjit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test produces two different ReferenceError versions with options "-m -a" vs. no options on mozilla-central revision e320f9f5536f:


function f(code) {
    a |= code.replace(/s/, "");
    f = eval("(function(){" + code + "})")
}
f("switch(''){default:break;/*DUPTRY525*/}")


Output:

$ $JS -m -a min.js 
min.js:2: ReferenceError: a is not defined
$ $JS min.js 
min.js:2: ReferenceError: reference to undefined property "a"

I remember that we had a bug open about this at some point but I can't find it. It would be good to fix these inconsistencies to make differential fuzzing results more reliable.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Maybe bug 622271, but that bug has just been marked WFM.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

You're right, js -a -m -n does indeed output "ReferenceError: z is not defined" in the bug 622271 testcase. I'll leave it up to you to decide whether to re-open that bug or just to deal with it here.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

FWIW, d8 also outputs "ReferenceError: z is not defined"

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

The method JIT is doing the right thing here, this should get reported as an undefined name and not an undefined property.  When executing a GETXPROP in the interpreter, the getProperty treats it as a property access when reporting the error (despite knowing it is at a GETXPROP opcode).

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 579151 [details] [diff] [review]
patch

Review of attachment 579151 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +5941,1 @@
>              } else {

If the consequent block always completes by returning, the else-block doesn't need to be 1) indented or 2) an else-block.  Remove the else, de-indent the block contents, and separate the two with a blank line.

@@ +5966,5 @@
> +
> +                /* Ok, bad undefined property reference: whine about it. */
> +                if (!js_ReportValueErrorFlags(cx, flags, JSMSG_UNDEFINED_PROP,
> +                                              JSDVG_IGNORE_STACK, IdToValue(id),
> +                                              NULL, NULL, NULL)) {

{ on new line, please.

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c77d88e54bb3

Comment 7

[Ed Morley [:emorley]]

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