67682 2011-09-06 15:59:19 -0700 Accessibility tests crashing in BasicRawSentinelNode code 2011-09-06 19:04:14 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P1 Normal --- 1 ap webkit-unassigned barraclough fpizlo ggaren oliver webkit.review.bot oldest_to_newest 462961 0 ap 2011-09-06 15:59:19 -0700 When I run regression tests with GuardMalloc, there are semi-reproducible crashes like the below. $ run-webkit-tests -g accessibility/ --repeat 3 This doesn't seem to be a failure from any single test - when I run them each 20 times, I sometimes don't get any crash at all. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000107351d74 WTF::BasicRawSentinelNode<JSC::CallLinkInfo>::setNext(WTF::BasicRawSentinelNode<JSC::CallLinkInfo>*) + 20 (SentinelLinkedList.h:60) 1 com.apple.JavaScriptCore 0x000000010735351f WTF::SentinelLinkedList<JSC::CallLinkInfo, WTF::BasicRawSentinelNode<JSC::CallLinkInfo> >::remove(JSC::CallLinkInfo*) + 319 (SentinelLinkedList.h:147) 2 com.apple.JavaScriptCore 0x000000010734d535 WTF::BasicRawSentinelNode<JSC::CallLinkInfo>::remove() + 21 (SentinelLinkedList.h:98) 3 com.apple.JavaScriptCore 0x000000010734ef98 JSC::CallLinkInfo::~CallLinkInfo() + 56 (CodeBlock.h:114) 4 com.apple.JavaScriptCore 0x000000010734ef55 JSC::CallLinkInfo::~CallLinkInfo() + 21 (CodeBlock.h:114) ... 462969 1 fpizlo 2011-09-06 16:12:12 -0700 Yup, that's my fault. CodeBlock is assuming that it won't be destroyed until after any CodeBlocks that have calls into it are destroyed, which is a demonstrably wrong assumption. This bug will manifest itself if you have two CodeBlocks, A and B, where A contains a call to code owned by B. Then the GC decides to first destroy B and then destroy A. This can happen if both A and B became unreachable at the same time, if the heap is being destroyed, or if we call recompileAllJSFunctions(). At this stage A has a node on a linked list owned by B. Thus A likely has multiple pointers into memory that will be destroyed when B's destructor runs. Then the following dangerous and unpredictable course of events unfolds: 1) B gets destroyed, but does not remove A's node from its linked list. 2) A gets destroyed, and decides to be a good citizen and remove its node from B's linked list. Linked list removal requires accessing B's linked list, which no longer exists. Some manner of memory corruption ensues. This bug occurs because of a misguided optimization on my part. CodeBlock::~CodeBlock should ensure that any incoming calls (CallLinkInfos) that are on this's linked list are removed form the linked list and are marked as having been removed, so that when those CodeBlock's destructors run, they don't try to access garbage memory. For some reason I had convinced myself that this was unnecessary and removed it. The fix is to bring this back. I'm rolling a patch for this now. 462971 2 106511 fpizlo 2011-09-06 16:18:07 -0700 Created attachment 106511 proposed patch Still testing this but it's ready for review. 462985 3 106515 fpizlo 2011-09-06 16:32:06 -0700 Created attachment 106515 the patch - fixed comment to more accurately reflect the change 462988 4 106515 fpizlo 2011-09-06 16:39:44 -0700 Comment on attachment 106515 the patch - fixed comment to more accurately reflect the change This appears to fix the problem. I cannot get crashes anymore, and nothing else seems to have regressed. 463000 5 106515 ggaren 2011-09-06 16:58:00 -0700 Comment on attachment 106515 the patch - fixed comment to more accurately reflect the change r=me 463070 6 106515 webkit.review.bot 2011-09-06 19:04:09 -0700 Comment on attachment 106515 the patch - fixed comment to more accurately reflect the change Clearing flags on attachment: 106515 Committed r94623: <http://trac.webkit.org/changeset/94623> 463071 7 webkit.review.bot 2011-09-06 19:04:14 -0700 All reviewed patches have been landed. Closing bug. 106511 2011-09-06 16:18:07 -0700 2011-09-06 16:32:06 -0700 proposed patch fixlinkedlist_patch_1.diff text/plain 1599 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTQ2MTMpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAK KzIwMTEtMDktMDYgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBB Y2Nlc3NpYmlsaXR5IHRlc3RzIGNyYXNoaW5nIGluIEJhc2ljUmF3U2VudGluZWxOb2RlIGNvZGUK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3NjgyCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisgICAgICAgIAorICAgICAgICBB IENvZGVCbG9jayBzaG91bGQgZW5zdXJlIHRoYXQgbm8gb3RoZXIgQ29kZUJsb2NrcyBoYXZlIHJl ZmVyZW5jZXMgdG8gaXQgYWZ0ZXIKKyAgICAgICAgaXQgaXMgZGVzdHJveWVkLgorCisgICAgICAg ICogYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6Q29kZUJsb2NrOjp+Q29k ZUJsb2NrKToKKwogMjAxMS0wOS0wNiAgTWFyayBIYWhuZW5iZXJnICA8bWhhaG5lbmJlcmdAYXBw bGUuY29tPgogCiAgICAgICAgIEZpeCBicm9rZW4gUFBDIGJ1aWxkIGR1ZSB0byBuZXcgZHRvYSBs aWJyYXJ5CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2NrLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2Nr LmNwcAkocmV2aXNpb24gOTQ2MTIpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUv Q29kZUJsb2NrLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTQ0OSw4ICsxNDQ5LDkgQEAgQ29kZUJs b2NrOjp+Q29kZUJsb2NrKCkKICNlbmRpZgogICAgIAogICAgIC8vIFdlIHNob3VsZCBub3QgYmUg Z2FyYmFnZSBjb2xsZWN0ZWQgaWYgdGhlcmUgYXJlIGluY29taW5nIGNhbGxzLiBCdXQKLSAgICAv LyBpZiB0aGlzIGlzIGNhbGxlZCBkdXJpbmcgaGVhcCBkZXN0cnVjdGlvbiwgdGhlbiB0aGVyZSBt YXkgc3RpbGwgYmUKLSAgICAvLyBpbmNvbWluZyBjYWxscywgd2hpY2ggaXMgaGFybWxlc3MuCisg ICAgLy8gaWYgdGhpcyBpcyBjYWxsZWQgZHVyaW5nIGhlYXAgZGVzdHJ1Y3Rpb24gb3Igd2hlbiB3 ZSBkZXN0cm95IGFsbCBjb2RlLgorICAgIHdoaWxlIChtX2luY29taW5nQ2FsbHMuYmVnaW4oKSAh PSBtX2luY29taW5nQ2FsbHMuZW5kKCkpCisgICAgICAgIG1faW5jb21pbmdDYWxscy5iZWdpbigp LT5yZW1vdmUoKTsKICAgICAKICAgICAvLyBOb3RlIHRoYXQgb3VyIG91dGdvaW5nIGNhbGxzIHdp bGwgYmUgcmVtb3ZlZCBmcm9tIG90aGVyIENvZGVCbG9ja3MnCiAgICAgLy8gbV9pbmNvbWluZ0Nh bGxzIGxpbmtlZCBsaXN0cyB0aHJvdWdoIHRoZSBleGVjdXRpb24gb2YgdGhlIH5DYWxsTGlua0lu Zm8K 106515 2011-09-06 16:32:06 -0700 2011-09-06 19:04:09 -0700 the patch - fixed comment to more accurately reflect the change fixlinkedlist_patch_2.diff text/plain 2006 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTQ2MTMpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAK KzIwMTEtMDktMDYgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBB Y2Nlc3NpYmlsaXR5IHRlc3RzIGNyYXNoaW5nIGluIEJhc2ljUmF3U2VudGluZWxOb2RlIGNvZGUK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3NjgyCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisgICAgICAgIAorICAgICAgICBB IENvZGVCbG9jayBzaG91bGQgZW5zdXJlIHRoYXQgbm8gb3RoZXIgQ29kZUJsb2NrcyBoYXZlIHJl ZmVyZW5jZXMgdG8gaXQgYWZ0ZXIKKyAgICAgICAgaXQgaXMgZGVzdHJveWVkLgorCisgICAgICAg ICogYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6Q29kZUJsb2NrOjp+Q29k ZUJsb2NrKToKKwogMjAxMS0wOS0wNiAgTWFyayBIYWhuZW5iZXJnICA8bWhhaG5lbmJlcmdAYXBw bGUuY29tPgogCiAgICAgICAgIEZpeCBicm9rZW4gUFBDIGJ1aWxkIGR1ZSB0byBuZXcgZHRvYSBs aWJyYXJ5CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2NrLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2Nr LmNwcAkocmV2aXNpb24gOTQ2MTIpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUv Q29kZUJsb2NrLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTQ0OCw5ICsxNDQ4LDE0IEBAIENvZGVC bG9jazo6fkNvZGVCbG9jaygpCiAgICAgfQogI2VuZGlmCiAgICAgCi0gICAgLy8gV2Ugc2hvdWxk IG5vdCBiZSBnYXJiYWdlIGNvbGxlY3RlZCBpZiB0aGVyZSBhcmUgaW5jb21pbmcgY2FsbHMuIEJ1 dAotICAgIC8vIGlmIHRoaXMgaXMgY2FsbGVkIGR1cmluZyBoZWFwIGRlc3RydWN0aW9uLCB0aGVu IHRoZXJlIG1heSBzdGlsbCBiZQotICAgIC8vIGluY29taW5nIGNhbGxzLCB3aGljaCBpcyBoYXJt bGVzcy4KKyAgICAvLyBXZSBtYXkgYmUgZGVzdHJveWVkIGJlZm9yZSBhbnkgQ29kZUJsb2NrcyB0 aGF0IHJlZmVyIHRvIHVzIGFyZSBkZXN0cm95ZWQuCisgICAgLy8gQ29uc2lkZXIgdGhhdCB0d28g Q29kZUJsb2NrcyBiZWNvbWUgdW5yZWFjaGFibGUgYXQgdGhlIHNhbWUgdGltZS4gVGhlcmUKKyAg ICAvLyBpcyBubyBndWFyYW50ZWUgYWJvdXQgdGhlIG9yZGVyIGluIHdoaWNoIHRoZSBDb2RlQmxv Y2tzIGFyZSBkZXN0cm95ZWQuCisgICAgLy8gU28sIGlmIHdlIGRvbid0IHJlbW92ZSBpbmNvbWlu ZyBjYWxscywgYW5kIGdldCBkZXN0cm95ZWQgYmVmb3JlIHRoZQorICAgIC8vIENvZGVCbG9jayhz KSB0aGF0IGhhdmUgY2FsbHMgaW50byB1cywgdGhlbiB0aGUgQ2FsbExpbmtJbmZvIHZlY3Rvcidz CisgICAgLy8gZGVzdHJ1Y3RvciB3aWxsIHRyeSB0byByZW1vdmUgbm9kZXMgZnJvbSBvdXIgKG5v IGxvbmdlciB2YWxpZCkgbGlua2VkIGxpc3QuCisgICAgd2hpbGUgKG1faW5jb21pbmdDYWxscy5i ZWdpbigpICE9IG1faW5jb21pbmdDYWxscy5lbmQoKSkKKyAgICAgICAgbV9pbmNvbWluZ0NhbGxz LmJlZ2luKCktPnJlbW92ZSgpOwogICAgIAogICAgIC8vIE5vdGUgdGhhdCBvdXIgb3V0Z29pbmcg Y2FsbHMgd2lsbCBiZSByZW1vdmVkIGZyb20gb3RoZXIgQ29kZUJsb2NrcycKICAgICAvLyBtX2lu Y29taW5nQ2FsbHMgbGlua2VkIGxpc3RzIHRocm91Z2ggdGhlIGV4ZWN1dGlvbiBvZiB0aGUgfkNh bGxMaW5rSW5mbwo=