Bug 32626 – Move some code from Database::~Database() to Database::close()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

Created attachment 45004 [details]
patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45004 [details] without any errors.

[Michael Nordman]

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.

[Dumitru Daniliuc]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45031 [details] without any errors.

[Michael Nordman]

> 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

[Dumitru Daniliuc]

Can a reviewer please take a look at this patch?

[Dmitry Titov]

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);

[Dumitru Daniliuc]

(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!

[Dumitru Daniliuc]

Landed as r52367.