Currently, DatabaseTracker is notified that a Database is closing/closed in the Database's destructor. At that point, we are very limited in what we can do, because the Database object is being destroyed. Instead, it seems like the correct thing to do is notify the DatabaseTracker that a database is being closed in the close() method.
Created attachment 45004 [details] patch
style-queue ran check-webkit-style on attachment 45004 [details] without any errors.
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. 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.
Created attachment 45031 [details] patch (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.
style-queue ran check-webkit-style on attachment 45031 [details] without any errors.
> you're right. i think this should be fixed now, please take another look. 365 DatabaseTracker::tracker().removeOpenDatabase(this); 366 ref(); 367 callOnMainThread(documentRemoveOpenDatabase, this); that works for me
Can a reviewer please take a look at this patch?
r=me On landing, could you please add a small comment against the ref() to say where is a balancing deref()? 366 ref(); 367 callOnMainThread(documentRemoveOpenDatabase, this);
(In reply to comment #8) > r=me > > On landing, could you please add a small comment against the ref() to say where > is a balancing deref()? > > 366 ref(); > 367 callOnMainThread(documentRemoveOpenDatabase, this); Will do. Thanks for taking a look at this patch!
Landed as r52367.