Force us to propagate HandleValue through all the interfaces that will require this.

This turned out to be much easier than I expected: only 92K worth of patch. https://tbpl.mozilla.org/?tree=Try&rev=237bf82a388b

Please give me an sr? on this prior to landing. Better yet, please also bring it to the mailing list for general discussion and issue spotting from the community.

(In reply to David Mandelin [:dmandelin] from comment #2) > Please give me an sr? on this prior to landing. Better yet, please also > bring it to the mailing list for general discussion and issue spotting from > the community. I did not mean to give the wrong impression here: this is not adding handles to the API, it is only ensuring that all entry points to the API root immediately on entry. This does add HandleValue to callbacks from the API, however, HandleObject is already present in all of these locations, so I do not think there are any new concerns here.

(In reply to Terrence Cole [:terrence] from comment #3) > (In reply to David Mandelin [:dmandelin] from comment #2) > > Please give me an sr? on this prior to landing. Better yet, please also > > bring it to the mailing list for general discussion and issue spotting from > > the community. > > I did not mean to give the wrong impression here: this is not adding handles > to the API, it is only ensuring that all entry points to the API root > immediately on entry. I know it doesn't change the API signatures for the most part. But it does change the semantics (in the future where rooting is used), so I want to think carefully about it.

Comment on attachment 658327 [details] [diff] [review] v0 Review of attachment 658327 [details] [diff] [review]: ----------------------------------------------------------------- Well, I read through the whole patch. It was boring. Assuming the browser still compiles with it (and I assume it does considering the files you touched), it seems like it's right. Hard to tell if anything's missing just from inspection, though. It does make me wonder why we have so many jsval* parameters when it doesn't seem very useful to be able to change the pointed-at value, but I know those tend to have legitimate if unpleasant reasons. I marked it sr?, though it's not that easy to figure out the eventual API just from looking at this patch. ::: js/src/jsinterp.cpp @@ +524,5 @@ > NULL /* evalInFrame */, rval); > } > > JSBool > +js::HasInstance(JSContext *cx, HandleObject obj, MutableHandleValue v, JSBool *bp) I sorta wish |v| were named better, but that's not this patch.

(In reply to Steve Fink [:sfink] from comment #5) > It does make me wonder why we have so many jsval* parameters when it doesn't > seem very useful to be able to change the pointed-at value, but I know those > tend to have legitimate if unpleasant reasons. Most were used in the bowels of SpiderMonkey (none by the browser, I think). I was able to make one of the jsval* a HandleValue, so there was some success at least.

Comment on attachment 658327 [details] [diff] [review] v0 Review of attachment 658327 [details] [diff] [review]: ----------------------------------------------------------------- I just wanted to make sure I had the opportunity to look it over before it lands. I could probably have just asked for an r?, but it does happen to touch the API. Anyway, I don't see any problems in the API area (which was the only one I looked at). I guess I do have a question (which can be answered after landing), which is what is the purpose of rooting just below the surface? Is it because once the surface has handles, everything it calls will need to be handles, and you want to do that work ahead of the API?

(In reply to David Mandelin [:dmandelin] from comment #7) > I guess I do have a question (which can be answered after landing), which is > what is the purpose of rooting just below the surface? Is it because once > the surface has handles, everything it calls will need to be handles, and > you want to do that work ahead of the API? You are quite right. This lets us split up work on the browser from work on the engine in a nice way.

(In reply to David Mandelin [:dmandelin] from comment #7) > I guess I do have a question (which can be answered after landing), which is > what is the purpose of rooting just below the surface? Is it because once > the surface has handles, everything it calls will need to be handles, and > you want to do that work ahead of the API? Yeah, the theory is that there's a large chunk of work getting everything handle-ified within SM, and another large chunk of work getting all Gecko users handle-ified, and this is a way to do those chunks independently and incrementally: after this patch and the comparable one for JSObject* handlification, it's relatively easy to add handles to an individual API entry (or all of them) with minimal changes to SM. You just need to undo the now-redundant rooting just under the surface. This is too abstract. Here's what I mean: -- time t0 -- Gecko: jsval v; JS_Frobnicate(cx, v); SM: JS_Frobnicate(JSContext *cx, jsval v) { js::Frobnicate(cx, v); return v; } -- time t1 (rooting just below surface) -- Gecko: jsval v; JS_Frobnicate(cx, v); SM: JS_Frobnicate(JSContext *cx, jsval vArg) { RootedValue v(cx, vArg); // vArg may be invalidated by GC, but it's dead anyway js::Frobnicate(cx, v); // Now safe to run rooting analysis or moving GC return v; } -- time t2 (all or only part of Gecko updated) -- Gecko: RootedValue vr(cx, v); JS_Frobnicate(cx, vr); // RootedValue will happily decay into a plain Value/jsval SM: JS_Frobnicate(JSContext *cx, jsval vArg) { RootedValue v(cx, vArg); js::Frobnicate(cx, v); return v; } -- time t3 (all of Gecko updated, this JSAPI entry Handle-ified) -- Gecko: RootedValue vr(cx, v); JS_Frobnicate(cx, vr); SM: JS_Frobnicate(JSContext *cx, HandleValue v) { js::Frobnicate(cx, v); return v; }

Cool. Thanks for the explanation. So it seems like it's not only what I said, but that also you need this for the rooting analysis to pass. (Btw, for future reference, I like to see rationales given for all nontrivial bugs. I haven't been specifically asked for rationales too much in the past, but I do think it's important, especially as our developer community grows.)

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=4630f9a372e6 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/58bebcfa82af