Closed Bug 477142 Opened 13 years ago Closed 13 years ago

_FAIL builtins need to be GC-safe

Categories

(Core :: JavaScript Engine, defect, P2)

Other Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

When a _FAIL builtin deep-bails, the arguments are copied from the native stack to the interpreter stack, so those values are rooted.  However, intermediate values, such as the ids in GetProperty_tn and GetElement_tn, may need TempValueRooters (if they are used after deep-bail and are not otherwise reachable).

All the _FAIL builtins ought to be reviewed.  Shouldn't take long.  I'll take this bug tomorrow if nobody beats me to it.
Flags: blocking1.9.1?
Attached patch v1Splinter Review
JSAutoTempIdRooter is to keep the jsid-vs-value distinction alive.  I'm not terribly attached to it.
Assignee: general → jorendorff
Attachment #361247 - Flags: review?(brendan)
How would these ids be prematurely collected in current code? Not by a last ditch GC, which keeps atoms.

Our C++ style does not capitalize method names, so addr and value would be best. Not sure we're totally consistent, and the mFoo member convention seems to have already made inroads (I'm against it too :-/). Early nit, main thing is whether there is actually a hazard here.

/be
Hmm.  Well, js_ValueToStringId can create a new string (e.g. if the given string isn't flat).  js_Int32ToId can too.  Suppose the call to OBJ_{GET,SET}_PROPERTY then ends up calling a C/C++ JSPropertyOp which calls JS_GC.
I would like that to be an API error or no-op (in my little pony mental playground it is ;-). Can you see any perf effects from the patch, say with SS or a talos tryserver?

/be
(In reply to comment #4)
> I would like that to be an API error or no-op (in my little pony mental
> playground it is ;-).

I dunno--the JSPropertyOp should be allowed to call JS_EvaluateUCScript, right? which could call an operation callback, which should be allowed to call JS_GC.

> Can you see any perf effects from the patch, say with SS
> or a talos tryserver?

No.  I ran SS with --runs=30.  A few SunSpider tests hit these functions: GetElement_tn is called in 3d-cube because there's a slowarray, and GetProperty_tn is called in a few tests that do "for (p in obj) {...obj[p]...}".  Any perf loss isn't enough to show up though.
Status: NEW → ASSIGNED
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment on attachment 361247 [details] [diff] [review]
v1

Safety first, but can you pick the capitalized-name nits? r=me with addr/value instead of Addr/Value.

/be
Comment on attachment 361247 [details] [diff] [review]
v1

r=me with name nits.

/be
Attachment #361247 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/f2f5a9f18e25
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/2ce90221e80c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.