These are currently using the (public) AutoRooter infrastructure to accomplish what is a quite dangerous and specialized task that is only used within the scary parts of the GC. Let's make something simpler and more specialized for this use. More importantly, let's improve the comment. Usually, understanding the comment in enough depth to rewrite it makes me feel safer. Not so much this time. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dda86d6d8dc

Comment on attachment 8714604 [details] [diff] [review] 3.1_simplify_wrapper_tracing-v0.diff This looks green.

Comment on attachment 8714604 [details] [diff] [review] 3.1_simplify_wrapper_tracing-v0.diff Review of attachment 8714604 [details] [diff] [review]: ----------------------------------------------------------------- This patch removes some code that was desperately in need of removal. This is r=me, since whatever happens with the const-ness isn't a big deal given the limited usage. ::: js/src/jscompartment.h @@ +900,2 @@ > > + Value operator[](size_t aIndex) const { return storage[aIndex]; } So you can't modify an entry using []? I'd expect this to return a Value&. @@ +900,4 @@ > > + Value operator[](size_t aIndex) const { return storage[aIndex]; } > + Value* begin() { return storage.begin(); } > + Value* end() { return storage.end(); } ...especially since you can modify things via *begin = XXX, and (I think?) for (auto& x : wrapperVec) { x = UndefinedValue(); } ::: js/src/proxy/CrossCompartmentWrapper.cpp @@ +467,5 @@ > // Some cross-compartment wrappers are for strings. We're not > // interested in those. > const CrossCompartmentKey& k = e.front().key(); > if (k.kind != CrossCompartmentKey::ObjectWrapper) > continue; Totally unrelated to this patch, but the comment and the code are rather different. This doesn't just skip strings; it skips DebuggerScripts and DebuggerObjects and other DebuggerStuff. Which perhaps never reach here, or are ok for some reason? But if so, the comment should discuss it and/or the code should assert it. @@ +470,5 @@ > if (k.kind != CrossCompartmentKey::ObjectWrapper) > continue; > > + NonEscapingWrapperVector wobj(cx->runtime()); > + wobj.infallibleAppend(e); Why does this need to be infallible? Returning false wouldn't do the right thing? (Totally believable, given what this is, but then why does it even have a bool return value?) @@ +611,5 @@ > } > } > > // Recompute all the wrappers in the list. > + for (const Value& v : toRecompute) { It's nice to see WrapperValue gone. Its similar look to ObjectValue makes it look like a very generic thing, which is a fairly terrifying concept.

Comment on attachment 8714604 [details] [diff] [review] 3.1_simplify_wrapper_tracing-v0.diff Review of attachment 8714604 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/proxy/CrossCompartmentWrapper.cpp @@ +470,5 @@ > if (k.kind != CrossCompartmentKey::ObjectWrapper) > continue; > > + NonEscapingWrapperVector wobj(cx->runtime()); > + wobj.infallibleAppend(e); (Is this the infallibleAppend question you had for me several days ago, and you have an explicit reserve() added locally? Looks insta-assert if not.)

(In reply to Steve Fink [:sfink, :s:] from comment #3) > Comment on attachment 8714604 [details] [diff] [review] > 3.1_simplify_wrapper_tracing-v0.diff > > Review of attachment 8714604 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch removes some code that was desperately in need of removal. > > This is r=me, since whatever happens with the const-ness isn't a big deal > given the limited usage. > > ::: js/src/jscompartment.h > @@ +900,2 @@ > > > > + Value operator[](size_t aIndex) const { return storage[aIndex]; } > > So you can't modify an entry using []? I'd expect this to return a Value&. D'oh. That's just a mistake. We happen to not use it in the two cases where this is used. > ::: js/src/proxy/CrossCompartmentWrapper.cpp > @@ +467,5 @@ > > // Some cross-compartment wrappers are for strings. We're not > > // interested in those. > > const CrossCompartmentKey& k = e.front().key(); > > if (k.kind != CrossCompartmentKey::ObjectWrapper) > > continue; > > Totally unrelated to this patch, but the comment and the code are rather > different. This doesn't just skip strings; it skips DebuggerScripts and > DebuggerObjects and other DebuggerStuff. Which perhaps never reach here, or > are ok for some reason? But if so, the comment should discuss it and/or the > code should assert it. I agree. Unfortunately, I have no idea what the answer to your question is. > @@ +470,5 @@ > > if (k.kind != CrossCompartmentKey::ObjectWrapper) > > continue; > > > > + NonEscapingWrapperVector wobj(cx->runtime()); > > + wobj.infallibleAppend(e); > > Why does this need to be infallible? Returning false wouldn't do the right > thing? (Totally believable, given what this is, but then why does it even > have a bool return value?) This is infallible because we've pre-allocated 1 slot via the template instanciation (for this case, specifically). Unfortunately, this is the wrong API (see Waldo's comment below). The correct code is MOZ_ALWAYS_TRUE(wobj.append(e)); > @@ +611,5 @@ > > } > > } > > > > // Recompute all the wrappers in the list. > > + for (const Value& v : toRecompute) { > > It's nice to see WrapperValue gone. Its similar look to ObjectValue makes it > look like a very generic thing, which is a fairly terrifying concept. Exactly! (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > Comment on attachment 8714604 [details] [diff] [review] > 3.1_simplify_wrapper_tracing-v0.diff > > Review of attachment 8714604 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/proxy/CrossCompartmentWrapper.cpp > @@ +470,5 @@ > > if (k.kind != CrossCompartmentKey::ObjectWrapper) > > continue; > > > > + NonEscapingWrapperVector wobj(cx->runtime()); > > + wobj.infallibleAppend(e); > > (Is this the infallibleAppend question you had for me several days ago, and > you have an explicit reserve() added locally? Looks insta-assert if not.) Yes, exactly. It was. I fixed it. I waited for the green try run, but apparently forgot to upload the new patch. Oh well, it was only a one-line change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7acb1edc3f914e4184b3626fe615377750067a89 Bug 1244956 - Simplify Wrapper rooting mechanism; r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6e49542ca9fdf8ee2db825864962361d6bc91f Backout 7acb1edc3f91 (bug 1244956) for regressing tpaint.

This regressed Tpaint. Maybe a pair of vectors will work better. In any case I'll run it through Tpaint before landing again.

Actually, wrapper remapping and recomputation is only implicated in navigation, not in painting. I should not have backed this out.