Last Comment Bug 361346 - Crash with setter, watch, and GC
: Crash with setter, watch, and GC
: crash, testcase, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
P1 critical (vote)
: mozilla1.8.1
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: jsfunfuzz 361467
  Show dependency treegraph
Reported: 2006-11-20 19:28 PST by Jesse Ruderman
Modified: 2007-05-16 17:44 PDT (History)
2 users (show)
jaymoz: blocking1.8.1.1+
jaymoz: blocking1.8.0.9+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Potential fix (2.66 KB, patch)
2006-11-20 22:27 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
With that fixed (2.64 KB, patch)
2006-11-20 23:07 PST, Blake Kaplan (:mrbkap)
brendan: review+
jaymoz: approval1.8.0.9+
jaymoz: approval1.8.1.1+
Details | Diff | Splinter Review
js1_5/Regress/regress-361346.js (2.12 KB, text/plain)
2006-11-24 16:05 PST, Bob Clary [:bc:]
no flags Details

Description User image Jesse Ruderman 2006-11-20 19:28:49 PST
js> this.x setter= new Function;'x', function(){}); gc(); x = {};
before 9232, after 9232, break 01205000
Bus error

I saw this jump to a bogus address a few times, so I'm marking the bug security-sensitive.
Comment 1 User image Blake Kaplan (:mrbkap) 2006-11-20 22:27:11 PST
Created attachment 246122 [details] [diff] [review]
Potential fix

I think the problem here is that we replace the sprop that contains the setter as its setter on the object, and then fail to mark it ever again (since it's stored in the watchpoint's setter field, and to this point, nobody marks it). This patch fixes the bug for me.
Comment 2 User image Brendan Eich [:brendan] 2006-11-20 22:50:27 PST
Comment on attachment 246122 [details] [diff] [review]
Potential fix

>+        JS_MarkGCThing(cx, wp->setter, "wp->setter", NULL);

Only if (wp->sprop->attrs & JSPROP_SETTER), right?  Otherwise, it's a JSPropertyOp and we shouldn't try to mark it.

Comment 3 User image Blake Kaplan (:mrbkap) 2006-11-20 23:07:21 PST
Created attachment 246125 [details] [diff] [review]
With that fixed
Comment 4 User image Brendan Eich [:brendan] 2006-11-20 23:24:23 PST
Comment on attachment 246125 [details] [diff] [review]
With that fixed

Looks good, please nominate the bug too.

Comment 5 User image Jay Patel [:jay] 2006-11-21 11:33:23 PST
Comment on attachment 246125 [details] [diff] [review]
With that fixed

Approved for both branches.  a=jay for drivers, please land asap.
Comment 6 User image Brendan Eich [:brendan] 2006-11-23 11:56:59 PST
This has yet to be landed anywhere -- mrbkap, can you do the checkins?

Comment 7 User image Blake Kaplan (:mrbkap) 2006-11-24 10:58:01 PST
Fixed on trunk.
Comment 8 User image Blake Kaplan (:mrbkap) 2006-11-24 11:15:07 PST
Fixed on the 1.8 branch now too.
Comment 9 User image Blake Kaplan (:mrbkap) 2006-11-24 11:43:45 PST
...And the 1.8.0 branch too...
Comment 10 User image Bob Clary [:bc:] 2006-11-24 16:05:39 PST
Created attachment 246497 [details]

crashes on shutdown before fixes.
Comment 11 User image Bob Clary [:bc:] 2006-11-25 22:56:29 PST
verified fixed 20061125 windows/linux/mac*, windows/linux/mac*, 1.9 windows/linux
Comment 12 User image Bob Clary [:bc:] 2007-02-08 22:30:28 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-361346.js,v  <--  regress-361346.js

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