361346 - Crash with setter, watch, and GC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

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) (inactive)]

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

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) (inactive)]

Attachment: With that fixed

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 246125 [details] [diff] [review]
With that fixed

Looks good, please nominate the bug too.

/be

Comment 5

[Jay Patel [:jay]]

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]]

This has yet to be landed anywhere -- mrbkap, can you do the checkins?

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on trunk.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch now too.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

...And the 1.8.0 branch too...

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-361346.js

crashes on shutdown before fixes.

Comment 11

[Bob Clary [:bc] (inactive)]

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] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-361346.js,v  <--  regress-361346.js