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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
x86
Mac OS X
assertion, crash, fixed1.8.0.13, testcase, verified1.8.1.5
Points:
---
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 1

11 years ago
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?
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

11 years ago
trying to get owners for all sg:bugs
Assignee: general → mrbkap
(Reporter)

Updated

10 years ago
No longer blocks: 349611
(Reporter)

Updated

10 years ago
Blocks: 349611
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Created attachment 265554 [details] [diff] [review]
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.
Attachment #265554 - Flags: review?(crowder)
Attachment #265554 - Flags: review?(brendan)

Updated

10 years ago
Attachment #265554 - Flags: review?(crowder) → review+
(Assignee)

Comment 6

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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
Attachment #265554 - Flags: review?(brendan) → review+

Comment 8

10 years ago
Created attachment 266303 [details]
js1_5/GetSet/regress-375976.js

Updated

10 years ago
Flags: in-testsuite+
This affects the 1.8.0 and 1.8 branches, too.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
(Assignee)

Comment 10

10 years ago
Comment on attachment 265554 [details] [diff] [review]
Proposed fix

This applies to the 1.8 and 1.8.0 branches as is.
Attachment #265554 - Flags: approval1.8.1.5?
Attachment #265554 - Flags: approval1.8.0.13?
Comment on attachment 265554 [details] [diff] [review]
Proposed fix

approved for 1.8.1.5 and 1.8.0.13, a=dveditz
Attachment #265554 - Flags: approval1.8.1.5?
Attachment #265554 - Flags: approval1.8.1.5+
Attachment #265554 - Flags: approval1.8.0.13?
Attachment #265554 - Flags: approval1.8.0.13+
(Assignee)

Comment 12

10 years ago
Fixed on the 1.8 branches.
Keywords: fixed1.8.0.13, fixed1.8.1.5

Comment 13

10 years ago
verified fixed 1.8.1, 1.9.0 windows/linux/macppc opt/debug browser/shell 7/16
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.5 → verified1.8.1.5
Group: security

Comment 14

10 years ago
/cvsroot/mozilla/js/tests/js1_5/GetSet/regress-375976.js,v  <--  regress-375976.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.