Last Comment Bug 327708 - fun_xdrObject should root fun across call to js_XDRScript
: fun_xdrObject should root fun across call to js_XDRScript
Status: RESOLVED FIXED
[patch]
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on:
Blocks: 307312
  Show dependency treegraph
 
Reported: 2006-02-18 01:42 PST by David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
Modified: 2006-05-31 09:24 PDT (History)
4 users (show)
dbaron: blocking1.8.1?
dveditz: blocking1.8.0.4+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible patch (2.29 KB, patch)
2006-02-18 04:16 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
Using tvrs across the whole function (6.39 KB, patch)
2006-03-22 17:53 PST, Blake Kaplan (:mrbkap)
brendan: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-02-18 01:42:57 PST
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)
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-02-18 04:16:25 PST
Created attachment 212310 [details] [diff] [review]
possible patch

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.
Comment 2 Blake Kaplan (:mrbkap) 2006-02-18 04:28:51 PST
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.
Comment 3 Blake Kaplan (:mrbkap) 2006-03-22 17:53:22 PST
Created attachment 215958 [details] [diff] [review]
Using tvrs across the whole function

It seemed cleaner to me to to common out the ok = JS_FALSE setting into the two labels than the other options.
Comment 4 Brendan Eich [:brendan] 2006-03-22 21:02:30 PST
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
Comment 5 Blake Kaplan (:mrbkap) 2006-03-22 21:12:42 PST
Fix checked into trunk.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-04-19 00:22:51 PDT
Any chance of getting this on 1.8.1; maybe even 1.8.0.3?
Comment 7 Blake Kaplan (:mrbkap) 2006-04-19 00:34:37 PDT
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.
Comment 8 Daniel Veditz [:dveditz] 2006-04-20 11:37:40 PDT
Comment on attachment 215958 [details] [diff] [review]
Using tvrs across the whole function

aproved for 1.8.0 branch, a=dveditz for drivers
Comment 9 Blake Kaplan (:mrbkap) 2006-04-21 14:24:48 PDT
Fix checked into both 1.8 branches.
Comment 10 Bob Clary [:bc:] 2006-05-22 21:39:41 PDT
David, did you have a test for this?
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-05-31 09:24:49 PDT
Starting up Firefox (not the first time, presumably).

Note You need to log in before you can comment on or make changes to this bug.