22364 2008-11-19 13:01:11 -0800 Crashes seen on Tiger buildbots due to worker threads exhausting pthread keys 2008-11-20 10:13:53 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P1 Normal --- 1 ap ap oldest_to_newest 99368 0 ap 2008-11-19 13:01:11 -0800 Currently, each heap creates a pthread key that tracks threads that use this heap. This isn't necessary for workers, which are tied to a single thread. 99441 1 25307 ap 2008-11-20 02:16:18 -0800 Created attachment 25307 proposed fix 99486 2 25307 darin 2008-11-20 09:17:33 -0800 Comment on attachment 25307 proposed fix > #ifndef NDEBUG > + int error = > #endif > + pthread_key_delete(m_currentThreadRegistrar); > + ASSERT(!error); > + } The little ditties we do so we can assert the results of these functions are ugly enough that I think we should instead, in the future, make little helper inline functions to encapsulate them. They distract too much from the rest of the flow of the functions they are in. > +void Heap::makeUsableFromMultipleThreads() > +{ > + if (m_currentThreadRegistrar) > + return; > + > + int error = pthread_key_create(&m_currentThreadRegistrar, unregisterThread); > + if (error) > + CRASH(); > +} I wonder what we should do about cases like this. I think I understand what's going on -- this is an assertion, but one so important that you'd like it to be included in production code. But I'm not sure that would be clear to other readers. Also, it's not clear when to do abort() and when to do CRASH(). We should create some guidelines for this. > +#if ENABLE(JSC_MULTIPLE_THREADS) > + instance->makeUsableFromMultipleThreads(); > +#endif In general, when the semantics of a function like this is simple, I prefer to make empty inline versions so we don't need to make each call site conditional. That would have prevented us from needing conditionals in JSGlobalData.h and here in JSGlobalData.cpp and in JSContextRef.cpp. What do you think of that practice? r=me as is 99499 3 ap 2008-11-20 10:13:53 -0800 Committed revision 38622. (In reply to comment #2) > Also, it's not clear when to do abort() and when to do CRASH(). We > should create some guidelines for this. I don't like calling abort(), because it doesn't give any information for a bug report. Also, I think it's impolite to silently close an application. as if it was never running. > What do you think of that practice? In general, it's fine, but I didn't want code that's not guarded with ENABLE(JSC_MULTIPLE_THREADS) to call makeUsableFromMultipleThreads(), and I still don't want to. 25307 2008-11-20 02:16:18 -0800 2008-11-20 09:17:33 -0800 proposed fix NoThreadSpecificKeyForWorkers.txt text/plain 5471 ap SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDM4NjE2KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjYgQEAKKzIwMDgtMTEtMjAgIEFsZXhleSBQ cm9za3VyeWFrb3YgIDxhcEB3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisJCWh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0y MjM2NAorCQlDcmFzaGVzIHNlZW4gb24gVGlnZXIgYnVpbGRib3RzIGR1ZSB0byB3b3JrZXIgdGhy ZWFkcyBleGhhdXN0aW5nIHB0aHJlYWQga2V5cworCisgICAgICAgICogcnVudGltZS9Db2xsZWN0 b3IuY3BwOgorICAgICAgICAoSlNDOjpIZWFwOjpIZWFwKToKKyAgICAgICAgKEpTQzo6SGVhcDo6 ZGVzdHJveSk6CisgICAgICAgIChKU0M6OkhlYXA6Om1ha2VVc2FibGVGcm9tTXVsdGlwbGVUaHJl YWRzKToKKyAgICAgICAgKEpTQzo6SGVhcDo6cmVnaXN0ZXJUaHJlYWQpOgorICAgICAgICAqIHJ1 bnRpbWUvQ29sbGVjdG9yLmg6CisgICAgICAgIFB0aHJlYWQga2V5IGZvciB0cmFja2luZyB0aHJl YWRzIGlzIG9ubHkgY3JlYXRlZCBvbiByZXF1ZXN0IG5vdywgYmVjYXVzZSB0aGlzIGlzIGEgbGlt aXRlZAorICAgICAgICByZXNvdXJjZSwgYW5kIHRocmVhZCB0cmFja2luZyBpcyBub3QgbmVlZGVk IGZvciB3b3JrZXIgaGVhcHMsIG9yIGZvciBXZWJDb3JlIGhlYXAuCisKKyAgICAgICAgKiBBUEkv SlNDb250ZXh0UmVmLmNwcDogKEpTR2xvYmFsQ29udGV4dENyZWF0ZUluR3JvdXApOiBDYWxsIG1h a2VVc2FibGVGcm9tTXVsdGlwbGVUaHJlYWRzKCkuCisKKyAgICAgICAgKiBydW50aW1lL0pTR2xv YmFsRGF0YS5jcHA6IChKU0M6OkpTR2xvYmFsRGF0YTo6c2hhcmVkSW5zdGFuY2UpOiBEaXR0by4K KworICAgICAgICAqIHJ1bnRpbWUvSlNHbG9iYWxEYXRhLmg6IChKU0M6OkpTR2xvYmFsRGF0YTo6 bWFrZVVzYWJsZUZyb21NdWx0aXBsZVRocmVhZHMpOiBKdXN0IGZvcndhcmQKKyAgICAgICAgdGhl IGNhbGwgdG8gSGVhcCwgd2hpY2ggY2xpZW50cyBuZWVkIG5vdCBrbm93IGFib3V0LCBpZGVhbGx5 LgorCiAyMDA4LTExLTE5ICBHZW9mZnJleSBHYXJlbiAgPGdnYXJlbkBhcHBsZS5jb20+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCkluZGV4OiBKYXZhU2NyaXB0Q29yZS9BUEkv SlNDb250ZXh0UmVmLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0Q29yZS9BUEkvSlNDb250 ZXh0UmVmLmNwcAkocmV2aXNpb24gMzg2MTYpCisrKyBKYXZhU2NyaXB0Q29yZS9BUEkvSlNDb250 ZXh0UmVmLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjYsNiArNjYsMTAgQEAgSlNHbG9iYWxDb250 ZXh0UmVmIEpTR2xvYmFsQ29udGV4dENyZWF0ZQogCiAgICAgUmVmUHRyPEpTR2xvYmFsRGF0YT4g Z2xvYmFsRGF0YSA9IGdyb3VwID8gUGFzc1JlZlB0cjxKU0dsb2JhbERhdGE+KHRvSlMoZ3JvdXAp KSA6IEpTR2xvYmFsRGF0YTo6Y3JlYXRlKCk7CiAKKyNpZiBFTkFCTEUoSlNDX01VTFRJUExFX1RI UkVBRFMpCisgICAgZ2xvYmFsRGF0YS0+bWFrZVVzYWJsZUZyb21NdWx0aXBsZVRocmVhZHMoKTsK KyNlbmRpZgorCiAgICAgaWYgKCFnbG9iYWxPYmplY3RDbGFzcykgewogICAgICAgICBKU0dsb2Jh bE9iamVjdCogZ2xvYmFsT2JqZWN0ID0gbmV3IChnbG9iYWxEYXRhLmdldCgpKSBKU0dsb2JhbE9i amVjdDsKICAgICAgICAgcmV0dXJuIEpTR2xvYmFsQ29udGV4dFJldGFpbih0b0dsb2JhbFJlZihn bG9iYWxPYmplY3QtPmdsb2JhbEV4ZWMoKSkpOwpJbmRleDogSmF2YVNjcmlwdENvcmUvcnVudGlt ZS9Db2xsZWN0b3IuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3JpcHRDb3JlL3J1bnRpbWUvQ29s bGVjdG9yLmNwcAkocmV2aXNpb24gMzg2MTYpCisrKyBKYXZhU2NyaXB0Q29yZS9ydW50aW1lL0Nv bGxlY3Rvci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEyMSwxNyArMTIxLDEyIEBAIEhlYXA6Okhl YXAoSlNHbG9iYWxEYXRhKiBnbG9iYWxEYXRhKQogICAgIDogbV9tYXJrTGlzdFNldCgwKQogI2lm IEVOQUJMRShKU0NfTVVMVElQTEVfVEhSRUFEUykKICAgICAsIG1fcmVnaXN0ZXJlZFRocmVhZHMo MCkKKyAgICAsIG1fY3VycmVudFRocmVhZFJlZ2lzdHJhcigwKQogI2VuZGlmCiAgICAgLCBtX2ds b2JhbERhdGEoZ2xvYmFsRGF0YSkKIHsKICAgICBBU1NFUlQoZ2xvYmFsRGF0YSk7CiAKLSNpZiBF TkFCTEUoSlNDX01VTFRJUExFX1RIUkVBRFMpCi0gICAgaW50IGVycm9yID0gcHRocmVhZF9rZXlf Y3JlYXRlKCZtX2N1cnJlbnRUaHJlYWRSZWdpc3RyYXIsIHVucmVnaXN0ZXJUaHJlYWQpOwotICAg IGlmIChlcnJvcikKLSAgICAgICAgQ1JBU0goKTsKLSNlbmRpZgotCiAgICAgbWVtc2V0KCZwcmlt YXJ5SGVhcCwgMCwgc2l6ZW9mKENvbGxlY3RvckhlYXApKTsKICAgICBtZW1zZXQoJm51bWJlckhl YXAsIDAsIHNpemVvZihDb2xsZWN0b3JIZWFwKSk7CiB9CkBAIC0xNjUsMTEgKzE2MCwxMyBAQCB2 b2lkIEhlYXA6OmRlc3Ryb3koKQogICAgIGZyZWVIZWFwKCZudW1iZXJIZWFwKTsKIAogI2lmIEVO QUJMRShKU0NfTVVMVElQTEVfVEhSRUFEUykKKyAgICBpZiAobV9jdXJyZW50VGhyZWFkUmVnaXN0 cmFyKSB7CiAjaWZuZGVmIE5ERUJVRwotICAgIGludCBlcnJvciA9CisgICAgICAgIGludCBlcnJv ciA9CiAjZW5kaWYKLSAgICAgICAgcHRocmVhZF9rZXlfZGVsZXRlKG1fY3VycmVudFRocmVhZFJl Z2lzdHJhcik7Ci0gICAgQVNTRVJUKCFlcnJvcik7CisgICAgICAgICAgICBwdGhyZWFkX2tleV9k ZWxldGUobV9jdXJyZW50VGhyZWFkUmVnaXN0cmFyKTsKKyAgICAgICAgQVNTRVJUKCFlcnJvcik7 CisgICAgfQogCiAgICAgTXV0ZXhMb2NrZXIgcmVnaXN0ZXJlZFRocmVhZHNMb2NrKG1fcmVnaXN0 ZXJlZFRocmVhZHNNdXRleCk7CiAgICAgZm9yIChIZWFwOjpUaHJlYWQqIHQgPSBtX3JlZ2lzdGVy ZWRUaHJlYWRzOyB0OykgewpAQCAtNDc4LDkgKzQ3NSwxOSBAQCBzdGF0aWMgaW5saW5lIFBsYXRm b3JtVGhyZWFkIGdldEN1cnJlbnRQCiAjZW5kaWYKIH0KIAordm9pZCBIZWFwOjptYWtlVXNhYmxl RnJvbU11bHRpcGxlVGhyZWFkcygpCit7CisgICAgaWYgKG1fY3VycmVudFRocmVhZFJlZ2lzdHJh cikKKyAgICAgICAgcmV0dXJuOworCisgICAgaW50IGVycm9yID0gcHRocmVhZF9rZXlfY3JlYXRl KCZtX2N1cnJlbnRUaHJlYWRSZWdpc3RyYXIsIHVucmVnaXN0ZXJUaHJlYWQpOworICAgIGlmIChl cnJvcikKKyAgICAgICAgQ1JBU0goKTsKK30KKwogdm9pZCBIZWFwOjpyZWdpc3RlclRocmVhZCgp CiB7Ci0gICAgaWYgKHB0aHJlYWRfZ2V0c3BlY2lmaWMobV9jdXJyZW50VGhyZWFkUmVnaXN0cmFy KSkKKyAgICBpZiAoIW1fY3VycmVudFRocmVhZFJlZ2lzdHJhciB8fCBwdGhyZWFkX2dldHNwZWNp ZmljKG1fY3VycmVudFRocmVhZFJlZ2lzdHJhcikpCiAgICAgICAgIHJldHVybjsKIAogICAgIHB0 aHJlYWRfc2V0c3BlY2lmaWMobV9jdXJyZW50VGhyZWFkUmVnaXN0cmFyLCB0aGlzKTsKSW5kZXg6 IEphdmFTY3JpcHRDb3JlL3J1bnRpbWUvQ29sbGVjdG9yLmgKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2YVNj cmlwdENvcmUvcnVudGltZS9Db2xsZWN0b3IuaAkocmV2aXNpb24gMzg2MTYpCisrKyBKYXZhU2Ny aXB0Q29yZS9ydW50aW1lL0NvbGxlY3Rvci5oCSh3b3JraW5nIGNvcHkpCkBAIC0xNDUsNiArMTQ1 LDggQEAgbmFtZXNwYWNlIEpTQyB7CiAgICAgICAgIEhhc2hTZXQ8QXJnTGlzdCo+KiBtX21hcmtM aXN0U2V0OwogCiAjaWYgRU5BQkxFKEpTQ19NVUxUSVBMRV9USFJFQURTKQorICAgICAgICB2b2lk IG1ha2VVc2FibGVGcm9tTXVsdGlwbGVUaHJlYWRzKCk7CisKICAgICAgICAgc3RhdGljIHZvaWQg dW5yZWdpc3RlclRocmVhZCh2b2lkKik7CiAgICAgICAgIHZvaWQgdW5yZWdpc3RlclRocmVhZCgp OwogCkluZGV4OiBKYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTR2xvYmFsRGF0YS5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0dsb2JhbERhdGEuY3BwCShyZXZpc2lv biAzODYxNikKKysrIEphdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNHbG9iYWxEYXRhLmNwcAkod29y a2luZyBjb3B5KQpAQCAtMTY1LDggKzE2NSwxMiBAQCBib29sIEpTR2xvYmFsRGF0YTo6c2hhcmVk SW5zdGFuY2VFeGlzdHMoCiBKU0dsb2JhbERhdGEmIEpTR2xvYmFsRGF0YTo6c2hhcmVkSW5zdGFu Y2UoKQogewogICAgIEpTR2xvYmFsRGF0YSomIGluc3RhbmNlID0gc2hhcmVkSW5zdGFuY2VJbnRl cm5hbCgpOwotICAgIGlmICghaW5zdGFuY2UpCisgICAgaWYgKCFpbnN0YW5jZSkgewogICAgICAg ICBpbnN0YW5jZSA9IG5ldyBKU0dsb2JhbERhdGEodHJ1ZSk7CisjaWYgRU5BQkxFKEpTQ19NVUxU SVBMRV9USFJFQURTKQorICAgICAgICBpbnN0YW5jZS0+bWFrZVVzYWJsZUZyb21NdWx0aXBsZVRo cmVhZHMoKTsKKyNlbmRpZgorICAgIH0KICAgICByZXR1cm4gKmluc3RhbmNlOwogfQogCkluZGV4 OiBKYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTR2xvYmFsRGF0YS5oCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNHbG9iYWxEYXRhLmgJKHJldmlzaW9uIDM4NjE2KQorKysg SmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0dsb2JhbERhdGEuaAkod29ya2luZyBjb3B5KQpAQCAt NjMsNiArNjMsMTEgQEAgbmFtZXNwYWNlIEpTQyB7CiAgICAgICAgIHN0YXRpYyBQYXNzUmVmUHRy PEpTR2xvYmFsRGF0YT4gY3JlYXRlTGVha2VkKCk7CiAgICAgICAgIH5KU0dsb2JhbERhdGEoKTsK IAorI2lmIEVOQUJMRShKU0NfTVVMVElQTEVfVEhSRUFEUykKKyAgICAgICAgLy8gV2lsbCBzdGFy dCB0cmFja2luZyB0aHJlYWRzIHRoYXQgdXNlIHRoZSBoZWFwLCB3aGljaCBpcyByZXNvdXJjZS1o ZWF2eS4KKyAgICAgICAgdm9pZCBtYWtlVXNhYmxlRnJvbU11bHRpcGxlVGhyZWFkcygpIHsgaGVh cC5tYWtlVXNhYmxlRnJvbU11bHRpcGxlVGhyZWFkcygpOyB9CisjZW5kaWYKKwogICAgICAgICBJ bnRlcnByZXRlciogaW50ZXJwcmV0ZXI7CiAKICAgICAgICAgSlNWYWx1ZSogZXhjZXB0aW9uOwo=