313803 - uneval() on func with embedded object with getter or setter has unmatched parenthesis

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Colin Blake]

case 1:
  obj = { get foo() { return "foo"; }};

case 2:
  func = function ff() {
    obj = { get foo() { return "foo"; }};
    return 1;
  }

Bug 306738 addresses case 1 and fixed it. Case 2 is not fixed.

js> uneval(obj)
({get foo() {return "foo";}})
js>
js>
js> uneval(func)
(function ff() {obj = {get foo () {return "foo";})};return 1;})
js>

Nota the uneval of obj has matched parens, but the uneval of func does not.

Comment 1

[Brendan Eich [:brendan]]

mrbkap has kindly offered (no rewrites for 1.8!).

/be

Comment 2

[Colin Blake]

Attachment: patch

This appears to fix the problem. Using the same test as before:

js> uneval(func)
(function ff() {obj = {get foo () {return "foo";}};return 1;})
js>
js> func
function ff() {
    obj = {get foo () {return "foo";}};
    return 1;
}
js>

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

>Index: jsopcode.c
>@@ -2427,7 +2427,13 @@
>+		if (lastop == JSOP_GETTER || lastop == JSOP_SETTER) {

Tabs Are Evil(TM)! Please don't introduce tabs into the JS engine.

>+		    char *pCloseParen = (char *) rval + strlen(rval) - 1;

Nit: leading p's are against house style. |char *closeParen| would be closer.

r=mrbkap

Comment 4

[Brendan Eich [:brendan]]

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

Cleaner way is to use %.*s for rval.  Before the Sprint, adjust rval via

    rval += strlen(js_function_str) + 1;

and use strlen(rval) - 1 as the precision.

/be

Comment 5

[Colin Blake]

Forgot about  %.*s 
Tried %*s and passing a negative width, but couldn't pass signed value into Sprint.

Sorry about the tabs. I noticed after submitting the patch and changed it locally.

New patch coming.

Comment 6

[Colin Blake]

Attachment: Revised patch

Would someone be good enough to check this in for me, if it gets the appropriate approvals.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 200833 [details] [diff] [review]
Revised patch

Colin, when you attach a patch to a bug, it's best to request review from one of the module owners or peers listed at http://www.mozilla.org/owners.html .

>+                    rval += strlen(js_function_str) + 1;

I think this is OK, since only the todo value is used after the switch statement.

>+                                  strlen(rval)-1,

Uber-nit: spaces around the -.

r=mrbkap

Comment 8

[timeless]

Comment on attachment 200833 [details] [diff] [review]
Revised patch

carrying forward sr=brendan

requesting approval for 1.8 branch, the change is small and fixes a problem that was being experienced by an embedder that is using the 1.8 branch.

Comment 9

[Brendan Eich [:brendan]]

This is trivially safe and hard to work around.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 200833 [details] [diff] [review]
Revised patch

Risks: compiler bugs biting movement of code, %.*s not working in some vsprintf C runtime -- both zero in known platforms.

"Not stop-ship" but yet another case of quality being served by allowing safe fixes for bugs that are ugly and/or hard to workaround.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

Checking in regress-313803.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-313803.js,v  <--  regress-313803.js
initial revision: 1.1
done

Comment 12

[Sébastien Auger]

it's safe enought for the branch, but i don't know if it's possible to accept this right now during the lockdown. RC2 ??

Comment 13

[Asa Dotzler [:asa]]

not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.

Comment 14

[Scott MacGregor]

Blake, is this something that can land on the trunk while we are waiting for potential branch ride along approval?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

timeless checked this in already:

2005-10-25 22:23	timeless%mozdev.org 	mozilla/ js/ src/ jsopcode.c 	3.98 	5/3  	Bug 313803 uneval() on func with embedded object with getter or setter has unmatched parenthesis patch by [e-mail] r=mrbkap sr=brendan

Marking FIXED to match that.

Comment 16

[Asa Dotzler [:asa]]

moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.

Comment 19

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-313803.js began to time out in browser tests on 1.8.0, 1.8 opt/dbg linux and 1.9 opt windows and 1.9 dbg linux on 20060606. This is a change from prior runs which did not. This may be due to other programs Jesse was running at the same time on plum... Not sure why the windows (peach) timed out as I can't reproduce locally on winxp.