Last Comment Bug 418128 - Yet another GC hazard with ++/-- in js_Interpret
: Yet another GC hazard with ++/-- in js_Interpret
Status: VERIFIED FIXED
[sg:critical?] trunk fixed in bug 418641
: verified1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: P2 critical (vote)
: mozilla1.9
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-17 13:33 PST by Igor Bukanov
Modified: 2008-07-03 07:29 PDT (History)
6 users (show)
dsicore: blocking1.9+
dveditz: blocking1.8.1.13-
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (17.54 KB, patch)
2008-02-19 09:53 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
js1_5/GC/regress-418128.js (2.54 KB, text/plain)
2008-03-29 14:53 PDT, Bob Clary [:bc:]
no flags Details
fix for 1.8 branch (11.56 KB, patch)
2008-06-04 13:41 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix for 1.8 branch (11.56 KB, patch)
2008-06-06 04:18 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix for 1.8 branch v2 for real (11.44 KB, patch)
2008-06-06 04:24 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review
1.8.0 attempt (10.25 KB, patch)
2008-06-16 03:55 PDT, Alexander Sack
asac: review-
Details | Diff | Splinter Review
1.8 to 1.8.0 plaindiff (6.17 KB, patch)
2008-06-16 04:00 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
1.8.0 attempt 2 (10.84 KB, patch)
2008-06-16 04:29 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
1.8 to 1.8.0 plaindiff (6.45 KB, patch)
2008-06-16 04:30 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
1.8.0 attempt 2 + comments (10.74 KB, patch)
2008-06-16 07:45 PDT, Alexander Sack
igor: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2008-02-17 13:33:15 PST
The current implementation of ++ and -- operators in js_Interpret to increase/decrease indexed property of an object does not root the id of the property. This leads to a GC hazard when code tries to save the result back into object as the following example demonstrates:

~/m/ff/mozilla/js/src $ cat ~/m/y.js
var obj = {};
var id = { toString: function() { return ""+Math.pow(2, 0.1); } }
obj[id] = { valueOf: unrooter };
print(obj[id]++);
gc();
print(uneval(obj));

function unrooter()
{
    delete obj[id];
    gc();
    return 10;
}
~/m/ff/mozilla/js/src $ ~/m/ff/mozilla/js/src/Linux_All_OPT.OBJ/js ~/m/y.js
before 16384, after 16384, break 09ffc000
10
before 16384, after 16384, break 09ffc000
segmentation fault
Comment 1 Igor Bukanov 2008-02-17 14:43:43 PST
There is another hazard in the ++/-- implementation. The code in js_Interpret that stores the result contains:

            ok = OBJ_SET_PROPERTY(cx, obj, id, &rval);
...
            PUSH_OPND(rtmp);

Here for pre "++" and "--" operations 

   rval == rtmp == incremenetd_or_decremented_value. 

