25138 2009-04-10 16:32:51 -0700 Need to change WTF::isMainThread() for pthreads to work correctly for thread-specific destructors 2009-04-23 14:14:36 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Mac OS X 10.5 RESOLVED DUPLICATE 25348 P2 Normal --- 1 dimich dimich ap aroben oldest_to_newest 117200 0 dimich 2009-04-10 16:32:51 -0700 Pthreads run destructors of thread-specific data after thread exits from its thread proc. On non-DARWIN or CHROMIUM flavors of pthreads-based ports, this causes the ThreadIdentifier to be removed from the WTF ThreadMap before those destructors are run. Because of this, the check in isMainThread(): return currentThread() == mainThreadIdentifier; will insert the new ThreadIdentifier for the exiting thread which will not be removed from the map and cause asserts later in establishIdentifierForPthreadHandle when a new thread with the same thread ID will be created (pthreads reuse their ids). Fixed with replacing the check with return pthread_equal(pthread_self(), mainThreadPthreadID); Since other platform use other methods of dealing with TLS, this is pthread-only fix. 117201 1 29409 dimich 2009-04-10 16:35:57 -0700 Created attachment 29409 Proposed patch 117202 2 29409 darin 2009-04-10 16:51:51 -0700 Comment on attachment 29409 Proposed patch Seems OK to fix isMainThread this way assuming we can live with not being able to fix currentThread(). But what about use of the currentThread() function in thread-specific destructors? Is there a way we can fix that? 117216 3 dimich 2009-04-10 17:30:03 -0700 (In reply to comment #2) > (From update of attachment 29409 [review]) > Seems OK to fix isMainThread this way assuming we can live with not being able > to fix currentThread(). > > But what about use of the currentThread() function in thread-specific > destructors? Is there a way we can fix that? > I'm not sure how to fix currentThread(), especially considering the order of thread-specific destructors is unspecified and pthread_t ids can be reused right after the previous thread using it is finally terminated. Perhaps it's better to say the ThreadIdentifier is undefined outside of the create/detach lifespan. Since we don't use currentThread() now in the thread-specific destructors, I think it would be useful to have a currentThread() return -1 and ASSERT if called outside of create/detach thread lifetime or if it is not the thread that's called initializeThreading(). This way, the threads from embedder created outside of WTF would not get assigned WTF ThreadIdentifier, nor create a problem with cleaning those out of the ThreadMap (since they wouldn't use WTF::detachThread). Also, it would actively discourage using currentThread() in thread-specific destructors in the future. If there will be a need to use WTF threading functions on threads created outside of WTF, we can always add something like WTF::initializeThreadingForCurrentThread with requirement to call detachThread later to reclaim WTF. Do you find this a reasonable direction? 117318 4 29409 dimich 2009-04-13 11:21:34 -0700 Comment on attachment 29409 Proposed patch removing the r? since this needs more work. 118539 5 dimich 2009-04-23 14:14:36 -0700 Resolving this one as a duplicate of 25348, since that one solves the same issue in a more complete way. *** This bug has been marked as a duplicate of 25348 *** 29409 2009-04-10 16:35:57 -0700 2009-04-13 11:21:34 -0700 Proposed patch patch.txt text/plain 1978 dimich ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBlNzYyY2M4Li4xMDYyMmFkIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMDktMDQtMTAgIERtaXRyeSBUaXRvdiAgPGRpbWljaEBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI1MTM4CisgICAgICAgIENoYW5nZSBXVEY6Omlz TWFpblRocmVhZCgpIGZvciBwdGhyZWFkcyB0byB3b3JrIGNvcnJlY3RseSBmb3IgdGhyZWFkLXNw ZWNpZmljIGRlc3RydWN0b3JzLgorCisgICAgICAgICogd3RmL1RocmVhZGluZ1B0aHJlYWRzLmNw cDoKKyAgICAgICAgKFdURjo6aW5pdGlhbGl6ZVRocmVhZGluZyk6IGNhcHR1cmUgcHRocmVhZF90 IGluc3RlYWQgb2YgVGhyZWFkSWRlbnRpZmllci4KKyAgICAgICAgKFdURjo6aXNNYWluVGhyZWFk KTogY29tcGFyZSBwdGhyZWFkX2lkIGluc3RlYWQgb2YgVGhyZWFkSWRlbnRpZmllci4KKwogMjAw OS0wNC0wOSAgQmVuIE11cmRvY2ggIDxiZW5tQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgQWxleGV5IFByb3NrdXJ5YWtvdi4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0 Zi9UaHJlYWRpbmdQdGhyZWFkcy5jcHAgYi9KYXZhU2NyaXB0Q29yZS93dGYvVGhyZWFkaW5nUHRo cmVhZHMuY3BwCmluZGV4IDFiYjMwMzMuLjZjYzZjMWIgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRD b3JlL3d0Zi9UaHJlYWRpbmdQdGhyZWFkcy5jcHAKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL1Ro cmVhZGluZ1B0aHJlYWRzLmNwcApAQCAtNTMsNyArNTMsNyBAQCB0eXBlZGVmIEhhc2hNYXA8VGhy ZWFkSWRlbnRpZmllciwgcHRocmVhZF90PiBUaHJlYWRNYXA7CiBzdGF0aWMgTXV0ZXgqIGF0b21p Y2FsbHlJbml0aWFsaXplZFN0YXRpY011dGV4OwogCiAjaWYgIVBMQVRGT1JNKERBUldJTikgfHwg UExBVEZPUk0oQ0hST01JVU0pCi1zdGF0aWMgVGhyZWFkSWRlbnRpZmllciBtYWluVGhyZWFkSWRl bnRpZmllcjsgLy8gVGhlIHRocmVhZCB0aGF0IHdhcyB0aGUgZmlyc3QgdG8gY2FsbCBpbml0aWFs aXplVGhyZWFkaW5nKCksIHdoaWNoIG11c3QgYmUgdGhlIG1haW4gdGhyZWFkLgorc3RhdGljIHB0 aHJlYWRfdCBtYWluVGhyZWFkUHRocmVhZElkOyAvLyBUaGUgdGhyZWFkIHRoYXQgd2FzIHRoZSBm aXJzdCB0byBjYWxsIGluaXRpYWxpemVUaHJlYWRpbmcoKSwgd2hpY2ggbXVzdCBiZSB0aGUgbWFp biB0aHJlYWQuCiAjZW5kaWYKIAogc3RhdGljIE11dGV4JiB0aHJlYWRNYXBNdXRleCgpCkBAIC02 OSw3ICs2OSw3IEBAIHZvaWQgaW5pdGlhbGl6ZVRocmVhZGluZygpCiAgICAgICAgIHRocmVhZE1h cE11dGV4KCk7CiAgICAgICAgIGluaXRpYWxpemVSYW5kb21OdW1iZXJHZW5lcmF0b3IoKTsKICNp ZiAhUExBVEZPUk0oREFSV0lOKSB8fCBQTEFURk9STShDSFJPTUlVTSkKLSAgICAgICAgbWFpblRo cmVhZElkZW50aWZpZXIgPSBjdXJyZW50VGhyZWFkKCk7CisgICAgICAgIG1haW5UaHJlYWRQdGhy ZWFkSWQgPSBwdGhyZWFkX3NlbGYoKTsKICNlbmRpZgogICAgICAgICBpbml0aWFsaXplTWFpblRo cmVhZCgpOwogICAgIH0KQEAgLTIyOCw3ICsyMjgsNyBAQCBib29sIGlzTWFpblRocmVhZCgpCiAj aWYgUExBVEZPUk0oREFSV0lOKSAmJiAhUExBVEZPUk0oQ0hST01JVU0pCiAgICAgcmV0dXJuIHB0 aHJlYWRfbWFpbl9ucCgpOwogI2Vsc2UKLSAgICByZXR1cm4gY3VycmVudFRocmVhZCgpID09IG1h aW5UaHJlYWRJZGVudGlmaWVyOworICAgIHJldHVybiBwdGhyZWFkX2VxdWFsKHB0aHJlYWRfc2Vs ZigpLCBtYWluVGhyZWFkUHRocmVhZElkKTsKICNlbmRpZgogfQogCg==