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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

This can also trigger:
Assertion failure: offsetInArena < GC_THINGS_SIZE, at jsgc.c:513

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 4

[Igor Bukanov]

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.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/GC/regress-381374.js

Comment 8

[Daniel Veditz [:dveditz]]

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)

Comment 9

[Jesse Ruderman]

You might be able to reproduce on branch using TOO_MUCH_GC or WAY_TOO_MUCH_GC.

Comment 10

[Daniel Veditz [:dveditz]]

yeah, that would be the "not easily" way.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branches.

Comment 14

[Bob Clary [:bc] (inactive)]

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

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Can someone with a debug build verify this is fixed with a Firefox 1.5.0.13 candidate?  Thanks!

Comment 16

[Bob Clary [:bc] (inactive)]

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

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 18

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/GC/regress-381374.js,v  <--  regress-381374.js
initial revision: 1.1