643927 - GC hazard in JS_XDRScriptObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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]]

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]

(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]

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]]

I'll see what I can do.

Comment 5

[Michael Wu [:mwu]]

Attachment: 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]

Comment on attachment 521327 [details] [diff] [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]]

http://hg.mozilla.org/tracemonkey/rev/4484468a829b

Comment 8

[Michael Wu [:mwu]]

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 [:Waldo]]

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]]

Attachment: 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 [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/3279d69de777 for the guard-macro followup

Comment 12

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/4484468a829b
http://hg.mozilla.org/mozilla-central/rev/c6388fb2d7be
http://hg.mozilla.org/mozilla-central/rev/3279d69de777