JS decompiler: i++, i-- fail when |i| is a property of a "cloned" function

VERIFIED FIXED in mozilla1.2beta

Status

()

P1
critical
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: desale, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.2beta
x86
Windows XP
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

16 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2002-09-09-08-trunk/

"counter++" increment fails (in certain circumstances) in situation where
"++counter" &  "counter = counter + 1" works fine & counght exception reports
that "counter" is not defined.

IMPORTANT:
This fails only on trunk builds. This works perfect in NS 7.0 RTM Branch.
I tested it on 2002-09-09-08-trunk & it fails. (I tested on WinXP)
I also tested it on NS 7.0 RTM & it works fine there.


Reproducible: Always

Steps to Reproduce:
1.Please load testcase I'm going to attach.
2.click button "Start Test".

Actual Results:  
You see 4 alerts showing following messages.
First Alert: "Starting test".
Second Alert: "callCount = callCount + 1 Succeeded".
Third Alert: "callCount++ failed with ReferenceError: callCount is not defined".
Fourth Alert: "test ended".

(Third alert shows actual failure)

Expected Results:  
You should see 4 alerts showing following messages.
First Alert: "Starting test".
Second Alert: "callCount = callCount + 1 Succeeded".
Third Alert: "callCount++ Succeeded".
Fourth Alert: "test ended".

This bug blocks DIG testbed because this is how our testbed calculates method
coverage.

TESTCASE CODE FLOW: (Writing just incase its needed & it makes easier to
understand).

"makeCoverFunction" is a function that creates a new function using the 
function body of the "foo" function as a string.

so we first call "makeCoverFunction" to create a new function. 
"makeCoverFunction" also hangs off a variable "callCount" to the newly 
created function and returns it.

we then call the newly created function.
(Reporter)

Comment 1

16 years ago
Created attachment 98953 [details]
Testcase.

Comment 2

16 years ago
Confirming bug with Mozilla trunk binary 2002091110 on WinNT.
As Prashant says, do not see bug with a 1.0 binary 20020823xx.

I will attach expanded testcases below. The first one has:

