You get this crash, e.g., when running run-javascriptcore-tests.
Created attachment 5273 [details] Reduction
Created attachment 5274 [details] First cut at fix This patch does three things: (1) Standardizes all our linked lists of nodes to use "next" as their next pointers. (2) Creates the ListRefPtr<T> class, which iteratively, instead of recursively, destroys the list. (3) Standardizes our recursively defined linked list nodes to use ListRefPtr<T> and implement the necessary releaseNext() member function. I'm not sure about this solution. I really like the idea of ListRefPtr<T>, which Darin and Maciej and I have already talked about, but I can't seem to make it simple enough. All ListRefPtr<T> should do is implement release(), along with a custom 4-line-ish destructor. However, I haven't had success just defining ListRefPtr<T> as a subclass of RefPtr<T>, so I've had to create a whole new file and copy and paste all the template definitions, which seems unwieldy to maintain, going forward. Shouldn't I just be able to subclass? When I tried that, it seemed like ListRefPtr<T> didn't inherit any of RefPtr<T>'s member functions. For example, operator= didn't work. If that shouldn't work, what about changing RefPtr<T> to take a template releaseNext() function pointer that defaults to 0? I got close on that, but couldn't get the "default to 0" behavior to compile: template <typename T, T* (*releaseNext)() = 0> class RefPtr was what I tried. The compiler basically said it refused to case a "uint" to a function pointer. ??? pointer = 0 is c++ bread and butter. Someone with more template fu... help?
I think some things like constructors and assignment operators do not inherit.
*** Bug 5223 has been marked as a duplicate of this bug. ***
I made a few suggestions to Geoff on IRC: 1) You can inherit from RefPtr but you will have to implement all constructors and destructors and call superclass explicitly. 2) + PassRefPtr<T> reaper = PassRefPtr<T>::adopt(m_ptr); The local variable here can just be a RefPtr. 3) Same as above, adopting m_ptr like that will cause it to be released twice. Instead, make release() on RefPtr return a PassRefPtr and do this: RefPtr<T> reaper = release(); 4) Make release() give a PassRefPtr instead of a raw pointer (and releaseNext() as well), like so: PassRefPtr<T> release() { PassRefPtr<T> tmp = adoptRef(m_ptr); m_ptr = 0; return tmp; } If you do 1-4, you should not need to use adopt at all in the destruction loop.
Comment on attachment 5274 [details] First cut at fix Marking r- due to the possible deref underflow. Please consider style issues as well.
Created attachment 5292 [details] Second cut at fix This one should do the trick. If you're wondering about the wonky this-> syntax, it's their to disambiguate the template mumbo-jumbo. Let me know if there's a snazzier way to get it to compile.
Created attachment 5293 [details] Second cut at fix Oops. Last patch had stray '{' '}' in it.
Comment on attachment 5293 [details] Second cut at fix Looks delicious. r=me Make sure to land the test case with this.
Landed with updated mozilla test that crashes (would crash) deployment as well as development builds.