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
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
