[webkit-reviews] review requested: [Bug 107784] webdatabase: Refactor DatabaseContext lookup to be thread-safe : [Attachment 184687] The final patch.

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has asked  for review:
Bug 107784: webdatabase: Refactor DatabaseContext lookup to be thread-safe
https://bugs.webkit.org/show_bug.cgi?id=107784

Attachment 184687: The final patch.
https://bugs.webkit.org/attachment.cgi?id=184687&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
In addition to the work that has already been written up in the bug
description, this final patch makes DatabaseContext a sub-class of
ContextDestructionObserver.  This allows it to be notified when the
ScriptExecutionContext is destructed, and trigger its own shutdown as well as
the shutdown of its DatabaseThread and close all the Database instances held by
the thread.

Previously, I thought that it is sufficient to depend on garbage collection of
the Database instances to trigger the shutdown of the DatabaseContext and
DatabaseThread.  But Jochen pointed out that the Database instances is
currently kept alive by the DatabaseThread itself
(https://bugs.webkit.org/show_bug.cgi?id=68303).  After thinking thru the issue
some more, I decided that fixing 68303 is beyond the scope of this bug.  I'm
also not convinced yet that the solution proposed in the 68303 patch is the
best way to go (I haven't had time to digest it properly yet).	I'll leave that
for a later time.

Also, previously, I mentioned that I saw some regression in the layout tests
with debug builds.  They were actually not failures, but stderr diffs in the
console out, but were consistently manifesting when I ran a debug build with
this patch applied (in contrast with the baseline).  It turned out that the
issue there is due to https://bugs.webkit.org/show_bug.cgi?id=79013 which Adam
provided a fix for.  Once I incorporate Adam's fix, the stderr diffs went away
completely.

I should point out that with this patch, I also cleaned up the shutdown code of
DatabaseContext and DatabaseThread.  They are still only shutdown when their
associated ScriptExecutionContext is destructed.  However, they are now
guaranteed to be cleaned up.  I'm not completely sure, but there may have been
a bug before where the DatabaseThread (and its databases) associated with the
MainThread may be leaked if stopDatabases() wasn't called by someone external
to the database system (because I couldn't find it in the code).  I've changed
the code so that the thread shutdown is done if it is not already triggered by
someone else before the DatabaseContext is destructed.	And I did some testing
with some printfs to ensure that the shutdown did indeed occurred as expected
with this patch.

The patch is now ready for a review please.  Thanks.