[webkit-reviews] review canceled: [Bug 32626] Move some code from Database::~Database() to Database::close() : [Attachment 45004] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has canceled Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 32626: Move some code from Database::~Database() to Database::close()
https://bugs.webkit.org/show_bug.cgi?id=32626

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(In reply to comment #3)
> 355	  if (m_document->databaseThread())
> 356	      m_document->databaseThread()->unscheduleDatabaseTasks(this);
> 
> Given the ASSERT on line 335, looks like we don't need to test for null'ness
> here.

good point. removed.

> 359	  m_document->removeOpenDatabase(this);
> 
> This looks like a threading bug waiting to happen? The doc->addOpenDatabase()

> method is being called on the main thread, but the call to
> doc->removeOpenDatabase() happens on the db thread (per the ASSERT on line
> 336). But the underlying collection being used by the Document is not thread
> safe.
> 
> This call to remove() used to be in the dtor, which contains code that
> indicates it can happen on a background thread in theory. In practice, who
> knows how often that does happen.
> 
> This change as coded will guarantee that the underlying set is accessed in a
> non-thread-safe fashion. I think you need to poke at this in some way.

you're right. i think this should be fixed now, please take another look.