375976 - "Assertion failure: *flagp != GCF_FINAL" with post-increment

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

To reproduce: paste this into jsshell

this.__defineSetter__('x', gc);
this.__defineGetter__('x', Math.sin);
x = x++;


Debug:

Assertion failure: *flagp != GCF_FINAL, at jsgc.c:2499


Opt:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00023011

Thread 0 Crashed:
0   js 	0x00023011 js_EmitTree + 11342
1   js 	0x0002de89 js_MarkStackFrame + 325
2   js 	0x0002e3eb js_GC + 816
3   js 	0x000058c9 JS_GC + 66
4   js 	0x0000279e GC + 34
5   js 	0x0003bb73 js_Invoke + 1843
6   js 	0x0003bfac js_InternalInvoke + 181
7   js 	0x0003c136 js_InternalGetOrSet + 261
8   js 	0x00041765 js_NativeSet + 165
9   js 	0x00045b61 js_SetProperty + 1084
10  js 	0x00031dfc js_Interpret + 6390
11  js 	0x0003b37a js_Execute + 513
12  js 	0x000079c3 JS_ExecuteScript + 56
13  js 	0x00002095 Process + 1105
14  js 	0x00004df9 main + 2270
15  js 	0x00001c26 _start + 216
16  js 	0x00001b4d start + 41

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

So, the post-increment code goes through some effort to root rtmp (the old value) but no attempt to root rval, which is the new value. So, I think that we call OBJ_SET_PROPERTY with an unrooted rval, which gets collected. I'm not really sure what the cleanest way to fix this is.

We could make JSOP_INC* take another stack slot and push rval too, or is there a cleaner way?

Comment 2

[Brendan Eich [:brendan]]

Is there a way to swap rval into the root protecting rtmp, once rtmp is protected by something else? If not, more stack.

/be

Comment 3

[chris hofmann]

trying to get owners for all sg:bugs

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

My initial diagnosis was not correct. The parameter to OBJ_SET_PROPERTY is, in fact, safe from GC (first, because it's a parameter to the setter, then because it's stored in a GC-safe object). The problem is actually that the old value needs to be protected across the OBJ_SET_PROPERTY, not just across the js_NewNumberValue.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

With this patch, the testcase runs cleanly for me, since rtmp is now protected over both the js_NewNumberValue and the OBJ_SET_PROPERTY.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 265554 [details] [diff] [review]
Proposed fix

Whew -- I *thought* we did not need another root -- a relief to know that's true still.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GetSet/regress-375976.js

Comment 9

[Daniel Veditz [:dveditz]]

This affects the 1.8.0 and 1.8 branches, too.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 265554 [details] [diff] [review]
Proposed fix

This applies to the 1.8 and 1.8.0 branches as is.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 265554 [details] [diff] [review]
Proposed fix

approved for 1.8.1.5 and 1.8.0.13, a=dveditz

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branches.

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1, 1.9.0 windows/linux/macppc opt/debug browser/shell 7/16

Comment 14

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/GetSet/regress-375976.js,v  <--  regress-375976.js
initial revision: 1.1