Summary: | [GTK] Threading problems with some of the tests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alejandro G. Castro <alex> | ||||||||||
Component: | Tools / Tests | Assignee: | 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
Alejandro G. Castro
2009-10-27 06:17:24 PDT
Another test with threading problems: storage/execute-sql-args.html Two more tests causing the same problem: storage/hash-change-with-xhr.html storage/database-lock-after-reload.html 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 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.
CCing the last person touching this code, so he can comment. 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) 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 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. Looks like bug 33294 can probably be duped to this one. If this is not easy to fix, I recommend we skip storage/ on the gtk bots so that the bots stay green. 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. (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. 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 on attachment 46128 [details]
Proposed patch to skip all the storage tests
Just skip "storage" instead of adding them all manually.
Unless GTK intentionally avoids skipping whole directories? style-queue ran check-webkit-style on attachment 46128 [details] without any errors.
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.
style-queue ran check-webkit-style on attachment 46129 [details] without any errors.
Comment on attachment 46129 [details]
Proposed patch to skip all the storage tests
r=me
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 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> All reviewed patches have been landed. Closing bug. Reopening until we find the problem with these tests. 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. 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. Created attachment 46257 [details]
Patch
Comment on attachment 46257 [details] Patch Clearing flags on attachment: 46257 Committed r53137: <http://trac.webkit.org/changeset/53137> All reviewed patches have been landed. Closing bug. I've unskipped these tests on the bots and haven't seen any trouble yet. |