Last Comment Bug 381374 - "Assertion failure: *flagp != GCF_FINAL" with getter, setter, watch, and more
: "Assertion failure: *flagp != GCF_FINAL" with getter, setter, watch, and more
Status: VERIFIED FIXED
[sg:critical?]
: assertion, fixed1.8.0.13, testcase, verified1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2007-05-20 16:52 PDT by Jesse Ruderman
Modified: 2007-09-21 07:49 PDT (History)
8 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (312 bytes, text/javascript)
2007-05-20 16:52 PDT, Jesse Ruderman
no flags Details
Potential fix (1.88 KB, patch)
2007-05-21 17:14 PDT, Blake Kaplan (:mrbkap)
igor: review+
Details | Diff | Splinter Review
What I'm about to check in (1.81 KB, patch)
2007-05-21 17:51 PDT, Blake Kaplan (:mrbkap)
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review
js1_7/GC/regress-381374.js (2.71 KB, text/plain)
2007-05-27 16:23 PDT, Bob Clary [:bc:]
no flags Details

Description Jesse Ruderman 2007-05-20 16:52:17 PDT
Created attachment 265464 [details]
testcase

$ ./js ~/gc-crash.js
Assertion failure: *flagp != GCF_FINAL, at jsgc.c:2147
Comment 1 Jesse Ruderman 2007-05-20 16:54:54 PDT
This can also trigger:
Assertion failure: offsetInArena < GC_THINGS_SIZE, at jsgc.c:513
Comment 2 Blake Kaplan (:mrbkap) 2007-05-21 17:05:16 PDT
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) 2007-05-21 17:14:04 PDT
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.
Comment 4 Igor Bukanov 2007-05-21 17:18:13 PDT
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) 2007-05-21 17:51:02 PDT
Created attachment 265586 [details] [diff] [review]
What I'm about to check in
Comment 6 Blake Kaplan (:mrbkap) 2007-05-21 17:55:14 PDT
Fix checked into trunk.
Comment 7 Bob Clary [:bc:] 2007-05-27 16:23:06 PDT
Created attachment 266296 [details]
js1_7/GC/regress-381374.js
Comment 8 Daniel Veditz [:dveditz] 2007-06-28 13:45:18 PDT
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 2007-06-28 13:51:09 PDT
You might be able to reproduce on branch using TOO_MUCH_GC or WAY_TOO_MUCH_GC.
Comment 10 Daniel Veditz [:dveditz] 2007-06-29 10:50:28 PDT
yeah, that would be the "not easily" way.
Comment 11 Blake Kaplan (:mrbkap) 2007-07-10 18:00:35 PDT
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] 2007-07-10 18:11:18 PDT
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) 2007-07-10 18:33:50 PDT
Fixed on the 1.8 branches.
Comment 14 Bob Clary [:bc:] 2007-07-17 10:55:04 PDT
verified fixed 1.8.1, 1.9.0 windows/linux/macppc opt/debug shell/browser 7/16
Comment 15 Stephen Donner [:stephend] 2007-08-21 15:15:56 PDT
Can someone with a debug build verify this is fixed with a Firefox 1.5.0.13 candidate?  Thanks!
Comment 16 Bob Clary [:bc:] 2007-08-22 13:07:42 PDT
(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] 2007-08-22 14:40:38 PDT
(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:] 2007-09-21 07:49:14 PDT
/cvsroot/mozilla/js/tests/js1_7/GC/regress-381374.js,v  <--  regress-381374.js
initial revision: 1.1

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