Bug 30814 – [GTK] Threading problems with some of the tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alejandro G. Castro]

These tests crash in one thread assertion randomly:

storage/close-during-stress-test.html -> crashed
storage/domstorage/documentURI.html -> crashed
storage/test-authorizer-stderr.txt -> crashed

The assertion is:

ASSERTION FAILED: currentThread() == m_openingThread
(../../WebCore/platform/sql/SQLiteDatabase.h:100 sqlite3* WebCore::SQLiteDatabase::sqlite3Handle() const)

[Alejandro G. Castro]

Another test with threading problems:

storage/execute-sql-args.html

[Alejandro G. Castro]

Two more tests causing the same problem:

storage/hash-change-with-xhr.html
storage/database-lock-after-reload.html

[Alejandro G. Castro]

This is another one failing in the last round, not sure if we should just skip all the storage test cases until we find the problem:

storage/domstorage/complex-keys.html

[Xan Lopez]

Created attachment 42109 [details]
crossthreadstring.diff

This patch seems to fix the crashers I can reproduce locally. I don't claim to fully understand what's going on, but from reading the code it seems the m_name string in Database is created with crossThreading String, the others are not used differently than this one is, and so it would seem natural to expect that all of them have to be created like that. More evidence for this comes from the fact that the trace for the crash is like:

Thread 3 (Thread 0xb6defb90 (LWP 19474)):
#0  0x00cabea9 in WTF::CrossThreadRefCounted<WTF::OwnFastMallocPtr<unsigned short> >::deref (this=0x840c390) at ../../JavaScriptCore/wtf/CrossThreadRefCounted.h:122
#1  0x0117b4b2 in ~StringImpl (this=0x83d3cf0) at ../../WebCore/platform/text/StringImpl.cpp:108
#2  0x00b10ad5 in WTF::RefCounted<WebCore::StringImpl>::deref (this=0x83d3cf0) at ../../JavaScriptCore/wtf/RefCounted.h:109
#3  0x00b107eb in ~RefPtr (this=0x840bbac) at ../../JavaScriptCore/wtf/RefPtr.h:53
#4  0x00b10129 in ~String (this=0x840bbac) at ../../WebCore/platform/text/PlatformString.h:69
#5  0x012d4825 in ~Database (this=0x840bb10) at ../../WebCore/storage/Database.cpp:200
#6  0x010055a1 in WTF::ThreadSafeShared<WebCore::Database>::deref (this=0x840bb10) at ../../JavaScriptCore/wtf/Threading.h:313
#7  0x01001153 in ~RefPtr (this=0x8408aec) at ../../JavaScriptCore/wtf/RefPtr.h:53
#8  0x012e0fa6 in WTF::HashTable<WTF::RefPtr<WebCore::Database>, WTF::RefPtr<WebCore::Database>, WTF::IdentityExtractor<WTF::RefPtr<WebCore::Database> >, WTF::PtrHash<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> >, WTF::HashTraits<WTF::RefPtr<WebCore::Database> > >::deallocateTable (table=0x8408a50, size=64) at ../../JavaScriptCore/wtf/HashTable.h:872
#9  0x012e0392 in ~HashTable (this=0xb6def24c) at ../../JavaScriptCore/wtf/HashTable.h:296
#10 0x012e0105 in ~HashSet (this=0xb6def24c) at ../../JavaScriptCore/wtf/HashSet.h:38
#11 0x012dfc16 in WebCore::DatabaseThread::databaseThread (this=0x83d3e48) at ../../WebCore/storage/DatabaseThread.cpp:117
#12 0x012dfa2b in WebCore::DatabaseThread::databaseThreadStart (vDatabaseThread=0x83d3e48) at ../../WebCore/storage/DatabaseThread.cpp:82
#13 0x00c0540e in threadEntryPoint (contextData=0x83d3f10) at ../../JavaScriptCore/wtf/Threading.cpp:64
#14 0x0024651f in start_thread (arg=0xb6defb90) at pthread_create.c:297
#15 0x0309b04e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130