When rval does not fit the int range, it will be a newborn double representing the result. So this code assumes that after the property setter returns, rval still will be alive GC thing that can be stored back to the stack. But this is not true in general. Consider array_length_setter from jsarray.c. It contains the following code:

    if (!ValueIsLength(cx, *vp, &newlen))
        return JS_FALSE;
    if (!js_GetLengthProperty(cx, obj, &oldlen))
        return JS_FALSE;
    if (oldlen > newlen) {
        if (oldlen - newlen < (1 << 24)) {
            do {
                --oldlen;
                if (!JS_CHECK_OPERATION_LIMIT(cx, JSOW_JUMP) ||
                    !DeleteArrayElement(cx, obj, oldlen)) {
                    return JS_FALSE;
                }
            } while (oldlen != newlen);
...
    }
    if (!IndexToValue(cx, newlen, vp))
        return JS_FALSE;
    STOBJ_SET_SLOT(obj, JSSLOT_ARRAY_LENGTH, *vp);
 
Here vp is &rval. When oldlen > newlen, the code can call operational callback. That in turn can call the full GC collecting the context of vp. 
In such situation when the setter returns, rtemp, the original value of rval, will refer to the collected value and the code will push it back into JS stack. I.e., if the operation callback runs the GC, then the following example will store the dead value into x:

var a = Array(1 << 30 + 1);
var x = --a.length;

Note that there is no hazard in the array code as vp is never read again after a potential GC call. It is precisely the ++/-- implementation that introduces the hazard when it accesses weakly rooted value after calling a setter.
Comment 2 Igor Bukanov 2008-02-19 09:53:36 PST
Created attachment 304256 [details] [diff] [review]
v1

Untested patch. To fix the first hazard, the patch always roots the result of FETCH_ELEMT_ID. To fix the second hazard the patch makes sure preincrements also includes JOF_TMPSLOT.

Besides the fix the patch also moves non-int increment and decrement to separated functions to share and simplify the code for this infrequently taken path. This shrinks the size of stripped jsinterp.o on Linux by 2K.

The patch is not tested.
Comment 3 Igor Bukanov 2008-02-20 09:52:39 PST
I filed bug 418641 as a cover bug for this.
Comment 4 Igor Bukanov 2008-02-26 14:01:08 PST
The new patch from bug 418641 comment 4 fixes this. It never passes pointers to unrooted values to getters/setters. This may seems excessive since the rest of code still passes &rval/&rtmp to the native functions. But given the history of hazards, I prefer not to rely on weak roots at all. 

Plus in fact &rval/&rtmp are expensive code wise since they force the compiler to assume that rval/rtmp can be modified by arbitrary native functions. 

On the other hand &sp is not exposed so sp lives most of the time in registers. Thus passing &rval and &sp[-1] and using rval or sp[-1] generates the same amount of code. In the former case the compiler will use native_stack_register[offset_for_rval] and in the latter case  register_storing_sp[-1].
Comment 5 Daniel Veditz [:dveditz] 2008-02-27 11:57:52 PST
Patch is kinda big and scary... will this land on the trunk anytime soon so we can check for regressions?
Comment 6 Igor Bukanov 2008-02-27 12:47:33 PST
(In reply to comment #5)
> Patch is kinda big and scary... will this land on the trunk anytime soon so we
> can check for regressions?

Note that the branch would require rather different patch as there rooting id is not trivial. So I will go there for a minimal patch that uses JS_KEEP_ATOMS for rooting the id and will apply JSOF_TEMPSLOT for index and property operations. But I do plan to replace the main macro implementing ++/-- for non-integer cases by a function to follow the trunk.

Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-27 14:11:18 PST
Moving bugs that aren't beta 4 blockers to target final release.
Comment 8 Daniel Veditz [:dveditz] 2008-03-03 11:49:55 PST
Since the trunk patch hasn't been reviewed or landed on trunk we'll likely have to wait for next branch release to catch this one. Minusing for '13
Comment 9 Igor Bukanov 2008-03-18 07:40:18 PDT
This is fixed on the trunk since landing bug 418641.
Comment 10 Bob Clary [:bc:] 2008-03-29 14:53:32 PDT
Created attachment 312529 [details]
js1_5/GC/regress-418128.js
Comment 11 Bob Clary [:bc:] 2008-04-17 13:44:17 PDT
v 1.9.0
Comment 12 Daniel Veditz [:dveditz] 2008-04-23 11:20:32 PDT
(In reply to comment #9)
> This is fixed on the trunk since landing bug 418641.

Does that mean the patch in this bug is obsolete and we should look at bug 418641 for the 1.8 branch, or that this patch was checked in when that one was?
Comment 13 Igor Bukanov 2008-04-23 14:08:27 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > This is fixed on the trunk since landing bug 418641.
> 
> Does that mean the patch in this bug is obsolete and we should look at bug
> 418641 for the 1.8 branch, or that this patch was checked in when that one was?

The branch requires a new separated patch. The patch in this bug or in the bug  418641 can serve only as a guideline.
Comment 14 Igor Bukanov 2008-06-04 13:41:57 PDT
Created attachment 323761 [details] [diff] [review]
fix for 1.8 branch

This is what I am going to test on 2008-06-04 late evening PST. Still I am asking for an early review to make sure that I have not missed anything in the patch.

To solve the problem with unrooted id the patch uses JS_KEEP_ATOMS. For the problem of the setter unrooting the pre-increment result the patch makes sure that an extra temporary slot is allocated both for post- and pre-increment.
Comment 15 Brendan Eich [:brendan] 2008-06-04 14:50:23 PDT
Comment on attachment 323761 [details] [diff] [review]
fix for 1.8 branch

Looks good. Assuming it all tests well too. r=me.

/be
Comment 16 Daniel Veditz [:dveditz] 2008-06-05 14:20:16 PDT
Comment on attachment 323761 [details] [diff] [review]
fix for 1.8 branch

Approved for 1.8.1.15, a=dveditz for release-drivers
Comment 17 Igor Bukanov 2008-06-05 18:24:01 PDT
Comment on attachment 323761 [details] [diff] [review]
fix for 1.8 branch

The patch regressed the js shell test suite.
Comment 18 Igor Bukanov 2008-06-06 04:18:27 PDT
Created attachment 324028 [details] [diff] [review]
fix for 1.8 branch

Now the patch passes shell tests without regressions: in the previous one I "optimized" ++/-- for var and args too much.

To Bob Clary: is there a slow test list for 1.8.1 branch? When I run the tests on sm-valgrind01 with -L slow-n.js it takes 3 hours or so.
Comment 19 Igor Bukanov 2008-06-06 04:24:40 PDT
Created attachment 324029 [details] [diff] [review]
fix for 1.8 branch v2 for real

The previous attachment has a wrong patch
Comment 20 Bob Clary [:bc:] 2008-06-06 05:17:22 PDT
(In reply to comment #18)

> 
> To Bob Clary: is there a slow test list for 1.8.1 branch? When I run the tests
> on sm-valgrind01 with -L slow-n.js it takes 3 hours or so.

No, but I can create branch specific versions. Are you using jsDriver with the default timeout? It is rather long at 3600 seconds. -T 480 or -T 120 will cut it down to more manageable period.
Comment 21 Brendan Eich [:brendan] 2008-06-06 11:40:56 PDT
Comment on attachment 324029 [details] [diff] [review]
fix for 1.8 branch v2 for real

Sorry I missed that!

/be
Comment 22 Daniel Veditz [:dveditz] 2008-06-06 14:00:30 PDT
Comment on attachment 324029 [details] [diff] [review]
fix for 1.8 branch v2 for real

Approved for 1.8.1.15, a=dveditz for release-drivers
Comment 23 Igor Bukanov 2008-06-06 14:50:33 PDT
I checked in the patch from the comment 19 to the MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.77; previous revision: 3.128.2.76
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.101; previous revision: 3.181.2.100
done
Comment 24 Bob Clary [:bc:] 2008-06-11 01:55:03 PDT
verified fixed 1.8.1.15 linux/mac/win
Comment 25 Alexander Sack 2008-06-16 03:55:49 PDT
Created attachment 325247 [details] [diff] [review]
1.8.0 attempt
Comment 26 Alexander Sack 2008-06-16 03:59:11 PDT
Comment on attachment 325247 [details] [diff] [review]
1.8.0 attempt

the other option would have been to pull in bug 352272 - however, thats 1.7 from what i can tell; thus this backport.

Anyway, it fixes the crash for me too.
Comment 27 Alexander Sack 2008-06-16 04:00:16 PDT
Created attachment 325248 [details] [diff] [review]
1.8 to 1.8.0 plaindiff
Comment 28 Alexander Sack 2008-06-16 04:04:07 PDT
Without patch:
js> before 7157, after 6409, break 0070e000
Segmentation fault
Comment 29 Alexander Sack 2008-06-16 04:13:46 PDT
Comment on attachment 325247 [details] [diff] [review]
1.8.0 attempt

we found a glitch, will attach updated one
Comment 30 Alexander Sack 2008-06-16 04:29:52 PDT
Created attachment 325249 [details] [diff] [review]
 1.8.0 attempt 2

2nd attempt
Comment 31 Alexander Sack 2008-06-16 04:30:24 PDT
Created attachment 325250 [details] [diff] [review]
 1.8 to 1.8.0 plaindiff
Comment 32 Igor Bukanov 2008-06-16 06:00:49 PDT
Comment on attachment 325249 [details] [diff] [review]
 1.8.0 attempt 2

>Index: jsemit.c

>         if (pn2->pn_type != TOK_NAME &&
>-            (js_CodeSpec[op].format & JOF_POST) &&
>+            (js_CodeSpec[op].format & (JOF_INC | JOF_DEC)) &&
>             (uintN)++cg->stackDepth > cg->maxStackDepth) {
>             cg->maxStackDepth = cg->stackDepth;

...
> 
>-        if (pn2->pn_type != TOK_NAME && (js_CodeSpec[op].format & JOF_POST))
>+        if (pn2->pn_type != TOK_NAME && (js_CodeSpec[op].format & (JOF_INC | JOF_DEC)))
>             --cg->stackDepth;

Right, on 1.8 there was only one place to patch as there the first "if" affected only cg->maxStackDepth without touching cg->stackDepth. On 1.8.0 both places has to be patched.

>Index: jsinterp.c
>             /* The operand must contain a number. */
>+            /* Preload for use in the if/else immediately below. */
>+            cs = &js_CodeSpec[op];
>+

On 1.8.0 branch cs is always initialized before interpreter's switch. There is no need to assign it again and these 3 added lines can be removed. r+ with this fixed.
Comment 33 Alexander Sack 2008-06-16 07:45:01 PDT
Created attachment 325262 [details] [diff] [review]
1.8.0 attempt 2 + comments
Comment 34 Bob Clary [:bc:] 2008-07-03 07:29:36 PDT
/cvsroot/mozilla/js/tests/js1_5/GC/regress-418128.js,v  <--  regress-418128.js
initial revision: 1.1

changeset:   15651:33dac11fb9ed

Note You need to log in before you can comment on or make changes to this bug.