477142 - _FAIL builtins need to be GC-safe

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

JSAutoTempIdRooter is to keep the jsid-vs-value distinction alive.  I'm not terribly attached to it.

Comment 2

[Brendan Eich [: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

Comment 3

[Jason Orendorff [:jorendorff]]

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.

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[Brendan Eich [:brendan]]

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 7

[Brendan Eich [:brendan]]

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

r=me with name nits.

/be

Comment 8

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/f2f5a9f18e25

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2ce90221e80c

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c7a867913d3f