Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash with setter, watch, and GC

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.8.1
crash, testcase, verified1.8.0.9, verified1.8.1.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

11 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

11 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 1

11 years ago
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.
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
(Assignee)

Comment 3

11 years ago
Created attachment 246125 [details] [diff] [review]
With that fixed
Attachment #246122 - Attachment is obsolete: true
Attachment #246125 - Flags: review?(brendan)
Attachment #246122 - Flags: review?(brendan)

Updated

11 years ago
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

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

Updated

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

Comment 5

11 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+

Updated

11 years ago
Blocks: 361467
This has yet to be landed anywhere -- mrbkap, can you do the checkins?

/be
(Assignee)

Comment 7

11 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 8

11 years ago
Fixed on the 1.8 branch now too.
Keywords: fixed1.8.1.1
(Assignee)

Comment 9

11 years ago
...And the 1.8.0 branch too...
Keywords: fixed1.8.0.9

Comment 10

11 years ago
Created attachment 246497 [details]
js1_5/Regress/regress-361346.js

crashes on shutdown before fixes.

Updated

11 years ago
Flags: in-testsuite+

Comment 11

11 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
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security

Comment 12

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

Updated

10 years ago
No longer blocks: 349611
(Reporter)

Updated

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