Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
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 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 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 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 Blake Kaplan (:mrbkap) 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.

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?

Comment 7 Blake Kaplan (:mrbkap) 2006-11-24 10:58:01 PST
Fixed on trunk.
Comment 8 Blake Kaplan (:mrbkap) 2006-11-24 11:15:07 PST
Fixed on the 1.8 branch now too.
Comment 9 Blake Kaplan (:mrbkap) 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]

crashes on shutdown before fixes.
Comment 11 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 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.