Bug 30814

Summary: [GTK] Threading problems with some of the tests
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, levin, mrobinson, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 33294    
Bug Blocks:    
Attachments:
Description Flags
crossthreadstring.diff
xan.lopez: review-, xan.lopez: commit-queue-
Proposed patch to skip all the storage tests
eric: review-
Proposed patch to skip all the storage tests
none
Patch none

Description Alejandro G. Castro 2009-10-27 06:17:24 PDT
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)
Comment 1 Alejandro G. Castro 2009-10-27 09:18:41 PDT
Another test with threading problems:

storage/execute-sql-args.html
Comment 2 Alejandro G. Castro 2009-10-27 10:10:18 PDT
Two more tests causing the same problem:

storage/hash-change-with-xhr.html
storage/database-lock-after-reload.html
Comment 3 Alejandro G. Castro 2009-10-27 10:19:42 PDT
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
Comment 4 Xan Lopez 2009-10-29 10:29:10 PDT
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.
Comment 5 Xan Lopez 2009-10-29 10:29:57 PDT
CCing the last person touching this code, so he can comment.
Comment 6 Xan Lopez 2009-10-29 10:59:58 PDT
This was landed in r50285, closing.
Comment 7 Alejandro G. Castro 2009-11-25 09:52:13 PST
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)
Comment 9 Eric Seidel (no email) 2009-12-09 14:04:04 PST
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.
Comment 10 Eric Seidel (no email) 2010-01-07 12:32:09 PST
Looks like bug 33294 can probably be duped to this one.
Comment 11 Eric Seidel (no email) 2010-01-07 12:32:37 PST
If this is not easy to fix, I recommend we skip storage/ on the gtk bots so that the bots stay green.
Comment 12 Eric Seidel (no email) 2010-01-08 03:13:20 PST
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.
Comment 13 Alejandro G. Castro 2010-01-08 03:27:36 PST
(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.
Comment 14 Alejandro G. Castro 2010-01-08 03:37:48 PST
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.
Comment 15 Eric Seidel (no email) 2010-01-08 03:38:53 PST
Comment on attachment 46128 [details]
Proposed patch to skip all the storage tests

Just skip "storage" instead of adding them all manually.
Comment 16 Eric Seidel (no email) 2010-01-08 03:39:17 PST
Unless GTK intentionally avoids skipping whole directories?
Comment 17 WebKit Review Bot 2010-01-08 03:39:18 PST
style-queue ran check-webkit-style on attachment 46128 [details] without any errors.
Comment 18 Alejandro G. Castro 2010-01-08 03:41:39 PST
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.
Comment 19 WebKit Review Bot 2010-01-08 03:45:25 PST
style-queue ran check-webkit-style on attachment 46129 [details] without any errors.
Comment 20 Xan Lopez 2010-01-08 03:46:03 PST
Comment on attachment 46129 [details]
Proposed patch to skip all the storage tests

r=me
Comment 21 Eric Seidel (no email) 2010-01-08 03:47:06 PST
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.
Comment 22 WebKit Commit Bot 2010-01-08 04:46:58 PST
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>
Comment 23 WebKit Commit Bot 2010-01-08 04:47:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Alejandro G. Castro 2010-01-08 04:53:36 PST
Reopening until we find the problem with these tests.
Comment 25 Eric Seidel (no email) 2010-01-09 15:34:12 PST
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.
Comment 26 Eric Seidel (no email) 2010-01-10 14:42:27 PST
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.
Comment 27 Eric Seidel (no email) 2010-01-10 21:18:30 PST
Created attachment 46257 [details]
Patch
Comment 28 WebKit Commit Bot 2010-01-12 05:06:03 PST
Comment on attachment 46257 [details]
Patch

Clearing flags on attachment: 46257

Committed r53137: <http://trac.webkit.org/changeset/53137>
Comment 29 WebKit Commit Bot 2010-01-12 05:06:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Martin Robinson 2011-03-11 13:29:35 PST
I've unskipped these tests on the bots and haven't seen any trouble yet.