[webkit-reviews] review granted: [Bug 110052] webdatabase: Need to fix some SQLTransaction leaks : [Attachment 188761] patch + some minor adjustments and comment additions.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 110052: webdatabase: Need to fix some SQLTransaction leaks
https://bugs.webkit.org/show_bug.cgi?id=110052

Attachment 188761: patch + some minor adjustments and comment additions.
https://bugs.webkit.org/attachment.cgi?id=188761&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.