Last Comment Bug 313803 - uneval() on func with embedded object with getter or setter has unmatched parenthesis
: uneval() on func with embedded object with getter or setter has unmatched par...
Status: VERIFIED FIXED
: verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-25 14:26 PDT by Colin Blake
Modified: 2006-06-08 03:06 PDT (History)
7 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.27 KB, patch)
2005-10-25 18:14 PDT, Colin Blake
mrbkap: review+
brendan: superreview-
Details | Diff | Splinter Review
Revised patch (1.16 KB, patch)
2005-10-25 21:23 PDT, Colin Blake
mrbkap: review+
timeless: superreview+
asa: approval1.8rc2+
Details | Diff | Splinter Review

Description Colin Blake 2005-10-25 14:26:05 PDT
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] 2005-10-25 18:02:27 PDT
mrbkap has kindly offered (no rewrites for 1.8!).

/be
Comment 2 Colin Blake 2005-10-25 18:14:37 PDT
Created attachment 200820 [details] [diff] [review]
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) 2005-10-25 18:39:47 PDT
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] 2005-10-25 18:58:58 PDT
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 2005-10-25 19:09:00 PDT
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 2005-10-25 21:23:48 PDT
Created attachment 200833 [details] [diff] [review]
Revised patch

Would someone be good enough to check this in for me, if it gets the appropriate approvals.
Comment 7 Blake Kaplan (:mrbkap) 2005-10-25 22:06:05 PDT
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 2005-10-25 22:26:39 PDT
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] 2005-10-25 23:25:55 PDT
This is trivially safe and hard to work around.

/be
Comment 10 Brendan Eich [:brendan] 2005-10-25 23:29:39 PDT
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:] 2005-10-26 00:57:37 PDT
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 2005-10-26 06:43:34 PDT
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] 2005-10-26 11:19:54 PDT
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 2005-10-28 16:28:40 PDT
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) 2005-10-28 16:37:24 PDT
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 colin@theblakes.com r=mrbkap sr=brendan

Marking FIXED to match that.
Comment 16 Asa Dotzler [:asa] 2005-10-31 14:47:09 PST
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) 2005-10-31 16:53:52 PST
Checked into MOZILLA_1_8_BRANCH.
Comment 18 Bob Clary [:bc:] 2006-04-09 21:25:47 PDT
verified fixed 1.8.x and trunk.
Comment 19 Bob Clary [:bc:] 2006-06-08 03:06:37 PDT
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.

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