function F()
{
  var f = arguments.callee;
  f.i = 0;
  showThis('At beg of function \ti = ' + f.i);

  try
  {
    f.i = f.i + 1
    showThis('i = i+1 succeeded \ti = ' + f.i);
  }
  catch(e)
  {
    showThis('i = i+1 failed with ' + e + '\ti = ' + f.i);
  }

                      etc.
                      etc.

We alter the property f.i in different ways, including ++f.i, f.i++,
and also --f.i, f.i--. When we call F() directly, all these succeed.

But if we make a "clone" G = (schematically) new Function(F.toString()),
the trunk and branch builds differ. The branch build (and IE6) succeed
on all the increments and decrements.

The trunk build succeeds on pre-increment ++f.i, pre-decrement --f.i,
but fails with a ReferenceError on post-increment f.i++, and f.i--.
Again, this only occurs with the "clone", and only in the trunk build.


The second testcase is like the first, only it uses a local variable
|var i = 0;| instead of creating a property |f.i = 0|. This testcase
succeeds on both trunk and branch -
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: "counter++" increment fails in situation where "++counter" & "counter = counter + 1" works fine. → i++, i-- fail when |i| is a property of a "cloned" function

Comment 3

16 years ago
Created attachment 98983 [details]
HTML testcase with |f.i = 0|

Comment 4

16 years ago
Created attachment 98984 [details]
HTML testcase with |var i = 0|

Comment 5

16 years ago
Created attachment 98986 [details]
JS shell testcase with |f.i = 0|
(Assignee)

Comment 6

16 years ago
Isn't this testcase invalid?  I see i.i++ and i.i--, not f.i++ and f.i--.

/be

Comment 7

16 years ago
Let me look into that; in the meantime -

OUTPUT

------------ CURRENT RHINO SHELL, MOZILLA 1.0 BUILD,  IE6  ------------
Calling F():

At beg of function 	i = 0
i = i+1 succeeded 	i = 1
++i succeeded 		i = 2
i++ succeeded 		i = 3
++i succeeded 		i = 4
--i succeeded 		i = 3
i-- succeeded 		i = 2


Calling G(), cloned from F:

At beg of function 	i = 0
i = i+1 succeeded 	i = 1
++i succeeded 		i = 2
i++ succeeded 		i = 3
++i succeeded 		i = 4
--i succeeded 		i = 3
i-- succeeded 		i = 2



------------ CURRENT JS SHELL, CURRENT MOZILLA TRUNK BUILD ------------ 
Calling F():

At beg of function      i = 0
i = i+1 succeeded       i = 1
++i succeeded           i = 2
i++ succeeded           i = 3
++i succeeded           i = 4
--i succeeded           i = 3
i-- succeeded           i = 2


Calling G(), cloned from F:

At beg of function      i = 0
i = i+1 succeeded       i = 1
++i succeeded           i = 2
i++ failed with ReferenceError: i is not defined        i = 2
++i succeeded           i = 3
--i succeeded           i = 2
i-- failed with ReferenceError: i is not defined        i = 2

Comment 8

16 years ago
>  I see i.i++ and i.i--, not f.i++ and f.i--

In the attachments above, I can't see i.i anywhere, only f.i.
Let me know if there is a typo somewhere I'm missing -

Comment 9

16 years ago
Brendan has found what's going wrong. But it's not an error in the
testcase, but rather in F.toString().

The actual source of the function F() has f.i++, but F.toString()
has i.i++ instead!!! That's what's making the clone G() fail!

To see this, load the HTML testcase with |f.i = 0|, and type

            javascript: alert(F.toString())

It's not the same as the definition of F() in the testcase!

Note i.i++ also shows up when we click the testcase's "Show Function"
button, because my display function is also relying on F.toString().
(Assignee)

Comment 10

16 years ago
Yes, sorry, I was looking at the output of F.toString() (F\n in the js shell
after loading the testcase).  Taking for fast turnaround,

/be
Assignee: khanson → brendan
(Assignee)

Comment 11

16 years ago
Oops, this is a bug introduced by the fix for bug 58274.  You can't POP_STR and
then QuoteString without the latter overwriting the former's string-stack space,
resulting in the i.i-- and i.i++ instead of f.i-- and f.i++ (f was popped, then
i was pushed in its place).  Fix coming up.

Good testcase, Prashant!  Not a blocker, though.

/be
Severity: blocker → critical
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla1.2
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 12

16 years ago
Created attachment 99029 [details] [diff] [review]
proposed fix

QuoteString is tricky, it steals sprinter space above the last pushed string
(when the sprinter you pass to it is owned by a sprint-stack), even if you just
popped something off the sprint-stack and want to use the sprinted space later.
 So order matters.

/be
Comment on attachment 99029 [details] [diff] [review]
proposed fix

If it were up to me, anything this "tricky" would be dragged behind the space
shuttle for a few launches.  r=shaver, though, for restoring the delicate
balance.
Attachment #99029 - Flags: review+
(Assignee)

Comment 14

16 years ago
rogerl, I shoulda caught this when reviewing bug 58274.  The decompiler is lean
and mean, more mean than lean I suppose.  It goes back to a time when I didn't
want to spend too much code space or dynamic memory on the silly but
occasionally useful task of decompilation.  QuoteString could be "safened" into
something that returns a malloc'ed copy, but that'd just clutter up all the
callsites.  I prefer to leave things delicately well-ordered, to keep all of us
on our toes :-).

Fixed on the trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Summary: i++, i-- fail when |i| is a property of a "cloned" function → JS decompiler: i++, i-- fail when |i| is a property of a "cloned" function
(Reporter)

Comment 15

16 years ago
Thanks for taking faster action on this one Brendan. It was blocking our DIG 
testing on trunk.
Hopefully everything will start working fine after this fix at our end.
(Reporter)

Comment 16

16 years ago
Thanks for taking faster action on this one Brendan. It was blocking our DIG 
testing on trunk.
Hopefully everything will start working fine after this fix at our end.
(Assignee)

Comment 17

16 years ago
Things could be better still:

