381205 - object uneval gets confused by special "getter functions"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> getter function p() { print(4) }
js> uneval({x getter: this.__lookupGetter__("p")})
({get x ction p() {print(4);})

I think it's trying to remove "function", but the first 8 characters are "getter f".  And it's also confused by it being a getter for "x" and being a getter for "p" at the same time.

Creating the object this way should probably be a runtime type error, just like:

js> j = 3; ({x getter: j})
typein:3: SyntaxError: invalid getter usage
(It says "SyntaxError", but it's really a runtime type error, see bug 352316.)

Comment 1

[Jesse Ruderman]

Much happier now that bug 367629 is fixed:

js> getter function p() { print(4) }
js> uneval({x getter: this.__lookupGetter__("p")})
({get x () {print(4);}})

Comment 2

[Bob Clary [:bc] (inactive)]

Checking in regress-381205.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-381205.js,v  <--  regress-381205.js
initial revision: 1.1

with SyntaxError instead of TypeError

Jesse, do you really not get an error as in comment 1?

Comment 3

[Jesse Ruderman]

When I run the commands in comment 1, I don't get any errors.  (Is that what you were asking?)

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to comment #3)
> When I run the commands in comment 1, I don't get any errors.  (Is that what
> you were asking?)
> 

Yes, but I am still confused. 

js> getter function p() { print(4) }
js> uneval({x getter: this.__lookupGetter__("p")})
({get x () {print(4);})

js> function f() { getter function p() { print(4) }; uneval({x getter: this.__lookupGetter__("p")}) }
js> f()
typein:1: SyntaxError: invalid getter usage

Comment 5

[Jesse Ruderman]

In the latter, this.__lookupGetter__("p") returns |undefined| because "p" is only bound within f's scope.  You'd see the same thing with a normal function or a local variable.

js> function f() { getter function p() { }; print(this.__lookupGetter__("p")); }
js> f()
undefined

Comment 6

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-381205.js,v  <--  regress-381205.js
new revision: 1.2; previous revision: 1.1

Comment 7

[Bob Clary [:bc] (inactive)]

New results are:

Expected value '({get x () {print(4);})', 
Actual value   '({get x function p() {print(4);}})'

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Oops, I bet looking for the first space finds the space after 'getter'.

Comment 9

[Brian Crowder]

Attachment: using getter/setter flags on the function to decide whether to skip "getter"/"setter"

I hate how this routine seems to grow with each tweak.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 266646 [details] [diff] [review]
using getter/setter flags on the function to decide whether to skip "getter"/"setter"

>Index: jsobj.c
>+                if (JSFUN_GETTER_TEST(fun->flags) ||
>+                    JSFUN_SETTER_TEST(fun->flags)) /* skip "getter/setter" */
>+                    vchars = js_strchr_limit(vchars, ' ', end) + 1;

Add braces around the if body and r=mrbkap.

At least we're converging on something sane!

Comment 11

[Brian Crowder]

Attachment: Added a null-check and a check for a space character where I expect one

Sorry for the review-harassment mrbkap, trying to reassure my paranoid self that this is as memory-safe as it can be.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 266654 [details] [diff] [review]
Added a null-check and a check for a space character where I expect one

>Index: jsobj.c
>+                JS_ASSERT(fun);

In general, we these sorts of assertions are rare, since we'll catch this sort of problem by null-deref crashing down below, so I'd say "take it out!".

Comment 13

[Brian Crowder]

jsobj.c: 3.345

Comment 14

[Bob Clary [:bc] (inactive)]

I assume that actual: ({get x p() {print(4);}}) is ok for the decompilation now?

Comment 15

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-381205.js,v  <--  regress-381205.js
new revision: 1.3; previous revision: 1.2

changed expected decompilation and fixed a missing brace in the expected value.

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows but the test fails due to bug 393269