The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Colin Blake, Assigned: mrbkap)

Tracking

({verified1.8})

Trunk
x86
Windows XP
verified1.8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.16 KB, patch
mrbkap
: review+
timeless
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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
(Reporter)

Comment 2

12 years ago
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>
(Assignee)

Comment 3

12 years ago
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-
(Reporter)

Comment 5

12 years ago
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.
(Reporter)

Comment 6

12 years ago
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.
Attachment #200820 - Attachment is obsolete: true
(Assignee)

Comment 7

12 years ago
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 8

12 years ago
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

Comment 11

12 years ago
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+

Comment 12

12 years ago
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

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

Updated

12 years ago
Flags: blocking1.8rc1+ → blocking1.8rc1?

Comment 14

12 years ago
Blake, is this something that can land on the trunk while we are waiting for potential branch ride along approval?
(Assignee)

Comment 15

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: blocking1.8rc2?

Comment 16

12 years ago
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Flags: blocking1.8rc1?

Updated

12 years ago
Attachment #200833 - Flags: approval1.8rc1? → approval1.8rc2+
(Assignee)

Comment 17

12 years ago
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8

Updated

12 years ago
Flags: blocking1.8rc2?

Comment 18

11 years ago
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8

Comment 19

11 years ago
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.