These were HeapPtr, but appear to only be used from the stack now, probably from bug 827258. This iter is sometimes needed on the stack across a GC and it was hard to push a |cx| down to all of the places where ion::InlineFrameIter gets used, so I just made an AutoRooter for it. With this patch applied I only have timeouts left (t=20) on jit-tests (no tbpl).

Comment on attachment 701356 [details] [diff] [review] v0 Review of attachment 701356 [details] [diff] [review]: ----------------------------------------------------------------- WFM

Comment on attachment 701356 [details] [diff] [review] v0 Review of attachment 701356 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfun.cpp @@ +99,5 @@ > vp.setNull(); > > /* Find fun's top-most activation record. */ > StackIter iter(cx); > + StackIter::AutoRooter iterRooter(cx, &iter); Hum, that's an awful solution. Would it be possible to make StackIter inherit from the AutoRooter and reuse the cx of the constructor? All use cases of Ion iterators and StackIter are located on the stack. In addition Ion iterators can be used without StackIter in some cases. ::: js/src/vm/Stack.cpp @@ +2041,5 @@ > > +void > +StackIter::trace(JSTracer *trc) > +{ > + ionInlineFrames_.trace(trc); Guard that StackIter is settled on a scripted Ion frame. ::: js/src/vm/Stack.h @@ +1959,4 @@ > }; > > /* A filtering of the StackIter to only stop at scripts. */ > class ScriptFrameIter : public StackIter Using inheritance would be better to avoid patching all locations which are using a filtered variant of StackIter.

(In reply to Terrence Cole [:terrence] from comment #0) > This iter is sometimes needed on the stack across a GC and it was hard to > push a |cx| down to all of the places where ion::InlineFrameIter gets used, > so I just made an AutoRooter for it. I think you can always get the context from somewhere: StackIter(JSContext *cx, SavedOption); -> cx StackIter(JSRuntime *rt, StackSegment &seg); -> seg.cx() StackIter(const StackIter &iter); -> none needed StackIter(const Data &data); -> data.maybecx_ is non-NULL (and just add an assert). In the StackIter methods, data_.seg_ should be non-NULL so you can use data_.seg->cx() In IonFrames and Bailouts::dump() you can use GetIonContext()->cx if there's no cx available.

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #2) > Comment on attachment 701356 [details] [diff] [review] > v0 Thank you for the feedback! > ::: js/src/jsfun.cpp > @@ +99,5 @@ > > vp.setNull(); > > > > /* Find fun's top-most activation record. */ > > StackIter iter(cx); > > + StackIter::AutoRooter iterRooter(cx, &iter); > > Hum, that's an awful solution. Would it be possible to make StackIter > inherit from the AutoRooter and reuse the cx of the constructor? Yes, that would be preferable, however, it would then look different from every other place where we use AutoGCRooter :-/. I've been meaning to go through and fix up all of these at once, but I haven't had the time. (In reply to Jan de Mooij [:jandem] from comment #3) > (In reply to Terrence Cole [:terrence] from comment #0) > > This iter is sometimes needed on the stack across a GC and it was hard to > > push a |cx| down to all of the places where ion::InlineFrameIter gets used, > > so I just made an AutoRooter for it. > > I think you can always get the context from somewhere: > > StackIter(JSContext *cx, SavedOption); -> cx > StackIter(JSRuntime *rt, StackSegment &seg); -> seg.cx() > StackIter(const StackIter &iter); -> none needed > StackIter(const Data &data); -> data.maybecx_ is non-NULL (and just add an > assert). > > In the StackIter methods, data_.seg_ should be non-NULL so you can use > data_.seg->cx() > > In IonFrames and Bailouts::dump() you can use GetIonContext()->cx if there's > no cx available. Thank you! This is exactly what I wanted to know! I'm going to rewrite this patch using Rooted. Do you mind if I flag you for review?

(In reply to Terrence Cole [:terrence] from comment #4) > Do you mind if I flag you for review? No problem :)

This uses a completely different (e.g. the correct) strategy to exactly root the GC things in InlineFrameIter. Nicolas asked for review of this today, so: here.

Comment on attachment 702110 [details] [diff] [review] v1: And now for something completely different. Review of attachment 702110 [details] [diff] [review]: ----------------------------------------------------------------- This sounds good. Still I wonder if we could initialize the Rooted object with something else than a cx, or using the case of a NULL cx to skip the rooting because some cases are in functions which will never cause any GC and in which we can add an AssetNoGC without any trouble. What do you think? ::: js/src/ion/IonFrameIterator-inl.h @@ +65,5 @@ > // Take arguments of the caller (parent inlined frame) it holds all actual > // arguments, needed in case of overflow, and because the analyze phase > // disable Ion inlining if the function redefine its arguments with JSOP_SETARG. > > + InlineFrameIterator it(cx, this); This function does not call into the GC, can we use another trick to avoid rooting stuff? (see other comment below) ::: js/src/ion/IonFrames.cpp @@ +986,5 @@ > InlineFrameIterator::isConstructing() const > { > // Skip the current frame and look at the caller's. > if (more()) { > + InlineFrameIterator parent(GetIonContext()->cx, this); isConstructing is not allocating anything, would there be a way to substitute the use of the JSContext* by AutoAssertNoGc class to give to the Rooted* objects creation? @@ +1014,5 @@ > } while (!parent.done() && !parent.isScripted()); > > if (parent.isScripted()) { > // In the case of a JS frame, look up the pc from the snapshot. > + InlineFrameIterator inlinedParent(GetIonContext()->cx, &parent); same thing here. ::: js/src/vm/Stack.cpp @@ +1486,5 @@ > > StackIter::StackIter(JSRuntime *rt, StackSegment &seg) > : data_(rt, &seg) > #ifdef JS_ION > + , ionInlineFrames_(seg.cx(), (js::ion::IonFrameIterator*) NULL) nit: can you initialize the data_.maybecx_ field the same way, and rename it cx_ instead? @@ +1514,3 @@ > #endif > { > + JS_ASSERT(data.maybecx_); Wouldn't the Rooted<T>::init segv before this assertion can be executed? comment: The only case in which we can provide a Data input, is if we have been able to run copyData, which implies having a non-NULL cx for allocating the Data. So, as long as this constructor is not used within a StackIter function to clone it-self, this should be fine.

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=835b378a9bc0 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/1739c6c51d1c