Closed
Bug 327708
Opened 19 years ago
Closed 19 years ago
fun_xdrObject should root fun across call to js_XDRScript
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: dbaron, Assigned: mrbkap)
References
Details
(Keywords: fixed1.8.0.4, fixed1.8.1, Whiteboard: [patch])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
brendan
:
review+
brendan
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
This is the cause of the first crash I hit when trying to start Firefox with WAY_TOO_MUCH_GC. The object allocated in fun_xdrObject at jsfun.c:1220 needs to be rooted across the call to js_XDRScript at jsfun.c:1337, or else it could potentially be collected here:
js_GC (/builds/trunk/mozilla/js/src/jsgc.c:1947)
js_NewGCThing (/builds/trunk/mozilla/js/src/jsgc.c:635)
js_NewString (/builds/trunk/mozilla/js/src/jsstr.c:2529)
js_NewStringCopyN (/builds/trunk/mozilla/js/src/jsstr.c:2644)
js_AtomizeString (/builds/trunk/mozilla/js/src/jsatom.c:664)
js_AtomizeChars (/builds/trunk/mozilla/js/src/jsatom.c:755)
js_XDRStringAtom (/builds/trunk/mozilla/js/src/jsxdrapi.c:675)
js_XDRAtom (/builds/trunk/mozilla/js/src/jsxdrapi.c:627)
XDRAtomMap (/builds/trunk/mozilla/js/src/jsscript.c:370)
js_XDRScript (/builds/trunk/mozilla/js/src/jsscript.c:501)
fun_xdrObject (/builds/trunk/mozilla/js/src/jsfun.c:1337)
Reporter | ||
Comment 1•19 years ago
|
||
This works around the problem for me; no idea if it's the recommended way within the JS engine, whether it has threadsafety problems, etc.
Assignee | ||
Comment 2•19 years ago
|
||
I'll take this... the current hip way to protect objects from GC in situations like this is to use JSTempValueRooter (see jscntxt.h).
Unfortunately, your patch isn't quite sufficient. There are calls to js_Atomize (and js_AtomizeString) that could also possibly collect func. This is one place where I _really_ miss C++'s nice destructors. I'll take this and root fun across fun_xdrObject.
Assignee: general → mrbkap
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 3•19 years ago
|
||
It seemed cleaner to me to to common out the ok = JS_FALSE setting into the two labels than the other options.
Attachment #212310 -
Attachment is obsolete: true
Attachment #215958 -
Flags: review?(brendan)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [patch]
Comment 4•19 years ago
|
||
Comment on attachment 215958 [details] [diff] [review]
Using tvrs across the whole function
>+ v = OBJECT_TO_JSVAL(fun->object);
Get rid of jsval v local, it is not needed -- JS_PUSH_SINGLE_TEMP_ROOT takes an arbitrary expression that evaluates to a jsval.
>+cleanup:
Canonical label name is out.
Fix these and r=me.
/be
Attachment #215958 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 5•19 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8.1?
Updated•19 years ago
|
Flags: in-testsuite-
Reporter | ||
Comment 6•19 years ago
|
||
Any chance of getting this on 1.8.1; maybe even 1.8.0.3?
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.0.3?
Assignee | ||
Comment 7•19 years ago
|
||
Comment on attachment 215958 [details] [diff] [review]
Using tvrs across the whole function
This patch has been baking on the trunk for a month with no known regressions. It uses an established method for protecting jsvals from begin GC'd.
Attachment #215958 -
Flags: approval1.8.0.3?
Attachment #215958 -
Flags: approval-branch-1.8.1?(brendan)
Updated•19 years ago
|
Attachment #215958 -
Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Updated•19 years ago
|
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment 8•19 years ago
|
||
Comment on attachment 215958 [details] [diff] [review]
Using tvrs across the whole function
aproved for 1.8.0 branch, a=dveditz for drivers
Attachment #215958 -
Flags: approval1.8.0.3? → approval1.8.0.3+
Assignee | ||
Comment 9•19 years ago
|
||
Fix checked into both 1.8 branches.
Keywords: fixed1.8.0.3,
fixed1.8.1
Comment 10•19 years ago
|
||
David, did you have a test for this?
Reporter | ||
Comment 11•19 years ago
|
||
Starting up Firefox (not the first time, presumably).
You need to log in
before you can comment on or make changes to this bug.
Description
•