(1) Some QuoteString calls (the one in DecompileSwitch in particular) lacked a
subsequent POP_STR or equivalent operation to retract the sprinter into whose
buffer QuoteString had stuffed chars, which could lead to bloat.

(2) Some old JS_strdup copying from the sprinter's buffer into the malloc heap
(temporarily) can be avoided by taking advantage of the QuoteString feature
(yes, feature) that leaves the sprinter's offset advanced after the quoted chars
(which requires eventually popping or otherwise retracting the sprinter).

rogerl's patch to bug 58274 took advantage of the QuoteString feature mentioned
in (2), in js_DecompileFunction.  But the code for JSOP_NAMEINC/DEC never did. 
Instead of (schematically, not all args/error-checks shown, "++" is e.g. only):

    rval = QuoteString(&ss->sprinter, lval, 0);
    SprintPut(&ss->sprinter, "++");
    todo = STR2OFF(&ss->sprinter, rval);

the old code went something like this:

    rval = QuoteString(&ss->sprinter, lval, 0);
    todo = Sprint(&ss->sprinter, "%s++", lval);

That bloats ss->sprinter's buffer with the quoted string hiding out underneath
the same string followed by ++ in ss->sprinter's buffer (todo is then pushed on
the sprint-stack's offset stack; we store offsets so we can realloc without
invalidating pointers).

I hope you guys get the drift of the upcoming patch and give it a review.  Maybe
rogerl will take the plunge this time.  The water's great!  :-)

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

16 years ago
Created attachment 99097 [details] [diff] [review]
POP_STR, SET_STR, or STR2OFF after all QuoteString calls from Decompile{,Switch}

If you always POP_STR after a QuoteString call that stores quoted chars after
another pushed string, the pop for that pushed string will pop the quoted
string too (the push stored a NUL, three actually, after the pushed string, so
the two won't run together).  If you aren'g going to pop, then retract using
SET_STR -- unless you *want* the quoted chars appended to the string stack's
sprinter's buffer (whew), in which case you probably want todo = STR2OFF(...,
rval) or whatever, so that the main decompiler loop will push that "todo"
offset.

Clear as mud, eh?  BTW, this is a diff -w -u9; I fixed only the tabs on
affected lines.

I tested the affected cases by hand.  All good.

/be
(Assignee)

Comment 19

16 years ago
One point that may not be clear: Sprint takes a printf-style format and uses
JS_smprintf to create a formatted string, then stores that in the sprinter using
SprintPut.  So it's safe to QuoteString();POP_STR();POP_STR();todo=Sprint(...)
where ... uses the POP_STR results.  The sprinter buffer won't be overwritten
until all the popped stuff (including the initial or uppermost quoted string)
has been converted by JS_smprintf inside Sprint.

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 20

16 years ago
To any lurkers, I hasten to point out that all bloat mentioned in comments here
was temporary, because the decompiler uses arena-pools for LIFO allocation, so
there were no leaks that I know of, and no long-lived bloat.  But even short
term bloat can thrash your system and leave too high a data segment watermark.

/be

Comment 21

16 years ago
Comment on attachment 99097 [details] [diff] [review]
POP_STR, SET_STR, or STR2OFF after all QuoteString calls from Decompile{,Switch}

Muddy though the waters be, r=rogerl
Attachment #99097 - Flags: review+
(Assignee)

Comment 22

16 years ago
Fixed for good, I hope.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 23

16 years ago
Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-168347.js

Comment 24

16 years ago
Verified Fixed -

JS shell testcase now passes in both the debug and optimized JS shell.
Furthermore, no test regressions have been introduced by the patch.

Before the fix, the test used to fail as follows, reflecting
the mixup between f.i++ and i.i++,  f.i-- and i.i-- 

FAILED!: Section 3 of test -

Expected value 'etc. etc.   try{f.i++;print("i++succeeded   etc. etc. '
Actual value   'etc. etc.   try{i.i++;print("i++succeeded   etc. etc. '

FAILED!: Section 4 of test -

Expected value 'etc. etc.   try{f.i--;print("i--'
Actual value   'etc. etc.   try{i.i--;print("i--'
Status: RESOLVED → VERIFIED

Updated

14 years ago
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.