Remove the unneeded FrameState structure from the stack capture code.

Comment on attachment 8549609 [details] [diff] [review] The patch Review of attachment 8549609 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SavedStacks.cpp @@ +92,5 @@ > private: > + SavedFrame::AutoLookupRooter *ref; > +}; > + > +class SavedFrame::AutoLookupVector : public JS::CustomAutoRooter { Uh, the MOZSTACK_CLASS change didn't make it to the patch. I've updated it now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=433165ae151b

Okay, I've been a little bit too optimistic in assuming the fact the code built without errors meant it had memory correctness. In the end I had to use two separate rooting classes, a rooting vector of Lookup* and the normal AutoLookupRooter. The memory handling is the best I came up with, but Lookup instances have to be allocated with "new" and "delete" explicitly. I'd ordinarily have used nsRefPtr but it seems it's not available here. Suggestions for cleaning this up are welcome. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb16f1c12285 In retrospect, I think the old code might have been copy constructing much more than needed.

Comment on attachment 8549671 [details] [diff] [review] This actually works Review of attachment 8549671 [details] [diff] [review]: ----------------------------------------------------------------- Rather than using new/delete directly, you should choose the appropriate function based on the scenario: https://dxr.mozilla.org/mozilla-central/source/js/public/Utility.h?from=js_new#141-181 Additionally, ScopedDeletePtr can help clean up failure branches: https://dxr.mozilla.org/mozilla-central/source/mfbt/Scoped.h?from=ScopedDeletePtr#238-243 (Here is some example usage: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp#209) However, I think we can avoid all of that here. Rather than defining copy constructors for `SavedFrame::Lookup` or using pointers, let's define move constructors (`Lookup(Lookup &&rhs)`) which transfer ownership of internal data rather than copying it. This lets us avoid messing with raw pointers and incurring malloc overhead for a stack-lifetime object, but also avoids the performance degradation of unnecessary copy construction. Best of both worlds! https://dxr.mozilla.org/mozilla-central/source/mfbt/Vector.h?from=VectorBase#510-515 https://dxr.mozilla.org/mozilla-central/source/mfbt/Move.h#16-193

(Sorry, I should have caught this before, when you flagged me in bug 1083359)

Thanks for all the information! It was really helpful. I've then seen we can implement the much simpler technique of initializing Vector's memory in place, that is another step forward from the move constructor idea: http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#6412 I've also found the RootedGeneric template class from bug 1020690 that works on any object implementing a "trace" method: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Shape-inl.h#133 Unfortunately, it has no support for mutability (no "set" method), and apparently doesn't work with JS::Handle<T> and JS::MutableHandle<T>. Any way of making the two work together (best), or alternatively define new template classes (if the former cannot be done)? See for example how we implement our own AutoRooter and MutableHandle classes for LocationValue, that could easily be one-liners.

Comment on attachment 8550976 [details] [diff] [review] Another approach Review of attachment 8550976 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! > Unfortunately, it has no support for mutability (no "set" method), and > apparently doesn't work with JS::Handle<T> and JS::MutableHandle<T>. > Any way of making the two work together (best), or alternatively define > new template classes (if the former cannot be done)? You could always add a set method, no? As for expanding it to support handle vs mutable handle: I think you could do by defining HandleGeneric<T> and MutableHandleGeneric<T> and adding the various implicit constructors. The patch looks good to me as-is, FWIW.

I saw one or two braced single-line loop bodies that shouldn't be braced, FYI. (In reply to Nick Fitzgerald [:fitzgen] from comment #4) > Additionally, ScopedDeletePtr can help clean up failure branches: > https://dxr.mozilla.org/mozilla-central/source/mfbt/Scoped. > h?from=ScopedDeletePtr#238-243 (Here is some example usage: > https://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp#209) Scoped as a class is deprecated, and ScopedDeletePtr is *particularly* deprecated. You should be using UniquePtr instead.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8) Thanks for catching these!