Bug 110052 – webdatabase: Need to fix some SQLTransaction leaks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Lam]

With https://bugs.webkit.org/show_bug.cgi?id=104750, there is now a window for leaking SQLTransaction and SQLTransactionBackend instances, and by extension, the Database and its extended network of objects.  This leak is intermittent (due to a race condition) and can manifest as an assertion failure in layout test fast/workers/storage/interrupt-database.html when run on a debug build.

The reason for this is because there now exists a circular reference between the SQLTransaction and its backend.  Under normal circumstances, the transaction will reach its end, and it will clean up after itself by breaking the reference cycle in the CleanAndTerminate state.  However, if the transaction is interrupted, we need to handle all the phases where the transaction can be interrupted, and do the appropriate cleanup there.

[Mark Lam]

Created attachment 188760 [details]
the proposed patch.

[Mark Lam]

Created attachment 188761 [details]
patch + some minor adjustments and comment additions.

[Geoffrey Garen]

Comment on attachment 188761 [details]
patch + some minor adjustments and comment additions.

View in context: https://bugs.webkit.org/attachment.cgi?id=188761&action=review

359407    // Now that we're done, break the reference cycle that keeps us alive.
360408    // See comment about the life-cycle above.
361409    m_frontend = 0;

Either this comment is incorrect, or this code is incorrect. This function references data members after nulling m_frontend, so if that action might delete this, it is wrong to do it here.

> Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.cpp:100
> +        m_transactionQueue.clear();

This line should is redundant: the loop above clears the queue.

> Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.cpp:105
> +    // Must ref() before calling databaseThread()->recordDatabaseClosed().

Why? This comment should explain what recordDatabaseClosed might do to cause us problems. Otherwise, it just duplicates the line of code below it.

> Source/WebCore/Modules/webdatabase/DatabaseTask.cpp:174
> +    m_wasExecuted = true;

Since this value is set by "doPerformTask", let's call it "m_didPerformTask". "m_wasExecuted" implies that a function named execute was called.

> Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:385
> +    // Note: if doCleanup() has already been invoked, then m_frontend would have
> +    // been nullified.

I think the line of code below, which nullifies m_fronted, is better documentation than this comment. Now that you've moved this code into a better place, I think it's self-documenting.

You could make this even better by moving the assignment to right under the early return, if that's sound.

[Mark Lam]

(In reply to comment #3)
> (From update of attachment 188761 [details])
> > Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:385
> > +    // Note: if doCleanup() has already been invoked, then m_frontend would have
> > +    // been nullified.
> 
> I think the line of code below, which nullifies m_fronted, is better documentation than this comment. Now that you've moved this code into a better place, I think it's self-documenting.
> 
> You could make this even better by moving the assignment to right under the early return, if that's sound.

Thanks for the review.  I've made adjustments for all the feedback provided except for this last one.  I removed the comment, but did not move the setting of "m_frontend = 0".  There is still come clean up work to do in doCleanup() that relies on the SQLTransactionBackend being alive.  If I set m_frontend = 0, the script thread can now asynchronously deref the SQLTransactionBackend and delete it before we're done cleaning up.  Hence, we should leave nullifying m_frontend till the end.

Patch landed in r 143271: <http://trac.webkit.org/changeset/143271>.

[Mark Lam]

(In reply to comment #4)
> If I set m_frontend = 0, the script thread can now asynchronously deref the SQLTransactionBackend and delete it before we're done cleaning up.  Hence, we should leave nullifying m_frontend till the end.

I stand corrected.  The caller of doCleanup() is responsible for holding a ref to the SQLTransactionBackend while doCleanup() is being called.  So, I moved the nullifying of m_frontend up as Geoff suggested.

Follow up patch landed in r143282: <http://trac.webkit.org/changeset/143282>.