Bug 6233 – Reproducible stack-overflow crash in ~RefPtr<T> due to RefPtr<T> use in linked lists

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

You get this crash, e.g., when running run-javascriptcore-tests.

[Geoffrey Garen]

Created attachment 5273 [details]
Reduction

[Geoffrey Garen]

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?

[Maciej Stachowiak]

I think some things like constructors and assignment operators do not inherit.

[Geoffrey Garen]

*** Bug 5223 has been marked as a duplicate of this bug. ***

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Comment on attachment 5274 [details]
First cut at fix

Marking r- due to the possible deref underflow. Please consider style issues as
well.

[Geoffrey Garen]

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.

[Geoffrey Garen]

Created attachment 5293 [details]
Second cut at fix

Oops. Last patch had stray '{' '}' in it.

[Maciej Stachowiak]

Comment on attachment 5293 [details]
Second cut at fix

Looks delicious. r=me

Make sure to land the test case with this.

[Geoffrey Garen]

Landed with updated mozilla test that crashes (would crash) deployment as well as development builds.