[webkit-reviews] review denied: [Bug 31206] Database can be the last to deref Document, resulting in ~Document on the Database thread. : [Attachment 42652] thunk to the main thread to deref Database's m_document

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kevin Watters
<kevinwatters at gmail.com>'s request for review:
Bug 31206: Database can be the last to deref Document, resulting in ~Document
on the Database thread.
https://bugs.webkit.org/show_bug.cgi?id=31206

Attachment 42652: thunk to the main thread to deref Database's m_document
https://bugs.webkit.org/attachment.cgi?id=42652&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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.