Last Comment Bug 361346 - Crash with setter, watch, and GC
: Crash with setter, watch, and GC
Status: VERIFIED FIXED
[sg:critical?]
: 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) (please use needinfo!)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Potential fix (2.66 KB, patch)
2006-11-20 22:27 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details | Diff | Review
With that fixed (2.64 KB, patch)
2006-11-20 23:07 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
brendan: review+
jaymoz: approval1.8.0.9+
jaymoz: approval1.8.1.1+
Details | Diff | 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 Jesse Ruderman 2006-11-20 19:28:49 PST
js> this.x setter= new Function; this.watch('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 Blake Kaplan (:mrbkap) (please use needinfo!) 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 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.

/be
Comment 3 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-11-20 23:07:21 PST
Created attachment 246125 [details] [diff] [review]
With that fixed
Comment 4 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.

/be
Comment 5 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 Brendan Eich [:brendan] 2006-11-23 11:56:59 PST
This has yet to be landed anywhere -- mrbkap, can you do the checkins?

/be
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-11-24 10:58:01 PST
Fixed on trunk.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-11-24 11:15:07 PST
Fixed on the 1.8 branch now too.
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-11-24 11:43:45 PST
...And the 1.8.0 branch too...
Comment 10 Bob Clary [:bc:] 2006-11-24 16:05:39 PST
Created attachment 246497 [details]
js1_5/Regress/regress-361346.js

crashes on shutdown before fixes.
Comment 11 Bob Clary [:bc:] 2006-11-25 22:56:29 PST
verified fixed 20061125 1.8.0.9 windows/linux/mac*, 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
Comment 12 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.