[webkit-dev] RFC: Improving “RefPtr inside destructor” checking

[Geoff Garen]

It’s a use-after-free error to create a RefPtr<T> during ~T(), and have that RefPtr live past the end of ~T().

In debug builds, we try to catch this error by eagerly assertion !T::m_deletionHasBegun in the RefPtr<T> constructor.

At the same time, our smart pointer rules sometimes require us to use RefPtr<T> inside functions that are reachable from destructors. To suppress the assertion in these cases, we use RefPtrAllowingPartiallyDestroyed<T>.

Problems I see with the current approach:
* We don’t do any checking in release builds
* RefPtrAllowingPartiallyDestroyed<T> complicates our smart pointer rules, when we want them to be simple

I’d like to improve this situation.

Enable checking in release builds 

There are two ways we can check in release builds without adding overhead.

Option 1: deref() checks for outstanding pointers after running ~T(), and if there are some, crashes eagerly.

We did not like this behavior in CheckedPtr<T>. It produced crash backtraces where we simply didn’t know what pointer had lived to long, or which code had made a mistake. But RefPtr<T> is different. If Option 1 crashes, we know that the error of escaping a pointer after destruction happened inside the destructor that is crashing (or one of its callees). So maybe we have enough information to go on.

Option 2: Scribble and leak the object, and crash later, just like CheckedPtr.

Option 2 has the upside that the crashing backtrace will usually point directly to the errant pointer.

Option 2 seems fine too, but it has the downside that, if you escape a pointer in the destructor, and then make more pointers after the destructor, the pointer that ultimately crashes may be far removed from the pointer the originally escaped.

Any strong preferences, or should I just try something?

Remove RefPtrAllowingPartiallyDestroyed<T> and the debug-only !T::m_deletionHasBegun check.

Once we have a form of checking that works in release builds, the existing debug-only assertion is arguably redundant. Getting rid of it and getting rid of RefPtrAllowingPartiallyDestroyed<T> can simplify our smart pointer rules.

One downside to removing RefPtrAllowingPartiallyDestroyed<T> it that it can be nice during code review to enforce a discipline that RefPtrAllowingPartiallyDestroyed<T> should only be used on the stack. (This discipline is pretty obvious in a lot of cases, but not all — see WebCore::HTMLMediaElement::speakCueText and WebCore:: MediaSource::ensureWeakOnHTMLMediaElementContext.)

Any strong preferences, or should I just try something?

Thanks,
Geoff
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20240502/412700c0/attachment.htm>