Bugzilla@Mozilla – Bug 643927
GC hazard in JS_XDRScriptObject
Last modified: 2011-07-12 08:22:22 PDT
Summon comment box
When reviewing the patch for the bug 632253 I have missed that JSScript::filename is only rooted by JSScript. So the result of js_SaveScriptFilename must be stored in the rooted script instance before a GC can happen. Yet I have suggested to decode the filename before any script instance id created.
What do you want to do here? I could move the js_SaveScriptFilename call back to js_XDRScriptAndSubscripts. Would be a bit ugly.
(In reply to comment #1) > What do you want to do here? I could move the js_SaveScriptFilename call back > to js_XDRScriptAndSubscripts. Would be a bit ugly. A better solution would be to add a class like XDRScriptState that would contain script-related serializations state and explicitly pass it to any function that needs global script state instead of adding more fields to JSXDRState. This way we could remove the hacks to temporary set xdr->atoms etc. when serializing the script. The class can have a flag like filenameDone to write/read the filename on the first occurrence when JSScript is available. js_CloenScript can set that flag to avoid useless serialization.
Michael: do you have time to implement this today-tomorrow? I need something that I described in the comment 2 to fix the bug 643967.
I'll see what I can do.
Created attachment 521327 [details] [review] Fix Not sure how I feel about this approach, but I did fix an error path memory leak in js_CloneScript along the way.
Comment on attachment 521327 [details] [review] Fix >@@ -348,6 +327,9 @@ js_XDRScriptAndSubscripts(JSXDRState *xd > JSScript *script = *scriptp; > nsrcnotes = ntrynotes = natoms = nobjects = nregexps = nconsts = 0; > jssrcnote *notes = NULL; >+ XDRScriptState *state = xdr->state; >+ >+ JS_ASSERT(xdr->state); Nit: JS_ASSERT(state) >@@ -618,10 +600,20 @@ js_XDRScriptAndSubscripts(JSXDRState *xd >+class JSXDRStateHolder { Thanks for creating the class - it is long overdue. Nit: rename this into AutoJSXDRState to follow the SM naming convention for such classes. >+public: >+ JSXDRStateHolder(JSXDRState *x) : xdr(x) {} >+ ~JSXDRStateHolder() { >+ JS_XDRDestroy(xdr); >+ } >+ >+ operator JSXDRState*() const { >+ return xdr; >+ } >+ >+private: >+ JSXDRState *xdr; Nit: make this JSXDRState *const xdr; >@@ -1967,48 +1974,42 @@ js_CloneScript(JSContext *cx, JSScript * > > // Hand p off from w to r. Don't want them to share the data > // mem, lest they both try to free it in JS_XDRDestroy > JS_XDRMemSetData(r, p, nbytes); > JS_XDRMemSetData(w, NULL, 0); > >- r->filename = script->filename; >+ XDRScriptState rstate(r); >+ rstate.filename = script->filename; >+ rstate.filenameSaved = true; >diff --git a/js/src/jsxdrapi.h b/js/src/jsxdrapi.h >--- a/js/src/jsxdrapi.h >+++ b/js/src/jsxdrapi.h >@@ -112,6 +112,8 @@ typedef struct JSXDROps { > typedef js::Vector<JSAtom *, 1, js::SystemAllocPolicy> XDRAtoms; > typedef js::HashMap<JSAtom *, uint32, js::DefaultHasher<JSAtom *>, js::SystemAllocPolicy> XDRAtomsHashMap; > >+class XDRScriptState; Nit: put the class into the js namespace.
http://hg.mozilla.org/tracemonkey/rev/4484468a829b
Follow fix to fix bustage on debug builds: http://hg.mozilla.org/tracemonkey/rev/c6388fb2d7be I'll remember to do a debug build next time..
Auto classes should use the parameter macros that make sure the auto-helper can't be temporary-initialized. See jscntxt.h and the Auto* classes in it for examples.
Created attachment 521638 [details] [review] Use guard object It doesn't matter a lot in this case since this is meant to be used as a smart pointer. However, in the rare case that someone tries to use this as a holder instead of a smart pointer, this will help.
http://hg.mozilla.org/tracemonkey/rev/3279d69de777 for the guard-macro followup
http://hg.mozilla.org/mozilla-central/rev/4484468a829b http://hg.mozilla.org/mozilla-central/rev/c6388fb2d7be http://hg.mozilla.org/mozilla-central/rev/3279d69de777