Bug 31206 – Database tries to deref Document on the Database thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kevin Watters]

If a page's Database object is the last to reference a Document, ~Database will result in ~Document running on the database thread. This hits an ASSERT on debug builds in ~Document with ASSERT(!m_stylerecalcTimer.IsActive()).

[Kevin Watters]

Created attachment 42652 [details]
thunk to the main thread to deref Database's m_document

This patch uses callOnMainThread to deref the Document object on the main thread.

[Darin Adler]

Comment on attachment 42652 [details]
thunk to the main thread to deref Database's m_document

> +static void derefDocument(void* document)
> +{
> +    (reinterpret_cast<Document*>(document))->deref();
> +}

This should be a static_cast, not a reinterpret_cast. Also, no need for the extra parentheses.

> +    // in case we're the last to reference the Document, deref it on the main thread
> +    m_document->ref();
> +    callOnMainThread(derefDocument, m_document.get());

Since the reference counting for nodes is not thread safe, it's not safe to modify the reference count of the document on a non-main thread. Doing a ref here just trades one kind of thread-safety problem for another, more subtle one that can lead to early destruction of the Document or a storage leak. The call to m_document->ref() is not correct. Instead, you have to call m_document.release().releaseRef(), which will not give you a Document* and not attempt to read or modify the reference count at all on the current thread.

[Kevin Watters]

Created attachment 42663 [details]
don't ref() on the Database thread--use release().releaseRef() instead

[Kevin Watters]

Thanks for clarifying the problems with the initial patch. With your suggestions, and the help of http://webkit.org/coding/RefPtr.html I've got a much better understanding of RefPtr now.

[Darin Adler]

Comment on attachment 42663 [details]
don't ref() on the Database thread--use release().releaseRef() instead

> +    // in case we're the last to reference the Document, deref it on the main thread
> +    callOnMainThread(derefDocument, m_document.release().releaseRef());

The comment isn't ideal.

It makes it sound like the issue is that we might be the last reference to the document. But that's not true. The real issue is that we can't manipulate the document's reference count at all on any thread other than the main thread. This issue would exist even if ours was guaranteed to never be the last reference to the document.

Further, our comment style is sentence formatting. It should be capitalized and have a period.

Neither of these issues is serious enough that I think we should review- this, but I wanted to mention both.

[Eric Seidel (no email)]

Is this the first instance of this issue, or is it worth thinking about adding a "MainThreadRefPtr" class which is used for this sort of member variable (with a better name).

[Kevin Watters]

Created attachment 42753 [details]
patch with improved comment

I attached a patch fixing the comment and flagged it for the commit queue.

Eric: as for creating a new RefPtr subclass that handles destruction on a specific thread--I wasn't confident enough that I could subclass RefPtr 100% correctly. Luckily the other RefPtr members in Database (m_securityOrigin and m_databaseAuthorizer) point to ThreadSafeShared subclasses, so I think m_document is the only member that had this problem.

Maybe a good interim solution would be a convenience function in MainThread.h like the following:

template <typename T>
static void deref(void* obj)
{
    static_cast<T*>(obj)->deref();
}

template <typename T>
static void derefOnMainThread(RefPtr<T>& ref)
{
    callOnMainThread(deref<T>, ref.release().releaseRef());
}

Then we could just "derefOnMainThread(m_document)" from ~Database. Let me know if I should create a patch for this.

[Darin Adler]

To make these bugs less mysterious we need to make it harder to make this mistake with any RefCounted object. The vast majority of objects that use RefCounted have a reference count that can only be used on the main thread.

The RefCounted class should have a feature for debugging where it asserts if ref or deref is done on the non-main thread that can be turned on and off as needed for the newer more sophisticated class. This can be only when NDEBUG is not defined.

One way to do it would be to make a class called SingleThreadRefCounted for this purpose or we could have RefCounted default to this behavior and have a function call to turn it off or modify it. A function call that would do nothing in NDEBUG builds. We can afford to have additional state stored in the object when NDEBUG is not defined. And maybe we can express the more complex policies too, for example an object that can be ref'd only on two threads or while a certain lock is held or something like that.

[WebKit Commit Bot]

Comment on attachment 42753 [details]
patch with improved comment

Clearing flags on attachment: 42753

Committed r50666: <http://trac.webkit.org/changeset/50666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.