And upon inspection the string being destroyed is m_displayName.

[Xan Lopez]

CCing the last person touching this code, so he can comment.

[Xan Lopez]

This was landed in r50285, closing.

[Alejandro G. Castro]

These problems are not still completely gone, we have to add this test also to the list in Skipped:

http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r51387%20(1286)/fast/js/excessive-comma-usage-stderr.txt

ASSERTION FAILED: currentThread() == m_openingThread
(../../WebCore/platform/sql/SQLiteDatabase.h:100 sqlite3* WebCore::SQLiteDatabase::sqlite3Handle() const)

[Alejandro G. Castro]

And another one with the same problem:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r51389%20(1302)/storage/transaction-error-callback-stderr.txt

[Eric Seidel (no email)]

Should this bug still be open?  Wasn't this patch landed?  If the patch was landed but the bug should still be open please obsolete the patch and clear the r+ flag.

[Eric Seidel (no email)]

Looks like bug 33294 can probably be duped to this one.

[Eric Seidel (no email)]

If this is not easy to fix, I recommend we skip storage/ on the gtk bots so that the bots stay green.

[Eric Seidel (no email)]

This would appear to be the worst reliability bug affecting the Gtk bots.  Unless someone knows what's going wrong here, I would recommend we skip storage/ for now.

[Alejandro G. Castro]

(In reply to comment #12)
> This would appear to be the worst reliability bug affecting the Gtk bots. 
> Unless someone knows what's going wrong here, I would recommend we skip
> storage/ for now.

I have talked to Xan and we agree about skipping them until we find a solution to this threading problem. I'm going to upload the patch.

[Alejandro G. Castro]

Created attachment 46128 [details]
Proposed patch to skip all the storage tests

Following Eric's recommendation this patch skips all the storage tests, we should leave the bug open after committing this patch until we find a proper solution.

[Eric Seidel (no email)]

Comment on attachment 46128 [details]
Proposed patch to skip all the storage tests

Just skip "storage" instead of adding them all manually.

[Eric Seidel (no email)]

Unless GTK intentionally avoids skipping whole directories?

[WebKit Review Bot]

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

[Alejandro G. Castro]

Created attachment 46129 [details]
Proposed patch to skip all the storage tests

I tested with just storage and it works. Thanks for the comment Eric.

[WebKit Review Bot]

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

[Xan Lopez]

Comment on attachment 46129 [details]
Proposed patch to skip all the storage tests

r=me

[Eric Seidel (no email)]

Comment on attachment 46129 [details]
Proposed patch to skip all the storage tests

Looks fine to me.  If you want to keep this bug open, you'll either have to land this manually, or re-open it after using the commit-queue.  The commit-queue will close the bug after landing because no other patches are waiting review on this patch.

[WebKit Commit Bot]

Comment on attachment 46129 [details]
Proposed patch to skip all the storage tests

Clearing flags on attachment: 46129

Committed r52988: <http://trac.webkit.org/changeset/52988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alejandro G. Castro]

Reopening until we find the problem with these tests.

[Eric Seidel (no email)]

This bug is also causing fast/js/excessive-comma-usage.html to fail:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53016%20(2228)/fast/js/excessive-comma-usage-stderr.txt
ASSERTION FAILED: currentThread() == m_openingThread
(../../WebCore/platform/sql/SQLiteDatabase.h:100 sqlite3* WebCore::SQLiteDatabase::sqlite3Handle() const)

Seems we should skip that test too.

[Eric Seidel (no email)]

Hit this again:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53050%20(2248)/fast/js/excessive-comma-usage-stderr.txt

I'll post a patch to skip that test too tomorrow unless someone beats me to it.

[Eric Seidel (no email)]

Created attachment 46257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 46257 [details]
Patch

Clearing flags on attachment: 46257

Committed r53137: <http://trac.webkit.org/changeset/53137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

I've unskipped these tests on the bots and haven't seen any trouble yet.