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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Prashant Desale]

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.

Comment 1

[Prashant Desale]

Attachment: Testcase.

Comment 2

[Phil Schwartau]

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 -

Comment 3

[Phil Schwartau]

Attachment: HTML testcase with |f.i = 0|

Comment 4

[Phil Schwartau]

Attachment: HTML testcase with |var i = 0|

Comment 5

[Phil Schwartau]

Attachment: JS shell testcase with |f.i = 0|

Comment 6

[Brendan Eich [:brendan]]

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

/be

Comment 7

[Phil Schwartau]

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

[Phil Schwartau]

>  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

[Phil Schwartau]

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().

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Brendan Eich [:brendan]]

Attachment: 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 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Prashant Desale]

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.

Comment 16

[Prashant Desale]

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.

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Brendan Eich [:brendan]]

Attachment: 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

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[Brendan Eich [:brendan]]

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

[rogerl (gone)]

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

Comment 22

[Brendan Eich [:brendan]]

Fixed for good, I hope.

/be

Comment 23

[Phil Schwartau]

Testcase added to JS testsuite:

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

Comment 24

[Phil Schwartau]

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--'