Closed Bug 313803 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: colin, Assigned: mrbkap)

Details

(Keywords: verified1.8)

Attachments

(1 file, 1 obsolete file)

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.
mrbkap has kindly offered (no rewrites for 1.8!).

/be
Assignee: general → mrbkap
Attached patch patch (obsolete) — Splinter Review
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 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
Attachment #200820 - Flags: superreview?(brendan)
Attachment #200820 - Flags: review+
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
Attachment #200820 - Flags: superreview?(brendan) → superreview-
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.
Attached patch Revised patchSplinter Review
Would someone be good enough to check this in for me, if it gets the appropriate approvals.
Attachment #200820 - Attachment is obsolete: true
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
Attachment #200833 - Flags: superreview?(brendan)
Attachment #200833 - Flags: review+
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.
Attachment #200833 - Flags: superreview?(brendan)
Attachment #200833 - Flags: superreview+
Attachment #200833 - Flags: approval1.8rc1?
This is trivially safe and hard to work around.

/be
Flags: blocking1.8rc1+
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
Checking in regress-313803.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-313803.js,v  <--  regress-313803.js
initial revision: 1.1
done
Flags: testcase+
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 ??
not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.
Flags: blocking1.8rc1+ → blocking1.8rc1?
Blake, is this something that can land on the trunk while we are waiting for potential branch ride along approval?
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: blocking1.8rc2?
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Flags: blocking1.8rc1?
Attachment #200833 - Flags: approval1.8rc1? → approval1.8rc2+
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Flags: blocking1.8rc2?
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8verified1.8
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.
You need to log in before you can comment on or make changes to this bug.