The default bug view has changed. See this FAQ.

"Assertion failure: *flagp != GCF_FINAL" with getter, setter, watch, and more

VERIFIED FIXED

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
assertion, 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

(4 attachments)

(Reporter)

Description

10 years ago
Created attachment 265464 [details]
testcase

$ ./js ~/gc-crash.js
Assertion failure: *flagp != GCF_FINAL, at jsgc.c:2147
(Reporter)

Comment 1

10 years ago
This can also trigger:
Assertion failure: offsetInArena < GC_THINGS_SIZE, at jsgc.c:513
(Assignee)

Comment 2

10 years ago
This is a GC hazard in js_AddScopeProperty in the |overwriting| case when the property we're overwriting has a watchpoint set on it. The problem is that we remove the old property from the property tree, preparing to replace it with a new property, and then we call js_WrapWatchedSetter, which can cause a GC. Because we've removed the property from its property tree, it doesn't get marked, which means that none of its members get marked, which leaves its getter up for grabs.
(Assignee)

Comment 3

10 years ago
Created attachment 265579 [details] [diff] [review]
Potential fix

I'm not sure if there's a cleaner way to fix this bug, but this does the trick by ensuring that the overwritten property is marked when we create the new setter for the watchpoint.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #265579 - Flags: review?(igor)

Comment 4

10 years ago
Comment on attachment 265579 [details] [diff] [review]
Potential fix

This is exactly the purpose of JS_PUSH_TEMP_ROOT_SPROP. But I would even suggest to remove "if (overwriting)" checks around tvr calls for safer and smaller code.
Attachment #265579 - Flags: review?(igor) → review+
(Assignee)

Comment 5

10 years ago
Created attachment 265586 [details] [diff] [review]
What I'm about to check in
(Assignee)

Comment 6

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 7

10 years ago
Created attachment 266296 [details]
js1_7/GC/regress-381374.js

Updated

10 years ago
Flags: in-testsuite+
(Reporter)

Updated

10 years ago
Whiteboard: [sg:critical?]
The testcase in the bug calls "gczeal()" which is new on the trunk so I can't test the branch easily, but the patched code looks identical in 1.8.1.x so we probably need this fix there.

May want to take gczeal on the branch (bug 308429)
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
(Reporter)

Comment 9

10 years ago
You might be able to reproduce on branch using TOO_MUCH_GC or WAY_TOO_MUCH_GC.
yeah, that would be the "not easily" way.
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
(Assignee)

Comment 11

10 years ago
Comment on attachment 265586 [details] [diff] [review]
What I'm about to check in

This applies the 1.8 and 1.8.0 branches as-is.
Attachment #265586 - Flags: approval1.8.1.5?
Attachment #265586 - Flags: approval1.8.0.13?
Comment on attachment 265586 [details] [diff] [review]
What I'm about to check in

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

Comment 13

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

Comment 14

10 years ago
verified fixed 1.8.1, 1.9.0 windows/linux/macppc opt/debug shell/browser 7/16
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.5 → verified1.8.1.5
Can someone with a debug build verify this is fixed with a Firefox 1.5.0.13 candidate?  Thanks!

Comment 16

10 years ago
(In reply to comment #15)
> Can someone with a debug build verify this is fixed with a Firefox 1.5.0.13
> candidate?  Thanks!

The test case isn't supported on the 1.8.0 branch. I don't know how it could be verified there.
(In reply to comment #16)
> (In reply to comment #15)
> > Can someone with a debug build verify this is fixed with a Firefox 1.5.0.13
> > candidate?  Thanks!
> 
> The test case isn't supported on the 1.8.0 branch. I don't know how it could be
> verified there.

Sorry, I really meant a debug build of Thunderbird 1.5.0.13 (using either Thunderbrowse or the Mail Start Page trick), since it has fixed1.5.0.13 as a keyword.

Group: security

Comment 18

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