Bug 108355 – Crash because DatabaseContext destructed before its ScriptExecutionContext

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jussi Kukkonen (jku)]

fast/js/caller-property.html has started crashing quite often sometime before r141210: my first guess is r141207 (bug 108275).

ASSERTION FAILED: m_scriptExecutionContext->isContextThread()
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/dom/ActiveDOMObject.cpp(65) : virtual WebCore::ActiveDOMObject::~ActiveDOMObject()
1   0x7f28e5ea029e WebCore::ActiveDOMObject::~ActiveDOMObject()
2   0x7f28e5c242f9 WebCore::DatabaseContext::~DatabaseContext()
3   0x7f28e5c2433c WebCore::DatabaseContext::~DatabaseContext()
4   0x7f28e5c13dcc WTF::RefCounted<WebCore::DatabaseContext>::deref()
5   0x7f28e5c12b0e void WTF::derefIfNotNull<WebCore::DatabaseContext>(WebCore::DatabaseContext*)
6   0x7f28e5c1b89f WTF::RefPtr<WebCore::DatabaseContext>::~RefPtr()
7   0x7f28e5c19067 WebCore::DatabaseBackend::~DatabaseBackend()
8   0x7f28e5c0e94a WebCore::Database::~Database()
9   0x7f28e5c0e982 WebCore::Database::~Database()
10  0x7f28e5c0e634 WTF::ThreadSafeRefCounted<WebCore::DatabaseBackend>::deref()
11  0x7f28e5c0e574 void WTF::derefIfNotNull<WebCore::Database>(WebCore::Database*)
12  0x7f28e5c0e43f WTF::RefPtr<WebCore::Database>::~RefPtr()
13  0x7f28e5c2c747 WTF::HashTable<WTF::RefPtr<WebCore::Database>, WTF::RefPtr<WebCore::Database>, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> > >::deallocateTable(WTF::RefPtr<WebCore::Database>*, int)
14  0x7f28e5c2ba66 WTF::HashTable<WTF::RefPtr<WebCore::Database>, WTF::RefPtr<WebCore::Database>, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> > >::~HashTable()
15  0x7f28e5c2b6bc WTF::HashSet<WTF::RefPtr<WebCore::Database>, WTF::PtrHash<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> > >::~HashSet()
16  0x7f28e5c2afff WebCore::DatabaseThread::databaseThread()
17  0x7f28e5c2adaa WebCore::DatabaseThread::databaseThreadStart(void*)
18  0x7f28e72128f1
19  0x7f28e723a44a
20  0x7f28e0520e9a
21  0x7f28ea03acbd clone

[Jussi Kukkonen (jku)]

and same assert happens in:
  storage/websql/close-during-stress-test.html
  storage/websql/sql-error-codes.html

[Mark Lam]

(In reply to comment #0)
> fast/js/caller-property.html has started crashing quite often sometime before r141210: my first guess is r141207 (bug 108275).

Not related to bug 108275.  Possibly caused by https://bugs.webkit.org/show_bug.cgi?id=107784.

[Mark Lam]

(In reply to comment #0)
> ASSERTION FAILED: m_scriptExecutionContext->isContextThread()
> /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/dom/ActiveDOMObject.cpp(65) : virtual WebCore::ActiveDOMObject::~ActiveDOMObject()
> 1   0x7f28e5ea029e WebCore::ActiveDOMObject::~ActiveDOMObject()
...
> 3   0x7f28e5c2433c WebCore::DatabaseContext::~DatabaseContext()
> 4   0x7f28e5c13dcc WTF::RefCounted<WebCore::DatabaseContext>::deref()
...
> 9   0x7f28e5c0e982 WebCore::Database::~Database()
...
> 16  0x7f28e5c2afff WebCore::DatabaseThread::databaseThread()
> 17  0x7f28e5c2adaa WebCore::DatabaseThread::databaseThreadStart(void*)
...

The crash stack trace shows that the DatabaseContext is being destructed from the DatabaseThread.  This should be fine.  However, the failed assertion only triggers if the DatabaseContext is still associated with the ScriptExecutionContext.  The code to disassociate the DatabaseContext can only be safely called from the script thread (hence, the assertion in ActiveDOMObject).  Hence, the DatabaseContext needs to stay alive until the ScriptExecutionContext destructs and calls (from the script thread) the relevant DatabaseContext functions to do the disassociation.

Therefore, the fix is to add a m_selfRef to DatabaseContext that will keep itself alive until DatabaseContext::contextDestroyed() is called.   contextDestroyed() will nullify m_selfRef and allow the DatabaseContext to be safely destructed thereafter.

The patch for the fix will be coming shortly.

[Mark Lam]

Created attachment 185666 [details]
The fix.

[Geoffrey Garen]

Comment on attachment 185666 [details]
The fix.

The standard way to indicate that DatabaseContext must survive until ScriptExecutionContext is destroyed is to give ScriptExecutionContext a RefPtr to DatabaseContext. Why isn't the standard way workable in this case?

[Mark Lam]

(In reply to comment #5)
> (From update of attachment 185666 [details])
> The standard way to indicate that DatabaseContext must survive until ScriptExecutionContext is destroyed is to give ScriptExecutionContext a RefPtr to DatabaseContext. Why isn't the standard way workable in this case?

The same reason that DatabaseContext was made a Supplement previously i.e. to remove it from the ScriptExecutionContext when !ENABLE(SQL_DATABASE) without resorting to an #if.  With the current implementation, there remains no references to Database in ScriptExecutionContext.

Note: this pattern is used in DatabaseThread and FileThread as well.  I learned it from DatabaseThread.  That's not to say that it is a standard.

Do you object to using it here?

[Mark Lam]

(In reply to comment #6)
> Note: this pattern is used in DatabaseThread and FileThread as well.  I learned it from DatabaseThread.  That's not to say that it is a standard.
> 
> Do you object to using it here?

By "pattern", I meant the pattern of using a selfRef.

[Geoffrey Garen]

> to remove it from the ScriptExecutionContext when !ENABLE(SQL_DATABASE) without resorting to an #if.

What's so bad about #ifs?

I certainly prefer them to difficult-to-understand object lifetime management patterns.

[Mark Lam]

(In reply to comment #8)
> > to remove it from the ScriptExecutionContext when !ENABLE(SQL_DATABASE) without resorting to an #if.
> 
> What's so bad about #ifs?
> 
> I certainly prefer them to difficult-to-understand object lifetime management patterns.

OK. I'll re-do the patch to use a RefPtr in ScriptExecutionContext.

[Mark Lam]

Created attachment 185690 [details]
Revised fix.

[Geoffrey Garen]

Comment on attachment 185690 [details]
Revised fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=185690&action=review

r=me

> Source/WebCore/dom/ScriptExecutionContext.cpp:452
> +    m_databaseContext = databaseContext;

Would be nice to ASSERT(!m_databaseContext) here.

[Mark Lam]

(In reply to comment #11)
> > Source/WebCore/dom/ScriptExecutionContext.cpp:452
> > +    m_databaseContext = databaseContext;
> 
> Would be nice to ASSERT(!m_databaseContext) here.

Thanks for the review.  Assertion added.  Landed in r141431: <http://trac.webkit.org/changeset/141431>.