Using rooting verification mode on code for the ECMAScript Internationalization API, I found and fixed a few issues in other code (self-hosting support, JIT). I'd appreciate if someone more familiar with the modified code could take it from here. Changes in the attached patch: - a few API changes from jsid to HandleId and from Value* to MutableHandleValue, plus the required changes to callers, - several additional Rooted<T> declarations and related changes. For the latter I verified that they are required, i.e., undoing any of them reintroduced assertion failures.

Comment on attachment 666364 [details] [diff] [review] patch with miscellaneous fixes Review of attachment 666364 [details] [diff] [review]: ----------------------------------------------------------------- Nice job! Just a couple small nits. I'll go ahead and apply them and get this in tree. ::: js/src/jscntxt.cpp @@ +314,5 @@ > return funVal.toObject().toFunction(); > } > > bool > +JSRuntime::cloneSelfHostedValueById(JSContext *cx, HandleId id, HandleObject holder, Value *vp) We should just make vp a MutableHandleValue here, rather than using result as a temporary. ::: js/src/methodjit/Compiler.cpp @@ +78,5 @@ > globalSlots(globalObj ? globalObj->getRawSlots() : NULL), > sps(&cx->runtime->spsProfiler), > masm(&sps, &PC), > frame(cx, *thisFromCtor(), masm, stubcc), > + a(NULL), outer(NULL), script_(cx, NULL), PC(NULL), loop(NULL), Rooted will default to a sane value (NULL for T*), so we leave if off here.

https://hg.mozilla.org/mozilla-central/rev/495a115ec2cc Should this have tests?

(In reply to Ryan VanderMeulen from comment #3) > > Should this have tests? Reliably reproducing the assertion failures that prompted these changes requires a special SpiderMonkey build (--enable-root-analysis). I assume such a build isn't produced as part of regular build and test processes, so a test case wouldn't be very useful. In addition, the test case I used relies on a pile of changes that haven't been committed yet.

Actually, we run the full jit-test suite in the rooting analysis mode. You can find it here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey under 'r'. This particular change is just moving to a new API, so the existing tests should cover this case, or new tests should get added on the bug covering the actual feature work.