[webkit-reviews] review canceled: [Bug 32626] Move some code from Database::~Database() to Database::close() : [Attachment 45004] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 16 18:41:27 PST 2009
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.
More information about the webkit-reviews
mailing list