Crash with setter, watch, and GC

VERIFIED FIXED in mozilla1.8.1

Status

()

defect
P1
critical
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: jruderman, Assigned: mrbkap)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

13 years ago
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.
Reporter

Updated

13 years ago
Whiteboard: [sg:critical?]
Posted patch Potential fix (obsolete) — Splinter Review
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #246122 - Flags: review?(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
Attachment #246122 - Attachment is obsolete: true
Attachment #246125 - Flags: review?(brendan)
Attachment #246122 - Flags: review?(brendan)
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Comment on attachment 246125 [details] [diff] [review]
With that fixed

Looks good, please nominate the bug too.

/be
Attachment #246125 - Flags: review?(brendan)
Attachment #246125 - Flags: review+
Attachment #246125 - Flags: approval1.8.1.1?
Attachment #246125 - Flags: approval1.8.0.9?
Reporter

Updated

13 years ago
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?

Updated

13 years ago
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+

Comment 5

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

Approved for both branches.  a=jay for drivers, please land asap.
Attachment #246125 - Flags: approval1.8.1.1?
Attachment #246125 - Flags: approval1.8.1.1+
Attachment #246125 - Flags: approval1.8.0.9?
Attachment #246125 - Flags: approval1.8.0.9+
Blocks: 361467
This has yet to be landed anywhere -- mrbkap, can you do the checkins?

/be
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1
Fixed on the 1.8 branch now too.
Keywords: fixed1.8.1.1
...And the 1.8.0 branch too...
Keywords: fixed1.8.0.9

Comment 10

13 years ago
crashes on shutdown before fixes.

Updated

13 years ago
Flags: in-testsuite+

Comment 11

13 years ago
verified fixed 20061125 1.8.0.9 windows/linux/mac*, 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
Status: RESOLVED → VERIFIED
Group: security

Comment 12

13 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-361346.js,v  <--  regress-361346.js
Reporter

Updated

12 years ago
No longer blocks: 349611
Reporter

Updated

12 years ago
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.