Closed
Bug 313803
Opened 20 years ago
Closed 20 years ago
uneval() on func with embedded object with getter or setter has unmatched parenthesis
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: colin, Assigned: mrbkap)
Details
(Keywords: verified1.8)
Attachments
(1 file, 1 obsolete file)
1.16 KB,
patch
|
mrbkap
:
review+
timeless
:
superreview+
asa
:
approval1.8rc2+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
mrbkap has kindly offered (no rewrites for 1.8!).
/be
Assignee: general → mrbkap
Reporter | ||
Comment 2•20 years ago
|
||
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•20 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 4•20 years ago
|
||
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•20 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•20 years ago
|
||
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•20 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 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?
Comment 10•20 years ago
|
||
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•20 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•20 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•20 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•20 years ago
|
Flags: blocking1.8rc1+ → blocking1.8rc1?
Comment 14•20 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•20 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
Closed: 20 years ago
Resolution: --- → FIXED
Comment 16•20 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•20 years ago
|
Attachment #200833 -
Flags: approval1.8rc1? → approval1.8rc2+
Updated•20 years ago
|
Flags: blocking1.8rc2?
Comment 18•19 years ago
|
||
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8
Comment 19•19 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.
Description
•