Last Comment Bug 643927 - GC hazard in JS_XDRScriptObject
: GC hazard in JS_XDRScriptObject
Status: RESOLVED FIXED
: [sg:critical?]
: regression
Product: Core
Classification: Components
Component: JavaScript Engine
: unspecified
: All All
: -- enhancement (vote)
: mozilla5
Assigned To: Michael Wu [:mwu]
: general
:
:
: 632253
  Show dependency treegraph
 
Reported: 2011-03-22 14:52 PDT by Igor Bukanov
Modified: 2011-07-12 08:22 PDT (History)
5 users (show)
See Also:
Crash Signature:
  ---
  fixed
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  unaffected
  ---
  unaffected
  ---
  unaffected


Attachments
Fix (8.97 KB, patch)
2011-03-23 15:24 PDT, Michael Wu [:mwu]
igor: review+
Details | Diff | Splinter Review
Use guard object (775 bytes, patch)
2011-03-24 14:36 PDT, Michael Wu [:mwu]
jwalden+bmo: review+
Details | Diff | Splinter Review

Summon comment box

Description Igor Bukanov 2011-03-22 14:52:15 PDT
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.
Comment 1 Michael Wu [:mwu] 2011-03-22 18:21:08 PDT
What do you want to do here? I could move the js_SaveScriptFilename call back to js_XDRScriptAndSubscripts. Would be a bit ugly.
Comment 2 Igor Bukanov 2011-03-22 19:02:06 PDT
(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.
Comment 3 Igor Bukanov 2011-03-23 06:28:39 PDT
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.
Comment 4 Michael Wu [:mwu] 2011-03-23 09:35:21 PDT
I'll see what I can do.
Comment 5 Michael Wu [:mwu] 2011-03-23 15:24:13 PDT
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 6 Igor Bukanov 2011-03-24 05:04:55 PDT
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.
Comment 7 Michael Wu [:mwu] 2011-03-24 13:18:04 PDT
http://hg.mozilla.org/tracemonkey/rev/4484468a829b
Comment 8 Michael Wu [:mwu] 2011-03-24 13:50:14 PDT
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..
Comment 9 Jeff Walden (remove +bmo to email) 2011-03-24 14:08:47 PDT
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.
Comment 10 Michael Wu [:mwu] 2011-03-24 14:36:55 PDT
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.
Comment 11 Jeff Walden (remove +bmo to email) 2011-03-24 14:53:15 PDT
http://hg.mozilla.org/tracemonkey/rev/3279d69de777 for the guard-macro followup

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