Last Comment Bug 706240 - JS Correctness: "reference to undefined property" vs. "is not defined" errors with/without methodjit
: JS Correctness: "reference to undefined property" vs. "is not defined" errors...
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla11
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2011-11-29 13:06 PST by Christian Holler (:decoder)
Modified: 2011-12-10 20:45 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (2.69 KB, patch)
2011-12-05 13:44 PST, Brian Hackett (:bhackett)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-11-29 13:06:45 PST
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 + "})")


$ $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 User image Gary Kwong [:gkw] [:nth10sd] 2011-11-29 13:46:01 PST
Maybe bug 622271, but that bug has just been marked WFM.
Comment 2 User image Ryan VanderMeulen [:RyanVM] 2011-11-29 15:08:09 PST
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 User image Ryan VanderMeulen [:RyanVM] 2011-11-29 15:09:51 PST
FWIW, d8 also outputs "ReferenceError: z is not defined"
Comment 4 User image Brian Hackett (:bhackett) 2011-12-05 13:44:18 PST
Created attachment 579151 [details] [diff] [review]

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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-08 09:52:26 PST
Comment on attachment 579151 [details] [diff] [review]

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 User image Brian Hackett (:bhackett) 2011-12-08 19:50:36 PST
Comment 7 User image Ed Morley [:emorley] 2011-12-10 20:45:10 PST

Note You need to log in before you can comment on or make changes to this bug.