Bug 6233 - Reproducible stack-overflow crash in ~RefPtr<T> due to RefPtr<T> use in linked lists
Summary: Reproducible stack-overflow crash in ~RefPtr<T> due to RefPtr<T> use in linke...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
: 5223 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-12-24 13:20 PST by Geoffrey Garen
Modified: 2005-12-29 03:15 PST (History)
2 users (show)

See Also:


Attachments
Reduction (122 bytes, text/html)
2005-12-24 13:21 PST, Geoffrey Garen
no flags Details
First cut at fix (31.21 KB, patch)
2005-12-24 13:33 PST, Geoffrey Garen
mjs: review-
Details | Formatted Diff | Diff
Second cut at fix (31.12 KB, patch)
2005-12-26 17:11 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Second cut at fix (31.10 KB, patch)
2005-12-26 17:13 PST, Geoffrey Garen
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2005-12-24 13:20:45 PST
You get this crash, e.g., when running run-javascriptcore-tests.
Comment 1 Geoffrey Garen 2005-12-24 13:21:07 PST
Created attachment 5273 [details]
Reduction
Comment 2 Geoffrey Garen 2005-12-24 13:33:30 PST
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?
Comment 3 Maciej Stachowiak 2005-12-24 14:02:40 PST
I think some things like constructors and assignment operators do not inherit.
Comment 4 Geoffrey Garen 2005-12-25 00:48:12 PST
*** Bug 5223 has been marked as a duplicate of this bug. ***
Comment 5 Maciej Stachowiak 2005-12-25 00:49:59 PST
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 6 Maciej Stachowiak 2005-12-25 00:50:46 PST
Comment on attachment 5274 [details]
First cut at fix

Marking r- due to the possible deref underflow. Please consider style issues as
well.
Comment 7 Geoffrey Garen 2005-12-26 17:11:01 PST
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.
Comment 8 Geoffrey Garen 2005-12-26 17:13:08 PST
Created attachment 5293 [details]
Second cut at fix

Oops. Last patch had stray '{' '}' in it.
Comment 9 Maciej Stachowiak 2005-12-27 01:33:02 PST
Comment on attachment 5293 [details]
Second cut at fix

Looks delicious. r=me

Make sure to land the test case with this.
Comment 10 Geoffrey Garen 2005-12-29 03:15:39 PST
Landed with updated mozilla test that crashes (would crash) deployment as well as development builds.