Bindings stores a Shape pointer and is itself stored both on the stack and on the heap. It currently stores the Shape* as a HeapPtrShape. This leads to a rooting problem when we store the Bindings on the stack because the exact rooting does not know about the internal Shape* in the Bindings.

This approach templatizes Bindings on the underlying Shape* type.

This patch cleans up the way we do root analysis and exact rooting so that we can specialize it enough to make it possible to add a Rooted for Bindings. I think this patch is a good enough cleanup to stand on it's own, so I've split it out.

This patch implements and uses Rooted<Bindings> using the new analysis functionality. It also solves the issue uncovered by the root analysis, but less succinctly: since the internal pointer is still a HeapPtr stored on the stack, we have to be careful to ensure correct ordering between operator-> and operator= when assigning to it from a fallible method so that we do not corrupt the HeapPtr's |this| during operator=.

Comment on attachment 664186 [details] [diff] [review] v0: templatized version Review of attachment 664186 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.h @@ +215,5 @@ > > void trace(JSTracer *trc); > }; > > +typedef InternalHandle<BindingsBase<js::HeapPtrShape>*> HeapBindingsHandle; Could you call this InternalBindingsHandle? @@ +216,5 @@ > void trace(JSTracer *trc); > }; > > +typedef InternalHandle<BindingsBase<js::HeapPtrShape>*> HeapBindingsHandle; > +typedef InternalHandle<BindingsBase<RootedShape>*> StackBindingsHandle; If I understand InternalHandle correctly, I don't think it would ever make sense to have a StackBindingsHandle. ::: js/src/vm/ScopeObject.cpp @@ +1122,5 @@ > JSScript *script = callobj.callee().script(); > if (!script->ensureHasTypes(cx)) > return false; > > + HeapBindings &bindings = script->bindings; Could you make this an InterbalHeapBindingsHandle instead? It seems safer.

Comment on attachment 664191 [details] [diff] [review] v0: templatize root analysis APIs Review of attachment 664191 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather not do this. It just seems to add more indirection that we don't actually need.

Comment on attachment 664192 [details] [diff] [review] v0: Rooted<Bindings> Review of attachment 664192 [details] [diff] [review]: ----------------------------------------------------------------- I guess this is okay, except for the bits that overlap with the previous patch.

I realized when updating the exact marking code that I forgot to update it when I added rooters to the runtime, so I folded that change in as well.

Currently, BindingIter stores a Bindings*: this is sometimes a pointer into a JSScript, which causes the analysis to poison it. This patch straightforwardly replaces this Bindings* with an InternalBindingsHandle.

Comment on attachment 665177 [details] [diff] [review] v0 Review of attachment 665177 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsanalyze.cpp @@ +141,5 @@ > bool allVarsAliased = script->compartment()->debugMode(); > bool allArgsAliased = allVarsAliased || script->argumentsHasVarBinding(); > > + RootedScript script_(cx, script); > + for (BindingIter bi(script_); bi; bi++) { It looks like Nick changed this recently, and you can just use the script_ field directly without need for the rooted. ::: js/src/jsscript.cpp @@ +267,1 @@ > for (BindingIter bi(bindings); bi; bi++) { It seems like you could just pass fromScript into the bi constructor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdce684b523 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c611cc8df4

https://hg.mozilla.org/mozilla-central/rev/7cdce684b523 https://hg.mozilla.org/mozilla-central/rev/c2c611cc8df4